On 7/11/23 22:46, Aaron Conole wrote:

Eric Garver [off-list ref] writes:

quoted

On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote:

quoted

On 7/8/23 00:06, Jakub Kicinski wrote:

quoted

On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote:

quoted

quoted

quoted

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

Sounds like a good plan. Thanks.

quoted

quoted

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.