All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ido Schimmel <idosch@idosch.org>
To: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
Cc: netdev@vger.kernel.org, jiri@nvidia.com, davem@davemloft.net,
	linux-kernel@vger.kernel.org, kuba@kernel.org
Subject: Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action
Date: Mon, 1 Feb 2021 21:08:56 +0200	[thread overview]
Message-ID: <20210201190856.GA3458001@shredder.lan> (raw)
In-Reply-To: <20210125123856.1746-1-oleksandr.mazur@plvision.eu>

I missed this patch. Please Cc me on future versions given I commented
on previous versions.

On Mon, Jan 25, 2021 at 02:38:56PM +0200, Oleksandr Mazur wrote:
> Whenever query statistics is issued for trap with DROP action,
> devlink subsystem would also fill-in statistics 'dropped' field.
> In case if device driver did't register callback for hard drop
> statistics querying, 'dropped' field will be omitted and not filled.
> Add trap_drop_counter_get callback implementation to the netdevsim.
> Add new test cases for netdevsim, to test both the callback
> functionality, as well as drop statistics alteration check.
> 
> Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>

[...]

> +static int devlink_trap_stats_put(struct sk_buff *msg, struct devlink *devlink,
> +				  const struct devlink_trap_item *trap_item)
> +{
> +	struct devlink_stats stats;
> +	struct nlattr *attr;
> +	u64 drops = 0;
> +	int err;
> +
> +	if (trap_item->action == DEVLINK_TRAP_ACTION_DROP &&
> +	    devlink->ops->trap_drop_counter_get) {
> +		err = devlink->ops->trap_drop_counter_get(devlink,
> +							  trap_item->trap,
> +							  &drops);
> +		if (err)
> +			return err;
> +	}
> +
> +	devlink_trap_stats_read(trap_item->stats, &stats);
> +
> +	attr = nla_nest_start(msg, DEVLINK_ATTR_STATS);
> +	if (!attr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_DROPPED, drops,
> +			      DEVLINK_ATTR_PAD))

Commit message says: "In case if device driver did't register callback
for hard drop statistics querying, 'dropped' field will be omitted and
not filled."

But looks like this attribute is always reported to user space.

> +		goto nla_put_failure;
> +
> +	if (trap_item->action == DEVLINK_TRAP_ACTION_DROP &&
> +	    devlink->ops->trap_drop_counter_get &&
> +	    nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_PACKETS,
> +			      stats.rx_packets, DEVLINK_ATTR_PAD))

This is needed for DEVLINK_ATTR_STATS_RX_DROPPED, not for
DEVLINK_ATTR_STATS_RX_PACKETS.

I don't think it makes sense for a counter to come and go based on the
action. It should always be reported (if device supports it), regardless
of current action. Note that the first check will result in this counter
being reported as zero when the action is not drop, but as non-zero
otherwise. That's not good because the basic property of a counter is
that it is monotonically increasing.

> +			goto nla_put_failure;
> +
> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_BYTES,
> +			      stats.rx_bytes, DEVLINK_ATTR_PAD))
> +		goto nla_put_failure;
> +
> +	nla_nest_end(msg, attr);
> +
> +	return 0;
> +
> +nla_put_failure:
> +	nla_nest_cancel(msg, attr);
> +	return -EMSGSIZE;
> +}

      parent reply	other threads:[~2021-02-01 19:10 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-25 12:38 [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action Oleksandr Mazur
2021-01-25 21:23 ` Jakub Kicinski
2021-01-26 10:10   ` Oleksandr Mazur
     [not found]   ` <AM0P190MB0738FC4657CCB0E435C40B24E4B99@AM0P190MB0738.EURP190.PROD.OUTLOOK.COM>
2021-01-29 19:19     ` Jakub Kicinski
2021-02-01 14:54       ` Oleksandr Mazur
2021-02-01 19:08 ` Ido Schimmel [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210201190856.GA3458001@shredder.lan \
    --to=idosch@idosch.org \
    --cc=davem@davemloft.net \
    --cc=jiri@nvidia.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=oleksandr.mazur@plvision.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.