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