Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action
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
[...]
> +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,
> + );
> + if (err)
> + return err;
> + }
> +
> + devlink_trap_stats_read(trap_item->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;
> +}
Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action
On Fri, 29 Jan 2021 11:15:43 + Oleksandr Mazur wrote: > > >Thinking about it again - if the action can be changed wouldn't it > > >be best for the user to actually get a "HW condition hit" counter, > >> which would increment regardless of SW config (incl. policers)? > > > > >Otherwise if admin logs onto the box and temporarily enables a trap > >> for debug this count would disappear. >> >> But still this counter makes sense only for 'drop' action. >Okay, well, "dropped while trap was disabled" seems a lot less useful >of a definition than "number of times this trap would trigger" but if >that's all the HW can provide then it is what it is. >Does the HW also count packets dropped because of overload / overflow >or some other event, or purely dropped because disabled? Hw starts counting traffic (hw drops) only when action has been explicitly set to be 'DROP';
Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action
On Fri, 29 Jan 2021 11:15:43 + Oleksandr Mazur wrote: > > Thinking about it again - if the action can be changed wouldn't it > > be best for the user to actually get a "HW condition hit" counter, > > which would increment regardless of SW config (incl. policers)? > > > Otherwise if admin logs onto the box and temporarily enables a trap > > for debug this count would disappear. > > But still this counter makes sense only for 'drop' action. Okay, well, "dropped while trap was disabled" seems a lot less useful of a definition than "number of times this trap would trigger" but if that's all the HW can provide then it is what it is. Does the HW also count packets dropped because of overload / overflow or some other event, or purely dropped because disabled?
Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action
On Mon, 25 Jan 2021 14:38:56 +0200 Oleksandr Mazur wrote:
> + 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,
> + );
> + if (err)
> + return err;
> + }
> Why only report this counter when action is set to drop?
> Thinking about it again - if the action can be changed wouldn't it
> be best for the user to actually get a "HW condition hit" counter,
> which would increment regardless of SW config (incl. policers)?
> Otherwise if admin logs onto the box and temporarily enables a trap
> for debug this count would disappear.
Okay, so should it become like generic HW counter - trap_hw_counter_get?
Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action
On Mon, 25 Jan 2021 14:38:56 +0200 Oleksandr Mazur wrote:
> + 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,
> + );
> + if (err)
> + return err;
> + }
Why only report this counter when action is set to drop?
Thinking about it again - if the action can be changed wouldn't it
be best for the user to actually get a "HW condition hit" counter,
which would increment regardless of SW config (incl. policers)?
Otherwise if admin logs onto the box and temporarily enables a trap
for debug this count would disappear.
