lkml.org 
[lkml]   [2021]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2021-01-23 17:05    [W:0.085 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site