* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
@ 2021-01-22 8:36 Oleksandr Mazur
2021-01-23 1:12 ` Jakub Kicinski
2021-01-23 16:03 ` Ido Schimmel
0 siblings, 2 replies; 13+ messages in thread
From: Oleksandr Mazur @ 2021-01-22 8:36 UTC (permalink / raw)
To: Jakub Kicinski, Ido Schimmel; +Cc: netdev, jiri, davem, linux-kernel
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-22 8:36 [PATCH net-next] net: core: devlink: add new trap action HARD_DROP Oleksandr Mazur
@ 2021-01-23 1:12 ` Jakub Kicinski
2021-01-23 16:03 ` Ido Schimmel
1 sibling, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2021-01-23 1:12 UTC (permalink / raw)
To: Oleksandr Mazur; +Cc: Ido Schimmel, netdev, jiri, davem, linux-kernel
On Fri, 22 Jan 2021 08:36:01 +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.
Sure, but the action is drop. The statistics on the trap are orthogonal
to the action.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-22 8:36 [PATCH net-next] net: core: devlink: add new trap action HARD_DROP Oleksandr Mazur
2021-01-23 1:12 ` Jakub Kicinski
@ 2021-01-23 16:03 ` Ido Schimmel
2021-01-23 20:14 ` Jakub Kicinski
1 sibling, 1 reply; 13+ messages in thread
From: Ido Schimmel @ 2021-01-23 16:03 UTC (permalink / raw)
To: Oleksandr Mazur; +Cc: Jakub Kicinski, netdev, jiri, davem, linux-kernel
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-23 16:03 ` Ido Schimmel
@ 2021-01-23 20:14 ` Jakub Kicinski
2021-01-24 8:45 ` Ido Schimmel
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2021-01-23 20:14 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Oleksandr Mazur, netdev, jiri, davem, linux-kernel
On Sat, 23 Jan 2021 18:03:48 +0200 Ido Schimmel wrote:
> [DEVLINK_ATTR_STATS_RX_DROPPED]
nit: maybe discarded? dropped sounds like may have been due to an
overflow or something
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-23 20:14 ` Jakub Kicinski
@ 2021-01-24 8:45 ` Ido Schimmel
0 siblings, 0 replies; 13+ messages in thread
From: Ido Schimmel @ 2021-01-24 8:45 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Oleksandr Mazur, netdev, jiri, davem, linux-kernel
On Sat, Jan 23, 2021 at 12:14:39PM -0800, Jakub Kicinski wrote:
> On Sat, 23 Jan 2021 18:03:48 +0200 Ido Schimmel wrote:
> > [DEVLINK_ATTR_STATS_RX_DROPPED]
>
> nit: maybe discarded? dropped sounds like may have been due to an
> overflow or something
Well, it's an existing attribute which is why I suggested using it.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-25 14:56 ` Jiri Pirko
@ 2021-01-25 17:40 ` Ido Schimmel
0 siblings, 0 replies; 13+ messages in thread
From: Ido Schimmel @ 2021-01-25 17:40 UTC (permalink / raw)
To: Jiri Pirko
Cc: Oleksandr Mazur, Jakub Kicinski, netdev, jiri, davem, linux-kernel
On Mon, Jan 25, 2021 at 03:56:14PM +0100, Jiri Pirko wrote:
> Mon, Jan 25, 2021 at 01:24:27PM CET, oleksandr.mazur@plvision.eu wrote:
> >Thu, Jan 21, 2021 at 06:36:05PM CET, kuba@kernel.org 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.
> >
> >>+1
> >Device might be unable to trap only the 'DROP' packets, and this information should be transparent for the user.
> >
> >I agree on the statement, that new action might be an overhead.
> >I could continue on with the solution Ido Schimmel proposed: since no new action would be needed and no UAPI changes are required, i could simply do the dropped statistics (additional field) output added upon trap stats queiring.
> >(In case if driver registerd callback, of course; and do so only for DROP actions)
>
> It is not "a trap". You just need to count dropped packet. You don't
> trap anything. That is why I don't think this has anything to do with
> "trap" infra.
From [1] I understand that it is a trap and the action can be switched,
but when it is 'drop', the hardware can provide statistics about number
of packets that were discarded in hardware. If this is correct, then the
suggestion in [2] looks valid to me.
[1] https://lore.kernel.org/netdev/AM0P190MB073828252FFDA3215387765CE4A00@AM0P190MB0738.EURP190.PROD.OUTLOOK.COM/
[2] https://lore.kernel.org/netdev/20210123160348.GB2799851@shredder.lan/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-25 12:24 ` Oleksandr Mazur
@ 2021-01-25 14:56 ` Jiri Pirko
2021-01-25 17:40 ` Ido Schimmel
0 siblings, 1 reply; 13+ messages in thread
From: Jiri Pirko @ 2021-01-25 14:56 UTC (permalink / raw)
To: Oleksandr Mazur
Cc: Jakub Kicinski, Ido Schimmel, netdev, jiri, davem, linux-kernel
Mon, Jan 25, 2021 at 01:24:27PM CET, oleksandr.mazur@plvision.eu wrote:
>Thu, Jan 21, 2021 at 06:36:05PM CET, kuba@kernel.org 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.
>
>>+1
>Device might be unable to trap only the 'DROP' packets, and this information should be transparent for the user.
>
>I agree on the statement, that new action might be an overhead.
>I could continue on with the solution Ido Schimmel proposed: since no new action would be needed and no UAPI changes are required, i could simply do the dropped statistics (additional field) output added upon trap stats queiring.
>(In case if driver registerd callback, of course; and do so only for DROP actions)
It is not "a trap". You just need to count dropped packet. You don't
trap anything. That is why I don't think this has anything to do with
"trap" infra.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-25 12:12 ` Jiri Pirko
@ 2021-01-25 12:24 ` Oleksandr Mazur
2021-01-25 14:56 ` Jiri Pirko
0 siblings, 1 reply; 13+ messages in thread
From: Oleksandr Mazur @ 2021-01-25 12:24 UTC (permalink / raw)
To: Jiri Pirko, Jakub Kicinski
Cc: Ido Schimmel, netdev, jiri, davem, linux-kernel
Thu, Jan 21, 2021 at 06:36:05PM CET, kuba@kernel.org 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.
>+1
Device might be unable to trap only the 'DROP' packets, and this information should be transparent for the user.
I agree on the statement, that new action might be an overhead.
I could continue on with the solution Ido Schimmel proposed: since no new action would be needed and no UAPI changes are required, i could simply do the dropped statistics (additional field) output added upon trap stats queiring.
(In case if driver registerd callback, of course; and do so only for DROP actions)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-21 17:36 ` Jakub Kicinski
@ 2021-01-25 12:12 ` Jiri Pirko
2021-01-25 12:24 ` Oleksandr Mazur
0 siblings, 1 reply; 13+ messages in thread
From: Jiri Pirko @ 2021-01-25 12:12 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Ido Schimmel, Oleksandr Mazur, netdev, jiri, davem, linux-kernel
Thu, Jan 21, 2021 at 06:36:05PM CET, kuba@kernel.org 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.
+1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-21 12:21 ` Ido Schimmel
2021-01-21 17:36 ` Jakub Kicinski
@ 2021-01-22 8:30 ` Oleksandr Mazur
1 sibling, 0 replies; 13+ messages in thread
From: Oleksandr Mazur @ 2021-01-22 8:30 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, jiri, davem, linux-kernel, kuba
From: Ido Schimmel <idosch@idosch.org>
Sent: Thursday, January 21, 2021 2:21 PM
To: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
Cc: netdev@vger.kernel.org <netdev@vger.kernel.org>; jiri@nvidia.com <jiri@nvidia.com>; davem@davemloft.net <davem@davemloft.net>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; kuba@kernel.org <kuba@kernel.org>
Subject: Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
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?
Global statistics. Basically, it’s the DROP action, with the only difference that device might be unable to post the packet to the devlink subsystem.
Also, as this is an action, it could also be altered: e.g. changed to ‘mirror’ or else.
> Anyway, this patch really needs to be marked as "RFC" since we cannot
> add infrastructure without anyone using it.
Will do.
Also, should I make a V2 patch, that will already hold the RFC tag and the changes (which include the commentaries fixes)?
> Additionally, the documentation
> (Documentation/networking/devlink/devlink-trap.rst) needs to be updated,
> netdevsim needs to be patched and the test over netdevsim
> (tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh) needs to
> be extended to cover the new functionality.
Okay. Will do.
>> @@ -9876,6 +9915,9 @@ void devlink_trap_report(struct devlink *devlink, struct sk_buff *skb,
>> {
>> struct devlink_trap_item *trap_item = trap_ctx;
>>
>> + if (trap_item->action == DEVLINK_TRAP_ACTION_HARD_DROP)
>> + return;
>How can this happen?
My bad. Will get removed in V2.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-21 12:21 ` Ido Schimmel
@ 2021-01-21 17:36 ` Jakub Kicinski
2021-01-25 12:12 ` Jiri Pirko
2021-01-22 8:30 ` Oleksandr Mazur
1 sibling, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2021-01-21 17:36 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Oleksandr Mazur, netdev, jiri, davem, linux-kernel
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
2021-01-21 11:29 Oleksandr Mazur
@ 2021-01-21 12:21 ` Ido Schimmel
2021-01-21 17:36 ` Jakub Kicinski
2021-01-22 8:30 ` Oleksandr Mazur
0 siblings, 2 replies; 13+ messages in thread
From: Ido Schimmel @ 2021-01-21 12:21 UTC (permalink / raw)
To: Oleksandr Mazur; +Cc: netdev, jiri, davem, linux-kernel, kuba
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.
Anyway, this patch really needs to be marked as "RFC" since we cannot
add infrastructure without anyone using it.
Additionally, the documentation
(Documentation/networking/devlink/devlink-trap.rst) needs to be updated,
netdevsim needs to be patched and the test over netdevsim
(tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh) needs to
be extended to cover the new functionality.
More comments below.
>
> Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
> ---
> include/net/devlink.h | 10 ++++++++
> include/uapi/linux/devlink.h | 4 ++++
> net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++-
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index f466819cc477..6811a614f6fd 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1294,6 +1294,16 @@ struct devlink_ops {
> const struct devlink_trap_group *group,
> enum devlink_trap_action action,
> struct netlink_ext_ack *extack);
> + /**
> + * @trap_hard_drop_counter_get: Trap hard drop counter get function.
> + *
> + * Should be used by device drivers to report number of packets dropped
> + * by the underlying device, that have been dropped because device
> + * failed to pass the trapped packet.
> + */
> + int (*trap_hard_drop_counter_get)(struct devlink *devlink,
> + const struct devlink_trap *trap,
> + u64 *p_drops);
> /**
> * @trap_policer_init: Trap policer initialization function.
> *
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index cf89c318f2ac..9247d9c7db03 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -261,12 +261,16 @@ enum {
> * enum devlink_trap_action - Packet trap action.
> * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
> * sent to the CPU.
> + * @DEVLINK_TRAP_ACTION_HARD_DROP: Packet was dropped by the underlying device,
> + * and device cannot report packet to devlink
> + * (or inject it into the kernel RX path).
> * @DEVLINK_TRAP_ACTION_TRAP: The sole copy of the packet is sent to the CPU.
> * @DEVLINK_TRAP_ACTION_MIRROR: Packet is forwarded by the device and a copy is
> * sent to the CPU.
> */
> enum devlink_trap_action {
> DEVLINK_TRAP_ACTION_DROP,
> + DEVLINK_TRAP_ACTION_HARD_DROP,
This breaks uAPI. New values should be added at the end.
> DEVLINK_TRAP_ACTION_TRAP,
> DEVLINK_TRAP_ACTION_MIRROR,
> };
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index ee828e4b1007..5a06e00429e1 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -6732,6 +6732,7 @@ devlink_trap_action_get_from_info(struct genl_info *info,
> val = nla_get_u8(info->attrs[DEVLINK_ATTR_TRAP_ACTION]);
> switch (val) {
> case DEVLINK_TRAP_ACTION_DROP:
> + case DEVLINK_TRAP_ACTION_HARD_DROP:
> case DEVLINK_TRAP_ACTION_TRAP:
> case DEVLINK_TRAP_ACTION_MIRROR:
> *p_trap_action = val;
> @@ -6820,6 +6821,37 @@ static int devlink_trap_stats_put(struct sk_buff *msg,
> return -EMSGSIZE;
> }
>
> +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;
>
> @@ -9697,6 +9732,10 @@ devlink_trap_register(struct devlink *devlink,
> if (devlink_trap_item_lookup(devlink, trap->name))
> return -EEXIST;
>
> + if (trap->init_action == DEVLINK_TRAP_ACTION_HARD_DROP &&
> + !devlink->ops->trap_hard_drop_counter_get)
> + return -EINVAL;
> +
> trap_item = kzalloc(sizeof(*trap_item), GFP_KERNEL);
> if (!trap_item)
> return -ENOMEM;
> @@ -9876,6 +9915,9 @@ void devlink_trap_report(struct devlink *devlink, struct sk_buff *skb,
> {
> struct devlink_trap_item *trap_item = trap_ctx;
>
> + if (trap_item->action == DEVLINK_TRAP_ACTION_HARD_DROP)
> + return;
How can this happen?
> +
> devlink_trap_stats_update(trap_item->stats, skb->len);
> devlink_trap_stats_update(trap_item->group_item->stats, skb->len);
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next] net: core: devlink: add new trap action HARD_DROP
@ 2021-01-21 11:29 Oleksandr Mazur
2021-01-21 12:21 ` Ido Schimmel
0 siblings, 1 reply; 13+ messages in thread
From: Oleksandr Mazur @ 2021-01-21 11:29 UTC (permalink / raw)
To: netdev; +Cc: jiri, davem, linux-kernel, kuba, Oleksandr Mazur
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.
Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
---
include/net/devlink.h | 10 ++++++++
include/uapi/linux/devlink.h | 4 ++++
net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++-
3 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index f466819cc477..6811a614f6fd 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1294,6 +1294,16 @@ struct devlink_ops {
const struct devlink_trap_group *group,
enum devlink_trap_action action,
struct netlink_ext_ack *extack);
+ /**
+ * @trap_hard_drop_counter_get: Trap hard drop counter get function.
+ *
+ * Should be used by device drivers to report number of packets dropped
+ * by the underlying device, that have been dropped because device
+ * failed to pass the trapped packet.
+ */
+ int (*trap_hard_drop_counter_get)(struct devlink *devlink,
+ const struct devlink_trap *trap,
+ u64 *p_drops);
/**
* @trap_policer_init: Trap policer initialization function.
*
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index cf89c318f2ac..9247d9c7db03 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -261,12 +261,16 @@ enum {
* enum devlink_trap_action - Packet trap action.
* @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
* sent to the CPU.
+ * @DEVLINK_TRAP_ACTION_HARD_DROP: Packet was dropped by the underlying device,
+ * and device cannot report packet to devlink
+ * (or inject it into the kernel RX path).
* @DEVLINK_TRAP_ACTION_TRAP: The sole copy of the packet is sent to the CPU.
* @DEVLINK_TRAP_ACTION_MIRROR: Packet is forwarded by the device and a copy is
* sent to the CPU.
*/
enum devlink_trap_action {
DEVLINK_TRAP_ACTION_DROP,
+ DEVLINK_TRAP_ACTION_HARD_DROP,
DEVLINK_TRAP_ACTION_TRAP,
DEVLINK_TRAP_ACTION_MIRROR,
};
diff --git a/net/core/devlink.c b/net/core/devlink.c
index ee828e4b1007..5a06e00429e1 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6732,6 +6732,7 @@ devlink_trap_action_get_from_info(struct genl_info *info,
val = nla_get_u8(info->attrs[DEVLINK_ATTR_TRAP_ACTION]);
switch (val) {
case DEVLINK_TRAP_ACTION_DROP:
+ case DEVLINK_TRAP_ACTION_HARD_DROP:
case DEVLINK_TRAP_ACTION_TRAP:
case DEVLINK_TRAP_ACTION_MIRROR:
*p_trap_action = val;
@@ -6820,6 +6821,37 @@ static int devlink_trap_stats_put(struct sk_buff *msg,
return -EMSGSIZE;
}
+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;
@@ -9697,6 +9732,10 @@ devlink_trap_register(struct devlink *devlink,
if (devlink_trap_item_lookup(devlink, trap->name))
return -EEXIST;
+ if (trap->init_action == DEVLINK_TRAP_ACTION_HARD_DROP &&
+ !devlink->ops->trap_hard_drop_counter_get)
+ return -EINVAL;
+
trap_item = kzalloc(sizeof(*trap_item), GFP_KERNEL);
if (!trap_item)
return -ENOMEM;
@@ -9876,6 +9915,9 @@ void devlink_trap_report(struct devlink *devlink, struct sk_buff *skb,
{
struct devlink_trap_item *trap_item = trap_ctx;
+ if (trap_item->action == DEVLINK_TRAP_ACTION_HARD_DROP)
+ return;
+
devlink_trap_stats_update(trap_item->stats, skb->len);
devlink_trap_stats_update(trap_item->group_item->stats, skb->len);
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-01-26 5:59 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-22 8:36 [PATCH net-next] net: core: devlink: add new trap action HARD_DROP Oleksandr Mazur
2021-01-23 1:12 ` Jakub Kicinski
2021-01-23 16:03 ` Ido Schimmel
2021-01-23 20:14 ` Jakub Kicinski
2021-01-24 8:45 ` Ido Schimmel
-- strict thread matches above, loose matches on Subject: below --
2021-01-21 11:29 Oleksandr Mazur
2021-01-21 12:21 ` Ido Schimmel
2021-01-21 17:36 ` Jakub Kicinski
2021-01-25 12:12 ` Jiri Pirko
2021-01-25 12:24 ` Oleksandr Mazur
2021-01-25 14:56 ` Jiri Pirko
2021-01-25 17:40 ` Ido Schimmel
2021-01-22 8:30 ` Oleksandr Mazur
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.