Message-ID: <096871e8-3c0b-d5d7-8e68-833ba26b3882@ovn.org>
Date: Mon, 10 Jul 2023 18:51:19 +0200
From: Ilya Maximets <i.maximets@....org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: i.maximets@....org, Eric Garver <eric@...ver.life>,
 Aaron Conole <aconole@...hat.com>, netdev@...r.kernel.org,
 dev@...nvswitch.org, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, "David S. Miller" <davem@...emloft.net>,
 Adrian Moreno <amorenoz@...hat.com>, Eelco Chaudron <echaudro@...hat.com>
Subject: Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

On 7/8/23 00:06, Jakub Kicinski wrote:
> On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote:
>>>> That already exists, right? Johannes added it in the last release for WiFi.  
>>>
>>> I'm not sure.  The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly
>>> to that on a surface.  However, looking closer, any value that can be passed
>>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is
>>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very
>>> possible, because I don't really know wifi code).
>>>
>>> The difference, I guess, is that for openvswitch values will be provided by
>>> the userpsace application via netlink interface.  It'll be just a number not
>>> defined anywhere in the kernel.  Only the subsystem itself will be defined
>>> in order to occupy the range.  Garbage in, same garbage out, from the kernel's
>>> perspective.  
>>
>> To be clear, I think, not defining them in this particular case is better.
>> Definition of every reason that userspace can come up with will add extra
>> uAPI maintenance cost/issues with no practical benefits.  Values are not
>> going to be used for anything outside reporting a drop reason and subsystem
>> offset is not part of uAPI anyway.
> 
> Ah, I see. No, please don't stuff user space defined values into 
> the drop reason. The reasons are for debugging the kernel stack 
> itself. IOW it'd be abuse not reuse.

Makes sense.  I wasn't sure that's a good solution from a kernel perspective
either.  It's better than defining all these reasons, IMO, but it's not good
enough to be considered acceptable, I agree.

How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and
OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ?
One for an explicit drop action with a zero argument and one for an explicit
drop with non-zero argument.

The exact reason for the error can be retrieved by other means, i.e by looking
at the datapath flow dump or OVS logs/traces.

This way we can give a user who is catching packet drop traces a signal that
there was something wrong with an OVS flow and they can look up exact details
from the userspace / flow dump.

The point being, most of the flows will have a zero as a drop action argument,
i.e. a regular explicit packet drop.  It will be hard to figure out which flow
exactly we're hitting without looking at the full flow dump.  And if the value
is non-zero, then it should be immediately obvious which flow is to blame from
the dump, as we should not have a lot of such flows.

This would still allow us to avoid a maintenance burden of defining every case,
which are fairly meaningless for the kernel itself, while having 99% of the
information we may need.

Jakub, do you think this will be acceptable?

Eric, Adrian, Aaron, do you see any problems with such implementation?

P.S. There is a plan to add more drop reasons for other places in openvswitch
     module to catch more regular types of drops like memory issues or upcall
     failures.  So, the drop reason subsystem can be extended later.
     The explicit drop action is a bit of an odd case here.

Best regards, Ilya Maximets.

Powered by blists - more mailing lists