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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev