Message-ID: <a2df3e56-ca0c-a1ff-dd79-6e6b12568da9@ovn.org>
Date: Mon, 10 Jul 2023 20:39:11 +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/10/23 19:01, Jakub Kicinski wrote:
> On Mon, 10 Jul 2023 18:51:19 +0200 Ilya Maximets wrote:
>> 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?
> 
> As far as I understand what you're proposing, yes :)

OK.  Just to spell it all out:

Userspace will install a flow with an OVS_FLOW_CMD_NEW:

  match:ip,tcp,... actions:something,something,drop(0)
  match:ip,udp,... actions:something,something,drop(42)

drop() here represents the OVS_ACTION_ATTR_DROP.

Then, in net/openvswitch/actions.c:do_execute_actions(), while executing
these actions:

  case OVS_ACTION_ATTR_DROP:
      kfree_skb_reason(skb, nla_get_u32(a) ? OVS_DROP_ACTION_WITH_ERROR
                                           : OVS_DROP_ACTION);

Users can enable traces and catch the OVS_DROP_ACTION_WITH_ERROR.
Later they can dump flows with OVS_FLOW_CMD_GET and see that the
error value was 42.

> 
>> 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.
> 
> If you have more than ~4 OvS specific reasons, I wonder if it still
> makes sense to create a reason group/subsystem for OvS (a'la WiFi)?

I believe, we will easily have more than 4 OVS-specific reasons.  A few
from the top of my head:
  - upcall failure (failed to send a packet to userspace)
  - reached the limit for deferred actions
  - reached the recursion limit

So, creation of a reason group/subsystem seems reasonable to me.

Best regards, Ilya Maximets.

Powered by blists - more mailing lists