From: Aaron Conole <aconole@redhat•com>
To: Eric Garver <eric@garver•life>
Cc: Ilya Maximets <i.maximets@ovn•org>,
Jakub Kicinski <kuba@kernel•org>,
netdev@vger•kernel.org, dev@openvswitch•org,
Paolo Abeni <pabeni@redhat•com>,
Eric Dumazet <edumazet@google•com>,
"David S. Miller" <davem@davemloft•net>,
Adrian Moreno <amorenoz@redhat•com>,
Eelco Chaudron <echaudro@redhat•com>
Subject: Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
Date: Tue, 11 Jul 2023 16:46:56 -0400 [thread overview]
Message-ID: <f7tpm4ych1b.fsf@redhat.com> (raw)
In-Reply-To: <ZKxMJOdz8LoAA-A5@egarver-thinkpadt14sgen1.remote.csb> (Eric Garver's message of "Mon, 10 Jul 2023 14:21:24 -0400")
Eric Garver <eric@garver•life> writes:
> On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote:
>> 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?
>
> I see no problems. I'm content with this approach.
+1
>> 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.
>>
next prev parent reply other threads:[~2023-07-11 20:47 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-29 20:30 [PATCH net-next 0/2] net: openvswitch: add drop action Eric Garver
2023-06-29 20:30 ` [PATCH net-next 1/2] net: openvswitch: add drop reasons Eric Garver
2023-06-29 20:30 ` [PATCH net-next 2/2] net: openvswitch: add drop action Eric Garver
2023-06-29 22:46 ` kernel test robot
2023-06-29 22:56 ` kernel test robot
2023-06-30 9:47 ` Simon Horman
2023-06-30 12:29 ` Eric Garver
2023-06-30 13:25 ` [ovs-dev] " Simon Horman
2023-07-06 12:54 ` Aaron Conole
2023-07-06 13:57 ` Eric Garver
2023-07-07 10:30 ` Ilya Maximets
2023-07-07 15:00 ` Jakub Kicinski
2023-07-07 15:29 ` Ilya Maximets
2023-07-07 16:04 ` Ilya Maximets
2023-07-07 22:06 ` Jakub Kicinski
2023-07-10 16:51 ` Ilya Maximets
2023-07-10 17:01 ` Jakub Kicinski
2023-07-10 18:39 ` Ilya Maximets
2023-07-10 19:02 ` Jakub Kicinski
2023-07-10 18:21 ` Eric Garver
2023-07-11 20:46 ` Aaron Conole [this message]
2023-07-12 7:53 ` Adrian Moreno
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f7tpm4ych1b.fsf@redhat.com \
--to=aconole@redhat$(echo .)com \
--cc=amorenoz@redhat$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dev@openvswitch$(echo .)org \
--cc=echaudro@redhat$(echo .)com \
--cc=edumazet@google$(echo .)com \
--cc=eric@garver$(echo .)life \
--cc=i.maximets@ovn$(echo .)org \
--cc=kuba@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox