Messages in this thread Patch in this message | | | Date | Sat, 23 Jan 2021 18:03:48 +0200 | From | Ido Schimmel <> | Subject | Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP |
| |
On Fri, Jan 22, 2021 at 08:36:01AM +0000, Oleksandr Mazur wrote: > On Thu, 21 Jan 2021 14:21:52 +0200 Ido Schimmel wrote: > > On Thu, Jan 21, 2021 at 01:29:37PM +0200, Oleksandr Mazur wrote: > > > Add new trap action HARD_DROP, which can be used by the > > > drivers to register traps, where it's impossible to get > > > packet reported to the devlink subsystem by the device > > > driver, because it's impossible to retrieve dropped packet > > > from the device itself. > > > In order to use this action, driver must also register > > > additional devlink operation - callback that is used > > > to retrieve number of packets that have been dropped by > > > the device. > > > > Are these global statistics about number of packets the hardware dropped > > for a specific reason or are these per-port statistics? > > > > It's a creative use of devlink-trap interface, but I think it makes > > sense. Better to re-use an existing interface than creating yet another > > one. > > > Not sure if I agree, if we can't trap why is it a trap? > > It's just a counter. > > It's just another ACTION for trap item. Action however can be switched, e.g. from HARD_DROP to MIRROR. > > The thing is to be able to configure specific trap to be dropped, and provide a way for the device to report back how many packets have been dropped. > If device is able to report the packet itself, then devlink would be in charge of counting. If not, there should be a way to retrieve these statistics from the devlink.
So no need for another action. Just report these stats via 'DEVLINK_ATTR_STATS_RX_DROPPED' if the hardware supports it.
Currently you do:
+static int +devlink_trap_hard_drop_stats_put(struct sk_buff *msg, + struct devlink *devlink, + const struct devlink_trap_item *trap_item) +{ + struct nlattr *attr; + u64 drops; + int err; + + err = devlink->ops->trap_hard_drop_counter_get(devlink, trap_item->trap, + &drops); + if (err) + return err; + + 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)) + goto nla_put_failure; + + nla_nest_end(msg, attr); + + return 0; + +nla_put_failure: + nla_nest_cancel(msg, attr); + return -EMSGSIZE; +} + static int devlink_nl_trap_fill(struct sk_buff *msg, struct devlink *devlink, const struct devlink_trap_item *trap_item, enum devlink_command cmd, u32 portid, u32 seq, @@ -6857,7 +6889,10 @@ static int devlink_nl_trap_fill(struct sk_buff *msg, struct devlink *devlink, if (err) goto nla_put_failure; - err = devlink_trap_stats_put(msg, trap_item->stats); + if (trap_item->action == DEVLINK_TRAP_ACTION_HARD_DROP) + err = devlink_trap_hard_drop_stats_put(msg, devlink, trap_item); + else + err = devlink_trap_stats_put(msg, trap_item->stats); if (err) goto nla_put_failure;
Which means that user space will see stats come and go based on the trap's action. That's not desirable. Instead, change: [DEVLINK_ATTR_STATS] [DEVLINK_ATTR_STATS_RX_PACKETS] [DEVLINK_ATTR_STATS_RX_BYTES]
To:
[DEVLINK_ATTR_STATS] [DEVLINK_ATTR_STATS_RX_PACKETS] [DEVLINK_ATTR_STATS_RX_BYTES] [DEVLINK_ATTR_STATS_RX_DROPPED]
Where the last attribute is reported to user space for devices that support such stats. No changes required in uAPI / iproute2.
| |