Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 1 Feb 2021 21:08:56 +0200
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
Message-ID: <20210201190856.GA3458001@shredder.lan>
References: <20210125123856.1746-1-oleksandr.mazur@plvision.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210125123856.1746-1-oleksandr.mazur@plvision.eu>
Precedence: bulk

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;
> +}