| Message ID | 20230807164551.553365-2-amorenoz@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | openvswitch: add drop reasons | expand |
Adrian Moreno <amorenoz@redhat.com> writes: > Create a new drop reason subsystem for openvswitch and add the first > drop reason to represent flow drops. > > A flow drop happens when a flow has an empty action-set or there is no > action that consumes the packet (output, userspace, recirc, etc). > > Implementation-wise, most of these skb-consuming actions already call > "consume_skb" internally and return directly from within the > do_execute_actions() loop so with minimal changes we can assume that > any skb that exits the loop normally is a packet drop. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- Acked-by: Aaron Conole <aconole@redhat.com>
On 8/7/23 18:45, Adrian Moreno wrote: > Create a new drop reason subsystem for openvswitch and add the first > drop reason to represent flow drops. > > A flow drop happens when a flow has an empty action-set or there is no > action that consumes the packet (output, userspace, recirc, etc). > > Implementation-wise, most of these skb-consuming actions already call > "consume_skb" internally and return directly from within the > do_execute_actions() loop so with minimal changes we can assume that > any skb that exits the loop normally is a packet drop. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > include/net/dropreason.h | 6 ++++++ > net/openvswitch/actions.c | 12 ++++++++++-- > net/openvswitch/datapath.c | 16 ++++++++++++++++ > net/openvswitch/drop.h | 24 ++++++++++++++++++++++++ > 4 files changed, 56 insertions(+), 2 deletions(-) > create mode 100644 net/openvswitch/drop.h <snip> > diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h > new file mode 100644 > index 000000000000..cdd10629c6be > --- /dev/null > +++ b/net/openvswitch/drop.h > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * OpenvSwitch drop reason list. > + */ > + > +#ifndef OPENVSWITCH_DROP_H > +#define OPENVSWITCH_DROP_H > +#include <net/dropreason.h> > + > +#define OVS_DROP_REASONS(R) \ > + R(OVS_DROP_FLOW) \ Hi, Adrian. Not a full review, just complaining about names. :) The OVS_DROP_FLOW seems a bit confusing and unclear. A "flow drop" is also a strange term to use. Maybe we can somehow express in the name that this drop reason is used when there are no actions left to execute? e.g. OVS_DROP_NO_MORE_ACTIONS or OVS_DROP_LAST_ACTION or OVS_DROP_END_OF_ACTION_LIST or something of that sort? These may seem long, but they are not longer than some other names introduced later in the set. What do yo think? Best regards, Ilya Maximets.
On 8/8/23 20:02, Ilya Maximets wrote: > On 8/7/23 18:45, Adrian Moreno wrote: >> Create a new drop reason subsystem for openvswitch and add the first >> drop reason to represent flow drops. >> >> A flow drop happens when a flow has an empty action-set or there is no >> action that consumes the packet (output, userspace, recirc, etc). >> >> Implementation-wise, most of these skb-consuming actions already call >> "consume_skb" internally and return directly from within the >> do_execute_actions() loop so with minimal changes we can assume that >> any skb that exits the loop normally is a packet drop. >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> include/net/dropreason.h | 6 ++++++ >> net/openvswitch/actions.c | 12 ++++++++++-- >> net/openvswitch/datapath.c | 16 ++++++++++++++++ >> net/openvswitch/drop.h | 24 ++++++++++++++++++++++++ >> 4 files changed, 56 insertions(+), 2 deletions(-) >> create mode 100644 net/openvswitch/drop.h > > <snip> > >> diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h >> new file mode 100644 >> index 000000000000..cdd10629c6be >> --- /dev/null >> +++ b/net/openvswitch/drop.h >> @@ -0,0 +1,24 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * OpenvSwitch drop reason list. >> + */ >> + >> +#ifndef OPENVSWITCH_DROP_H >> +#define OPENVSWITCH_DROP_H >> +#include <net/dropreason.h> >> + >> +#define OVS_DROP_REASONS(R) \ >> + R(OVS_DROP_FLOW) \ > > Hi, Adrian. Not a full review, just complaining about names. :) > > The OVS_DROP_FLOW seems a bit confusing and unclear. A "flow drop" > is also a strange term to use. Maybe we can somehow express in the > name that this drop reason is used when there are no actions left > to execute? e.g. OVS_DROP_NO_MORE_ACTIONS or OVS_DROP_LAST_ACTION > or OVS_DROP_END_OF_ACTION_LIST or something of that sort? These may > seem long, but they are not longer than some other names introduced > later in the set. What do yo think? > Hi Ilya, Thanks for the suggestion. It looks reasonable. I did consider something similar but then it felt like having a bit of an "unexpected" or "involuntary" connotation. Given that there are other drop reasons that are involutary I wanted to somehow differentiate this one from the rest. Semantically it'd mean something like: When a flow is deliberately installed with an empty action list it means "it" (the flow?) _wants_ to drop the packet, that's why I ended at that name. OVS_DROP_LAST_ACTION seems to convey this intentionality as well. I'll can send another version changing all names. Thanks.
diff --git a/include/net/dropreason.h b/include/net/dropreason.h index 685fb37df8e8..56cb7be92244 100644 --- a/include/net/dropreason.h +++ b/include/net/dropreason.h @@ -23,6 +23,12 @@ enum skb_drop_reason_subsys { */ SKB_DROP_REASON_SUBSYS_MAC80211_MONITOR, + /** + * @SKB_DROP_REASON_SUBSYS_OPENVSWITCH: openvswitch drop reasons, + * see net/openvswitch/drop.h + */ + SKB_DROP_REASON_SUBSYS_OPENVSWITCH, + /** @SKB_DROP_REASON_SUBSYS_NUM: number of subsystems defined */ SKB_DROP_REASON_SUBSYS_NUM }; diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index cab1e02b63e0..af676dcac2b4 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -27,6 +27,7 @@ #include <net/sctp/checksum.h> #include "datapath.h" +#include "drop.h" #include "flow.h" #include "conntrack.h" #include "vport.h" @@ -1036,7 +1037,7 @@ static int sample(struct datapath *dp, struct sk_buff *skb, if ((arg->probability != U32_MAX) && (!arg->probability || get_random_u32() > arg->probability)) { if (last) - consume_skb(skb); + kfree_skb_reason(skb, OVS_DROP_FLOW); return 0; } @@ -1297,6 +1298,9 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, if (trace_ovs_do_execute_action_enabled()) trace_ovs_do_execute_action(dp, skb, key, a, rem); + /* Actions that rightfully have to consume the skb should do it + * and return directly. + */ switch (nla_type(a)) { case OVS_ACTION_ATTR_OUTPUT: { int port = nla_get_u32(a); @@ -1332,6 +1336,10 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, output_userspace(dp, skb, key, a, attr, len, OVS_CB(skb)->cutlen); OVS_CB(skb)->cutlen = 0; + if (nla_is_last(a, rem)) { + consume_skb(skb); + return 0; + } break; case OVS_ACTION_ATTR_HASH: @@ -1485,7 +1493,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, } } - consume_skb(skb); + kfree_skb_reason(skb, OVS_DROP_FLOW); return 0; } diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index a6d2a0b1aa21..d33cb739883f 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -41,6 +41,7 @@ #include <net/pkt_cls.h> #include "datapath.h" +#include "drop.h" #include "flow.h" #include "flow_table.h" #include "flow_netlink.h" @@ -2702,6 +2703,17 @@ static struct pernet_operations ovs_net_ops = { .size = sizeof(struct ovs_net), }; +static const char * const ovs_drop_reasons[] = { +#define S(x) (#x), + OVS_DROP_REASONS(S) +#undef S +}; + +static struct drop_reason_list drop_reason_list_ovs = { + .reasons = ovs_drop_reasons, + .n_reasons = ARRAY_SIZE(ovs_drop_reasons), +}; + static int __init dp_init(void) { int err; @@ -2743,6 +2755,9 @@ static int __init dp_init(void) if (err < 0) goto error_unreg_netdev; + drop_reasons_register_subsys(SKB_DROP_REASON_SUBSYS_OPENVSWITCH, + &drop_reason_list_ovs); + return 0; error_unreg_netdev: @@ -2769,6 +2784,7 @@ static void dp_cleanup(void) ovs_netdev_exit(); unregister_netdevice_notifier(&ovs_dp_device_notifier); unregister_pernet_device(&ovs_net_ops); + drop_reasons_unregister_subsys(SKB_DROP_REASON_SUBSYS_OPENVSWITCH); rcu_barrier(); ovs_vport_exit(); ovs_flow_exit(); diff --git a/net/openvswitch/drop.h b/net/openvswitch/drop.h new file mode 100644 index 000000000000..cdd10629c6be --- /dev/null +++ b/net/openvswitch/drop.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * OpenvSwitch drop reason list. + */ + +#ifndef OPENVSWITCH_DROP_H +#define OPENVSWITCH_DROP_H +#include <net/dropreason.h> + +#define OVS_DROP_REASONS(R) \ + R(OVS_DROP_FLOW) \ + /* deliberate comment for trailing \ */ + +enum ovs_drop_reason { + __OVS_DROP_REASON = SKB_DROP_REASON_SUBSYS_OPENVSWITCH << + SKB_DROP_REASON_SUBSYS_SHIFT, +#define ENUM(x) x, + OVS_DROP_REASONS(ENUM) +#undef ENUM + + OVS_DROP_MAX, +}; + +#endif /* OPENVSWITCH_DROP_H */
Create a new drop reason subsystem for openvswitch and add the first drop reason to represent flow drops. A flow drop happens when a flow has an empty action-set or there is no action that consumes the packet (output, userspace, recirc, etc). Implementation-wise, most of these skb-consuming actions already call "consume_skb" internally and return directly from within the do_execute_actions() loop so with minimal changes we can assume that any skb that exits the loop normally is a packet drop. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- include/net/dropreason.h | 6 ++++++ net/openvswitch/actions.c | 12 ++++++++++-- net/openvswitch/datapath.c | 16 ++++++++++++++++ net/openvswitch/drop.h | 24 ++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 net/openvswitch/drop.h