[PATCH net-next v7 00/10] net: openvswitch: Add sample multicasting.

[PATCH net-next v7 00/10] net: openvswitch: Add sample multicasting.

[Adrian Moreno]

** Background **
Currently, OVS supports several packet sampling mechanisms (sFlow,
per-bridge IPFIX, per-flow IPFIX). These end up being translated into a
userspace action that needs to be handled by ovs-vswitchd's handler
threads only to be forwarded to some third party application that
will somehow process the sample and provide observability on the
datapath.

A particularly interesting use-case is controller-driven
per-flow IPFIX sampling where the OpenFlow controller can add metadata
to samples (via two 32bit integers) and this metadata is then available
to the sample-collecting system for correlation.

** Problem **
The fact that sampled traffic share netlink sockets and handler thread
time with upcalls, apart from being a performance bottleneck in the
sample extraction itself, can severely compromise the datapath,
yielding this solution unfit for highly loaded production systems.

Users are left with little options other than guessing what sampling
rate will be OK for their traffic pattern and system load and dealing
with the lost accuracy.

Looking at available infrastructure, an obvious candidated would be
to use psample. However, it's current state does not help with the
use-case at stake because sampled packets do not contain user-defined
metadata.

** Proposal **
This series is an attempt to fix this situation by extending the
existing psample infrastructure to carry a variable length
user-defined cookie.

The main existing user of psample is tc's act_sample. It is also
extended to forward the action's cookie to psample.

Finally, a new OVS action (OVS_SAMPLE_ATTR_PSAMPLE) is created.
It accepts a group and an optional cookie and uses psample to
multicast the packet and the metadata.

--
v6 -> v7:
- Rebased
- Fixed typo in comment.

v5 -> v6:
- Renamed emit_sample -> psample
- Addressed unused variable and conditionally compilation of function.

v4 -> v5:
- Rebased.
- Removed lefover enum value and wrapped some long lines in selftests.

v3 -> v4:
- Rebased.
- Addressed Jakub's comment on private and unused nla attributes.

v2 -> v3:
- Addressed comments from Simon, Aaron and Ilya.
- Dropped probability propagation in nested sample actions.
- Dropped patch v2's 7/9 in favor of a userspace implementation and
consume skb if emit_sample is the last action, same as we do with
userspace.
- Split ovs-dpctl.py features in independent patches.

v1 -> v2:
- Create a new action ("emit_sample") rather than reuse existing
  "sample" one.
- Add probability semantics to psample's sampling rate.
- Store sampling probability in skb's cb area and use it in emit_sample.
- Test combining "emit_sample" with "trunc"
- Drop group_id filtering and tracepoint in psample.

rfc_v2 -> v1:
- Accommodate Ilya's comments.
- Split OVS's attribute in two attributes and simplify internal
handling of psample arguments.
- Extend psample and tc with a user-defined cookie.
- Add a tracepoint to psample to facilitate troubleshooting.

rfc_v1 -> rfc_v2:
- Use psample instead of a new OVS-only multicast group.
- Extend psample and tc with a user-defined cookie.


Adrian Moreno (10):
  net: psample: add user cookie
  net: sched: act_sample: add action cookie to sample
  net: psample: skip packet copy if no listeners
  net: psample: allow using rate as probability
  net: openvswitch: add psample action
  net: openvswitch: store sampling probability in cb.
  selftests: openvswitch: add psample action
  selftests: openvswitch: add userspace parsing
  selftests: openvswitch: parse trunc action
  selftests: openvswitch: add psample test

 Documentation/netlink/specs/ovs_flow.yaml     |  17 ++
 include/net/psample.h                         |   5 +-
 include/uapi/linux/openvswitch.h              |  31 +-
 include/uapi/linux/psample.h                  |  11 +-
 net/openvswitch/Kconfig                       |   1 +
 net/openvswitch/actions.c                     |  65 ++++-
 net/openvswitch/datapath.h                    |   3 +
 net/openvswitch/flow_netlink.c                |  32 ++-
 net/openvswitch/vport.c                       |   1 +
 net/psample/psample.c                         |  16 +-
 net/sched/act_sample.c                        |  12 +
 .../selftests/net/openvswitch/openvswitch.sh  | 115 +++++++-
 .../selftests/net/openvswitch/ovs-dpctl.py    | 272 +++++++++++++++++-
 13 files changed, 565 insertions(+), 16 deletions(-)

-- 
2.45.2


Adrian Moreno (10):
  net: psample: add user cookie
  net: sched: act_sample: add action cookie to sample
  net: psample: skip packet copy if no listeners
  net: psample: allow using rate as probability
  net: openvswitch: add psample action
  net: openvswitch: store sampling probability in cb.
  selftests: openvswitch: add psample action
  selftests: openvswitch: add userspace parsing
  selftests: openvswitch: parse trunc action
  selftests: openvswitch: add psample test

 Documentation/netlink/specs/ovs_flow.yaml     |  17 ++
 include/net/psample.h                         |   5 +-
 include/uapi/linux/openvswitch.h              |  31 +-
 include/uapi/linux/psample.h                  |  11 +-
 net/openvswitch/Kconfig                       |   1 +
 net/openvswitch/actions.c                     |  65 ++++-
 net/openvswitch/datapath.h                    |   3 +
 net/openvswitch/flow_netlink.c                |  32 ++-
 net/openvswitch/vport.c                       |   1 +
 net/psample/psample.c                         |  16 +-
 net/sched/act_sample.c                        |  12 +
 .../selftests/net/openvswitch/openvswitch.sh  | 115 +++++++-
 .../selftests/net/openvswitch/ovs-dpctl.py    | 272 +++++++++++++++++-
 13 files changed, 565 insertions(+), 16 deletions(-)

-- 
2.45.2

[PATCH net-next v7 01/10] net: psample: add user cookie

[Adrian Moreno]

Add a user cookie to the sample metadata so that sample emitters can
provide more contextual information to samples.

If present, send the user cookie in a new attribute:
PSAMPLE_ATTR_USER_COOKIE.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/net/psample.h        | 2 ++
 include/uapi/linux/psample.h | 1 +
 net/psample/psample.c        | 9 ++++++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/net/psample.h b/include/net/psample.h
index 0509d2d6be67..2ac71260a546 100644
--- a/include/net/psample.h
+++ b/include/net/psample.h
@@ -25,6 +25,8 @@ struct psample_metadata {
 	   out_tc_occ_valid:1,
 	   latency_valid:1,
 	   unused:5;
+	const u8 *user_cookie;
+	u32 user_cookie_len;
 };
 
 struct psample_group *psample_group_get(struct net *net, u32 group_num);
diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
index e585db5bf2d2..e80637e1d97b 100644
--- a/include/uapi/linux/psample.h
+++ b/include/uapi/linux/psample.h
@@ -19,6 +19,7 @@ enum {
 	PSAMPLE_ATTR_LATENCY,		/* u64, nanoseconds */
 	PSAMPLE_ATTR_TIMESTAMP,		/* u64, nanoseconds */
 	PSAMPLE_ATTR_PROTO,		/* u16 */
+	PSAMPLE_ATTR_USER_COOKIE,	/* binary, user provided data */
 
 	__PSAMPLE_ATTR_MAX
 };
diff --git a/net/psample/psample.c b/net/psample/psample.c
index a5d9b8446f77..b37488f426bc 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -386,7 +386,9 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
 		   nla_total_size(sizeof(u32)) +	/* group_num */
 		   nla_total_size(sizeof(u32)) +	/* seq */
 		   nla_total_size_64bit(sizeof(u64)) +	/* timestamp */
-		   nla_total_size(sizeof(u16));		/* protocol */
+		   nla_total_size(sizeof(u16)) +	/* protocol */
+		   (md->user_cookie_len ?
+		    nla_total_size(md->user_cookie_len) : 0); /* user cookie */
 
 #ifdef CONFIG_INET
 	tun_info = skb_tunnel_info(skb);
@@ -486,6 +488,11 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
 	}
 #endif
 
+	if (md->user_cookie && md->user_cookie_len &&
+	    nla_put(nl_skb, PSAMPLE_ATTR_USER_COOKIE, md->user_cookie_len,
+		    md->user_cookie))
+		goto error;
+
 	genlmsg_end(nl_skb, data);
 	genlmsg_multicast_netns(&psample_nl_family, group->net, nl_skb, 0,
 				PSAMPLE_NL_MCGRP_SAMPLE, GFP_ATOMIC);
-- 
2.45.2

[PATCH net-next v7 02/10] net: sched: act_sample: add action cookie to sample

[Adrian Moreno]

If the action has a user_cookie, pass it along to the sample so it can
be easily identified.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 net/sched/act_sample.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index a69b53d54039..2ceb4d141b71 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -167,7 +167,9 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
 {
 	struct tcf_sample *s = to_sample(a);
 	struct psample_group *psample_group;
+	u8 cookie_data[TC_COOKIE_MAX_SIZE];
 	struct psample_metadata md = {};
+	struct tc_cookie *user_cookie;
 	int retval;
 
 	tcf_lastuse_update(&s->tcf_tm);
@@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
 		if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
 			skb_push(skb, skb->mac_len);
 
+		rcu_read_lock();
+		user_cookie = rcu_dereference(a->user_cookie);
+		if (user_cookie) {
+			memcpy(cookie_data, user_cookie->data,
+			       user_cookie->len);
+			md.user_cookie = cookie_data;
+			md.user_cookie_len = user_cookie->len;
+		}
+		rcu_read_unlock();
+
 		md.trunc_size = s->truncate ? s->trunc_size : skb->len;
 		psample_sample_packet(psample_group, skb, s->rate, &md);
 
-- 
2.45.2

[PATCH net-next v7 03/10] net: psample: skip packet copy if no listeners

[Adrian Moreno]

If nobody is listening on the multicast group, generating the sample,
which involves copying packet data, seems completely unnecessary.

Return fast in this case.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 net/psample/psample.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/psample/psample.c b/net/psample/psample.c
index b37488f426bc..1c76f3e48dcd 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -376,6 +376,10 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
 	void *data;
 	int ret;
 
+	if (!genl_has_listeners(&psample_nl_family, group->net,
+				PSAMPLE_NL_MCGRP_SAMPLE))
+		return;
+
 	meta_len = (in_ifindex ? nla_total_size(sizeof(u16)) : 0) +
 		   (out_ifindex ? nla_total_size(sizeof(u16)) : 0) +
 		   (md->out_tc_valid ? nla_total_size(sizeof(u16)) : 0) +
-- 
2.45.2

[PATCH net-next v7 04/10] net: psample: allow using rate as probability

[Adrian Moreno]

Although not explicitly documented in the psample module itself, the
definition of PSAMPLE_ATTR_SAMPLE_RATE seems inherited from act_sample.

Quoting tc-sample(8):
"RATE of 100 will lead to an average of one sampled packet out of every
100 observed."

With this semantics, the rates that we can express with an unsigned
32-bits number are very unevenly distributed and concentrated towards
"sampling few packets".
For example, we can express a probability of 2.32E-8% but we
cannot express anything between 100% and 50%.

For sampling applications that are capable of sampling a decent
amount of packets, this sampling rate semantics is not very useful.

Add a new flag to the uAPI that indicates that the sampling rate is
expressed in scaled probability, this is:
- 0 is 0% probability, no packets get sampled.
- U32_MAX is 100% probability, all packets get sampled.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/net/psample.h        |  3 ++-
 include/uapi/linux/psample.h | 10 +++++++++-
 net/psample/psample.c        |  3 +++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/net/psample.h b/include/net/psample.h
index 2ac71260a546..c52e9ebd88dd 100644
--- a/include/net/psample.h
+++ b/include/net/psample.h
@@ -24,7 +24,8 @@ struct psample_metadata {
 	u8 out_tc_valid:1,
 	   out_tc_occ_valid:1,
 	   latency_valid:1,
-	   unused:5;
+	   rate_as_probability:1,
+	   unused:4;
 	const u8 *user_cookie;
 	u32 user_cookie_len;
 };
diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
index e80637e1d97b..b765f0e81f20 100644
--- a/include/uapi/linux/psample.h
+++ b/include/uapi/linux/psample.h
@@ -8,7 +8,11 @@ enum {
 	PSAMPLE_ATTR_ORIGSIZE,
 	PSAMPLE_ATTR_SAMPLE_GROUP,
 	PSAMPLE_ATTR_GROUP_SEQ,
-	PSAMPLE_ATTR_SAMPLE_RATE,
+	PSAMPLE_ATTR_SAMPLE_RATE,	/* u32, ratio between observed and
+					 * sampled packets or scaled probability
+					 * if PSAMPLE_ATTR_SAMPLE_PROBABILITY
+					 * is set.
+					 */
 	PSAMPLE_ATTR_DATA,
 	PSAMPLE_ATTR_GROUP_REFCOUNT,
 	PSAMPLE_ATTR_TUNNEL,
@@ -20,6 +24,10 @@ enum {
 	PSAMPLE_ATTR_TIMESTAMP,		/* u64, nanoseconds */
 	PSAMPLE_ATTR_PROTO,		/* u16 */
 	PSAMPLE_ATTR_USER_COOKIE,	/* binary, user provided data */
+	PSAMPLE_ATTR_SAMPLE_PROBABILITY,/* no argument, interpret rate in
+					 * PSAMPLE_ATTR_SAMPLE_RATE as a
+					 * probability scaled 0 - U32_MAX.
+					 */
 
 	__PSAMPLE_ATTR_MAX
 };
diff --git a/net/psample/psample.c b/net/psample/psample.c
index 1c76f3e48dcd..f48b5b9cd409 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -497,6 +497,9 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
 		    md->user_cookie))
 		goto error;
 
+	if (md->rate_as_probability)
+		nla_put_flag(skb, PSAMPLE_ATTR_SAMPLE_PROBABILITY);
+
 	genlmsg_end(nl_skb, data);
 	genlmsg_multicast_netns(&psample_nl_family, group->net, nl_skb, 0,
 				PSAMPLE_NL_MCGRP_SAMPLE, GFP_ATOMIC);
-- 
2.45.2

[PATCH net-next v7 05/10] net: openvswitch: add psample action

[Adrian Moreno]

Add support for a new action: psample.

This action accepts a u32 group id and a variable-length cookie and uses
the psample multicast group to make the packet available for
observability.

The maximum length of the user-defined cookie is set to 16, same as
tc_cookie, to discourage using cookies that will not be offloadable.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
 include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
 net/openvswitch/Kconfig                   |  1 +
 net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
 net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
 5 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
index 4fdfc6b5cae9..46f5d1cd8a5f 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -727,6 +727,12 @@ attribute-sets:
         name: dec-ttl
         type: nest
         nested-attributes: dec-ttl-attrs
+      -
+        name: psample
+        type: nest
+        nested-attributes: psample-attrs
+        doc: |
+          Sends a packet sample to psample for external observation.
   -
     name: tunnel-key-attrs
     enum-name: ovs-tunnel-key-attr
@@ -938,6 +944,17 @@ attribute-sets:
       -
         name: gbp
         type: u32
+  -
+    name: psample-attrs
+    enum-name: ovs-psample-attr
+    name-prefix: ovs-psample-attr-
+    attributes:
+      -
+        name: group
+        type: u32
+      -
+        name: cookie
+        type: binary
 
 operations:
   name-prefix: ovs-flow-cmd-
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index efc82c318fa2..3dd653748725 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -914,6 +914,31 @@ struct check_pkt_len_arg {
 };
 #endif
 
+#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
+/**
+ * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
+ * action.
+ *
+ * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
+ * sample.
+ * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
+ * contains user-defined metadata. The maximum length is
+ * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
+ *
+ * Sends the packet to the psample multicast group with the specified group and
+ * cookie. It is possible to combine this action with the
+ * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
+ */
+enum ovs_psample_attr {
+	OVS_PSAMPLE_ATTR_GROUP = 1,	/* u32 number. */
+	OVS_PSAMPLE_ATTR_COOKIE,	/* Optional, user specified cookie. */
+
+	/* private: */
+	__OVS_PSAMPLE_ATTR_MAX
+};
+
+#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
+
 /**
  * enum ovs_action_attr - Action types.
  *
@@ -966,6 +991,8 @@ struct check_pkt_len_arg {
  * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
  * argument.
  * @OVS_ACTION_ATTR_DROP: Explicit drop action.
+ * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external observers
+ * via psample.
  *
  * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
  * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -1004,6 +1031,7 @@ enum ovs_action_attr {
 	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
 	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
 	OVS_ACTION_ATTR_DROP,         /* u32 error code. */
+	OVS_ACTION_ATTR_PSAMPLE,      /* Nested OVS_PSAMPLE_ATTR_*. */
 
 	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
 				       * from userspace. */
diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
index 29a7081858cd..2535f3f9f462 100644
--- a/net/openvswitch/Kconfig
+++ b/net/openvswitch/Kconfig
@@ -10,6 +10,7 @@ config OPENVSWITCH
 		   (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
 				     (!NF_NAT || NF_NAT) && \
 				     (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT)))
+	depends on PSAMPLE || !PSAMPLE
 	select LIBCRC32C
 	select MPLS
 	select NET_MPLS_GSO
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 964225580824..a035b7e677dd 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -24,6 +24,11 @@
 #include <net/checksum.h>
 #include <net/dsfield.h>
 #include <net/mpls.h>
+
+#if IS_ENABLED(CONFIG_PSAMPLE)
+#include <net/psample.h>
+#endif
+
 #include <net/sctp/checksum.h>
 
 #include "datapath.h"
@@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_PSAMPLE)
+static void execute_psample(struct datapath *dp, struct sk_buff *skb,
+			    const struct nlattr *attr)
+{
+	struct psample_group psample_group = {};
+	struct psample_metadata md = {};
+	const struct nlattr *a;
+	int rem;
+
+	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
+		switch (nla_type(a)) {
+		case OVS_PSAMPLE_ATTR_GROUP:
+			psample_group.group_num = nla_get_u32(a);
+			break;
+
+		case OVS_PSAMPLE_ATTR_COOKIE:
+			md.user_cookie = nla_data(a);
+			md.user_cookie_len = nla_len(a);
+			break;
+		}
+	}
+
+	psample_group.net = ovs_dp_get_net(dp);
+	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
+	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
+
+	psample_sample_packet(&psample_group, skb, 0, &md);
+}
+#else
+static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
+				   const struct nlattr *attr) {}
+#endif
+
 /* Execute a list of actions against 'skb'. */
 static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			      struct sw_flow_key *key,
@@ -1502,6 +1540,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			ovs_kfree_skb_reason(skb, reason);
 			return 0;
 		}
+
+		case OVS_ACTION_ATTR_PSAMPLE:
+			execute_psample(dp, skb, a);
+			OVS_CB(skb)->cutlen = 0;
+			if (nla_is_last(a, rem)) {
+				consume_skb(skb);
+				return 0;
+			}
+			break;
 		}
 
 		if (unlikely(err)) {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index f224d9bcea5e..c92bdc4dfe19 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
 		case OVS_ACTION_ATTR_TRUNC:
 		case OVS_ACTION_ATTR_USERSPACE:
 		case OVS_ACTION_ATTR_DROP:
+		case OVS_ACTION_ATTR_PSAMPLE:
 			break;
 
 		case OVS_ACTION_ATTR_CT:
@@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
 	/* Whenever new actions are added, the need to update this
 	 * function should be considered.
 	 */
-	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
+	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25);
 
 	if (!actions)
 		return;
@@ -3157,6 +3158,28 @@ static int validate_and_copy_check_pkt_len(struct net *net,
 	return 0;
 }
 
+static int validate_psample(const struct nlattr *attr)
+{
+	static const struct nla_policy policy[OVS_PSAMPLE_ATTR_MAX + 1] = {
+		[OVS_PSAMPLE_ATTR_GROUP] = { .type = NLA_U32 },
+		[OVS_PSAMPLE_ATTR_COOKIE] = {
+			.type = NLA_BINARY,
+			.len = OVS_PSAMPLE_COOKIE_MAX_SIZE,
+		},
+	};
+	struct nlattr *a[OVS_PSAMPLE_ATTR_MAX + 1];
+	int err;
+
+	if (!IS_ENABLED(CONFIG_PSAMPLE))
+		return -EOPNOTSUPP;
+
+	err = nla_parse_nested(a, OVS_PSAMPLE_ATTR_MAX, attr, policy, NULL);
+	if (err)
+		return err;
+
+	return a[OVS_PSAMPLE_ATTR_GROUP] ? 0 : -EINVAL;
+}
+
 static int copy_action(const struct nlattr *from,
 		       struct sw_flow_actions **sfa, bool log)
 {
@@ -3212,6 +3235,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
 			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
 			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
+			[OVS_ACTION_ATTR_PSAMPLE] = (u32)-1,
 		};
 		const struct ovs_action_push_vlan *vlan;
 		int type = nla_type(a);
@@ -3490,6 +3514,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 				return -EINVAL;
 			break;
 
+		case OVS_ACTION_ATTR_PSAMPLE:
+			err = validate_psample(a);
+			if (err)
+				return err;
+			break;
+
 		default:
 			OVS_NLERR(log, "Unknown Action type %d", type);
 			return -EINVAL;
-- 
2.45.2

[PATCH net-next v7 06/10] net: openvswitch: store sampling probability in cb.

[Adrian Moreno]

When a packet sample is observed, the sampling rate that was used is
important to estimate the real frequency of such event.

Store the probability of the parent sample action in the skb's cb area
and use it in psample action to pass it down to psample module.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/uapi/linux/openvswitch.h |  3 ++-
 net/openvswitch/actions.c        | 20 +++++++++++++++++---
 net/openvswitch/datapath.h       |  3 +++
 net/openvswitch/vport.c          |  1 +
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 3dd653748725..3a701bd1f31b 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -649,7 +649,8 @@ enum ovs_flow_attr {
  * Actions are passed as nested attributes.
  *
  * Executes the specified actions with the given probability on a per-packet
- * basis.
+ * basis. Nested actions will be able to access the probability value of the
+ * parent @OVS_ACTION_ATTR_SAMPLE.
  */
 enum ovs_sample_attr {
 	OVS_SAMPLE_ATTR_UNSPEC,
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index a035b7e677dd..34af6bce4085 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1048,12 +1048,15 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
 	struct nlattr *sample_arg;
 	int rem = nla_len(attr);
 	const struct sample_arg *arg;
+	u32 init_probability;
 	bool clone_flow_key;
+	int err;
 
 	/* The first action is always 'OVS_SAMPLE_ATTR_ARG'. */
 	sample_arg = nla_data(attr);
 	arg = nla_data(sample_arg);
 	actions = nla_next(sample_arg, &rem);
+	init_probability = OVS_CB(skb)->probability;
 
 	if ((arg->probability != U32_MAX) &&
 	    (!arg->probability || get_random_u32() > arg->probability)) {
@@ -1062,9 +1065,16 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
 		return 0;
 	}
 
+	OVS_CB(skb)->probability = arg->probability;
+
 	clone_flow_key = !arg->exec;
-	return clone_execute(dp, skb, key, 0, actions, rem, last,
-			     clone_flow_key);
+	err = clone_execute(dp, skb, key, 0, actions, rem, last,
+			    clone_flow_key);
+
+	if (!last)
+		OVS_CB(skb)->probability = init_probability;
+
+	return err;
 }
 
 /* When 'last' is true, clone() should always consume the 'skb'.
@@ -1311,6 +1321,7 @@ static void execute_psample(struct datapath *dp, struct sk_buff *skb,
 	struct psample_group psample_group = {};
 	struct psample_metadata md = {};
 	const struct nlattr *a;
+	u32 rate;
 	int rem;
 
 	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
@@ -1329,8 +1340,11 @@ static void execute_psample(struct datapath *dp, struct sk_buff *skb,
 	psample_group.net = ovs_dp_get_net(dp);
 	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
 	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
+	md.rate_as_probability = 1;
+
+	rate = OVS_CB(skb)->probability ? OVS_CB(skb)->probability : U32_MAX;
 
-	psample_sample_packet(&psample_group, skb, 0, &md);
+	psample_sample_packet(&psample_group, skb, rate, &md);
 }
 #else
 static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
index 0cd29971a907..9ca6231ea647 100644
--- a/net/openvswitch/datapath.h
+++ b/net/openvswitch/datapath.h
@@ -115,12 +115,15 @@ struct datapath {
  * fragmented.
  * @acts_origlen: The netlink size of the flow actions applied to this skb.
  * @cutlen: The number of bytes from the packet end to be removed.
+ * @probability: The sampling probability that was applied to this skb; 0 means
+ * no sampling has occurred; U32_MAX means 100% probability.
  */
 struct ovs_skb_cb {
 	struct vport		*input_vport;
 	u16			mru;
 	u16			acts_origlen;
 	u32			cutlen;
+	u32			probability;
 };
 #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
 
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 972ae01a70f7..8732f6e51ae5 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -500,6 +500,7 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
 	OVS_CB(skb)->input_vport = vport;
 	OVS_CB(skb)->mru = 0;
 	OVS_CB(skb)->cutlen = 0;
+	OVS_CB(skb)->probability = 0;
 	if (unlikely(dev_net(skb->dev) != ovs_dp_get_net(vport->dp))) {
 		u32 mark;
 
-- 
2.45.2

[PATCH net-next v7 07/10] selftests: openvswitch: add psample action

[Adrian Moreno]

Add sample and psample action support to ovs-dpctl.py.

Refactor common attribute parsing logic into an external function.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 .../selftests/net/openvswitch/ovs-dpctl.py    | 162 +++++++++++++++++-
 1 file changed, 161 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 182a09975975..dcc400a21a22 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -8,6 +8,7 @@ import argparse
 import errno
 import ipaddress
 import logging
+import math
 import multiprocessing
 import re
 import socket
@@ -60,6 +61,7 @@ OVS_FLOW_CMD_DEL = 2
 OVS_FLOW_CMD_GET = 3
 OVS_FLOW_CMD_SET = 4
 
+UINT32_MAX = 0xFFFFFFFF
 
 def macstr(mac):
     outstr = ":".join(["%02X" % i for i in mac])
@@ -281,6 +283,75 @@ def parse_extract_field(
     return str_skipped, data
 
 
+def parse_attrs(actstr, attr_desc):
+    """Parses the given action string and returns a list of netlink
+    attributes based on a list of attribute descriptions.
+
+    Each element in the attribute description list is a tuple such as:
+        (name, attr_name, parse_func)
+    where:
+        name: is the string representing the attribute
+        attr_name: is the name of the attribute as defined in the uAPI.
+        parse_func: is a callable accepting a string and returning either
+            a single object (the parsed attribute value) or a tuple of
+            two values (the parsed attribute value and the remaining string)
+
+    Returns a list of attributes and the remaining string.
+    """
+    def parse_attr(actstr, key, func):
+        actstr = actstr[len(key) :]
+
+        if not func:
+            return None, actstr
+
+        delim = actstr[0]
+        actstr = actstr[1:]
+
+        if delim == "=":
+            pos = strcspn(actstr, ",)")
+            ret = func(actstr[:pos])
+        else:
+            ret = func(actstr)
+
+        if isinstance(ret, tuple):
+            (datum, actstr) = ret
+        else:
+            datum = ret
+            actstr = actstr[strcspn(actstr, ",)"):]
+
+        if delim == "(":
+            if not actstr or actstr[0] != ")":
+                raise ValueError("Action contains unbalanced parentheses")
+
+            actstr = actstr[1:]
+
+        actstr = actstr[strspn(actstr, ", ") :]
+
+        return datum, actstr
+
+    attrs = []
+    attr_desc = list(attr_desc)
+    while actstr and actstr[0] != ")" and attr_desc:
+        found = False
+        for i, (key, attr, func) in enumerate(attr_desc):
+            if actstr.startswith(key):
+                datum, actstr = parse_attr(actstr, key, func)
+                attrs.append([attr, datum])
+                found = True
+                del attr_desc[i]
+
+        if not found:
+            raise ValueError("Unknown attribute: '%s'" % actstr)
+
+        actstr = actstr[strspn(actstr, ", ") :]
+
+    if actstr[0] != ")":
+        raise ValueError("Action string contains extra garbage or has "
+                         "unbalanced parenthesis: '%s'" % actstr)
+
+    return attrs, actstr[1:]
+
+
 class ovs_dp_msg(genlmsg):
     # include the OVS version
     # We need a custom header rather than just being able to rely on
@@ -299,7 +370,7 @@ class ovsactions(nla):
         ("OVS_ACTION_ATTR_SET", "ovskey"),
         ("OVS_ACTION_ATTR_PUSH_VLAN", "none"),
         ("OVS_ACTION_ATTR_POP_VLAN", "flag"),
-        ("OVS_ACTION_ATTR_SAMPLE", "none"),
+        ("OVS_ACTION_ATTR_SAMPLE", "sample"),
         ("OVS_ACTION_ATTR_RECIRC", "uint32"),
         ("OVS_ACTION_ATTR_HASH", "none"),
         ("OVS_ACTION_ATTR_PUSH_MPLS", "none"),
@@ -318,8 +389,85 @@ class ovsactions(nla):
         ("OVS_ACTION_ATTR_ADD_MPLS", "none"),
         ("OVS_ACTION_ATTR_DEC_TTL", "none"),
         ("OVS_ACTION_ATTR_DROP", "uint32"),
+        ("OVS_ACTION_ATTR_PSAMPLE", "psample"),
     )
 
+    class psample(nla):
+        nla_flags = NLA_F_NESTED
+
+        nla_map = (
+            ("OVS_PSAMPLE_ATTR_UNSPEC", "none"),
+            ("OVS_PSAMPLE_ATTR_GROUP", "uint32"),
+            ("OVS_PSAMPLE_ATTR_COOKIE", "array(uint8)"),
+        )
+
+        def dpstr(self, more=False):
+            args = "group=%d" % self.get_attr("OVS_PSAMPLE_ATTR_GROUP")
+
+            cookie = self.get_attr("OVS_PSAMPLE_ATTR_COOKIE")
+            if cookie:
+                args += ",cookie(%s)" % \
+                        "".join(format(x, "02x") for x in cookie)
+
+            return "psample(%s)" % args
+
+        def parse(self, actstr):
+            desc = (
+                ("group", "OVS_PSAMPLE_ATTR_GROUP", int),
+                ("cookie", "OVS_PSAMPLE_ATTR_COOKIE",
+                    lambda x: list(bytearray.fromhex(x)))
+            )
+
+            attrs, actstr = parse_attrs(actstr, desc)
+
+            for attr in attrs:
+                self["attrs"].append(attr)
+
+            return actstr
+
+    class sample(nla):
+        nla_flags = NLA_F_NESTED
+
+        nla_map = (
+            ("OVS_SAMPLE_ATTR_UNSPEC", "none"),
+            ("OVS_SAMPLE_ATTR_PROBABILITY", "uint32"),
+            ("OVS_SAMPLE_ATTR_ACTIONS", "ovsactions"),
+        )
+
+        def dpstr(self, more=False):
+            args = []
+
+            args.append("sample={:.2f}%".format(
+                100 * self.get_attr("OVS_SAMPLE_ATTR_PROBABILITY") /
+                UINT32_MAX))
+
+            actions = self.get_attr("OVS_SAMPLE_ATTR_ACTIONS")
+            if actions:
+                args.append("actions(%s)" % actions.dpstr(more))
+
+            return "sample(%s)" % ",".join(args)
+
+        def parse(self, actstr):
+            def parse_nested_actions(actstr):
+                subacts = ovsactions()
+                parsed_len = subacts.parse(actstr)
+                return subacts, actstr[parsed_len :]
+
+            def percent_to_rate(percent):
+                percent = float(percent.strip('%'))
+                return int(math.floor(UINT32_MAX * (percent / 100.0) + .5))
+
+            desc = (
+                ("sample", "OVS_SAMPLE_ATTR_PROBABILITY", percent_to_rate),
+                ("actions", "OVS_SAMPLE_ATTR_ACTIONS", parse_nested_actions),
+            )
+            attrs, actstr = parse_attrs(actstr, desc)
+
+            for attr in attrs:
+                self["attrs"].append(attr)
+
+            return actstr
+
     class ctact(nla):
         nla_flags = NLA_F_NESTED
 
@@ -683,6 +831,18 @@ class ovsactions(nla):
                 self["attrs"].append(["OVS_ACTION_ATTR_CT", ctact])
                 parsed = True
 
+            elif parse_starts_block(actstr, "sample(", False):
+                sampleact = self.sample()
+                actstr = sampleact.parse(actstr[len("sample(") : ])
+                self["attrs"].append(["OVS_ACTION_ATTR_SAMPLE", sampleact])
+                parsed = True
+
+            elif parse_starts_block(actstr, "psample(", False):
+                psampleact = self.psample()
+                actstr = psampleact.parse(actstr[len("psample(") : ])
+                self["attrs"].append(["OVS_ACTION_ATTR_PSAMPLE", psampleact])
+                parsed = True
+
             actstr = actstr[strspn(actstr, ", ") :]
             while parencount > 0:
                 parencount -= 1
-- 
2.45.2

[PATCH net-next v7 08/10] selftests: openvswitch: add userspace parsing

[Adrian Moreno]

The userspace action lacks parsing support plus it contains a bug in the
name of one of its attributes.

This patch makes userspace action work.

Reviewed-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 .../selftests/net/openvswitch/ovs-dpctl.py    | 24 +++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index dcc400a21a22..4ccf26f96327 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -589,13 +589,27 @@ class ovsactions(nla):
                 print_str += "userdata="
                 for f in self.get_attr("OVS_USERSPACE_ATTR_USERDATA"):
                     print_str += "%x." % f
-            if self.get_attr("OVS_USERSPACE_ATTR_TUN_PORT") is not None:
+            if self.get_attr("OVS_USERSPACE_ATTR_EGRESS_TUN_PORT") is not None:
                 print_str += "egress_tun_port=%d" % self.get_attr(
-                    "OVS_USERSPACE_ATTR_TUN_PORT"
+                    "OVS_USERSPACE_ATTR_EGRESS_TUN_PORT"
                 )
             print_str += ")"
             return print_str
 
+        def parse(self, actstr):
+            attrs_desc = (
+                ("pid", "OVS_USERSPACE_ATTR_PID", int),
+                ("userdata", "OVS_USERSPACE_ATTR_USERDATA",
+                    lambda x: list(bytearray.fromhex(x))),
+                ("egress_tun_port", "OVS_USERSPACE_ATTR_EGRESS_TUN_PORT", int)
+            )
+
+            attrs, actstr = parse_attrs(actstr, attrs_desc)
+            for attr in attrs:
+                self["attrs"].append(attr)
+
+            return actstr
+
     def dpstr(self, more=False):
         print_str = ""
 
@@ -843,6 +857,12 @@ class ovsactions(nla):
                 self["attrs"].append(["OVS_ACTION_ATTR_PSAMPLE", psampleact])
                 parsed = True
 
+            elif parse_starts_block(actstr, "userspace(", False):
+                uact = self.userspace()
+                actstr = uact.parse(actstr[len("userspace(") : ])
+                self["attrs"].append(["OVS_ACTION_ATTR_USERSPACE", uact])
+                parsed = True
+
             actstr = actstr[strspn(actstr, ", ") :]
             while parencount > 0:
                 parencount -= 1
-- 
2.45.2

[PATCH net-next v7 09/10] selftests: openvswitch: parse trunc action

[Adrian Moreno]

The trunc action was supported decode-able but not parse-able. Add
support for parsing the action string.

Reviewed-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 .../testing/selftests/net/openvswitch/ovs-dpctl.py  | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 4ccf26f96327..e8dc9af10d4d 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -863,6 +863,19 @@ class ovsactions(nla):
                 self["attrs"].append(["OVS_ACTION_ATTR_USERSPACE", uact])
                 parsed = True
 
+            elif parse_starts_block(actstr, "trunc(", False):
+                parencount += 1
+                actstr, val = parse_extract_field(
+                    actstr,
+                    "trunc(",
+                    r"([0-9]+)",
+                    int,
+                    False,
+                    None,
+                )
+                self["attrs"].append(["OVS_ACTION_ATTR_TRUNC", val])
+                parsed = True
+
             actstr = actstr[strspn(actstr, ", ") :]
             while parencount > 0:
                 parencount -= 1
-- 
2.45.2

[PATCH net-next v7 10/10] selftests: openvswitch: add psample test

[Adrian Moreno]

Add a test to verify sampling packets via psample works.

In order to do that, create a subcommand in ovs-dpctl.py to listen to
on the psample multicast group and print samples.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 .../selftests/net/openvswitch/openvswitch.sh  | 115 +++++++++++++++++-
 .../selftests/net/openvswitch/ovs-dpctl.py    |  73 ++++++++++-
 2 files changed, 182 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 15bca0708717..02a366e01004 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -20,7 +20,8 @@ tests="
 	nat_related_v4				ip4-nat-related: ICMP related matches work with SNAT
 	netlink_checks				ovsnl: validate netlink attrs and settings
 	upcall_interfaces			ovs: test the upcall interfaces
-	drop_reason				drop: test drop reasons are emitted"
+	drop_reason				drop: test drop reasons are emitted
+	psample					psample: Sampling packets with psample"
 
 info() {
     [ $VERBOSE = 0 ] || echo $*
@@ -102,12 +103,21 @@ ovs_netns_spawn_daemon() {
 	shift
 	netns=$1
 	shift
-	info "spawning cmd: $*"
-	ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
+	if [ "$netns" == "_default" ]; then
+		$*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
+	else
+		ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
+	fi
 	pid=$!
 	ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
 }
 
+ovs_spawn_daemon() {
+	sbx=$1
+	shift
+	ovs_netns_spawn_daemon $sbx "_default" $*
+}
+
 ovs_add_netns_and_veths () {
 	info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
 	ovs_sbx "$1" ip netns add "$3" || return 1
@@ -170,6 +180,19 @@ ovs_drop_reason_count()
 	return `echo "$perf_output" | grep "$pattern" | wc -l`
 }
 
+ovs_test_flow_fails () {
+	ERR_MSG="Flow actions may not be safe on all matching packets"
+
+	PRE_TEST=$(dmesg | grep -c "${ERR_MSG}")
+	ovs_add_flow $@ &> /dev/null $@ && return 1
+	POST_TEST=$(dmesg | grep -c "${ERR_MSG}")
+
+	if [ "$PRE_TEST" == "$POST_TEST" ]; then
+		return 1
+	fi
+	return 0
+}
+
 usage() {
 	echo
 	echo "$0 [OPTIONS] [TEST]..."
@@ -184,6 +207,92 @@ usage() {
 	exit 1
 }
 
+
+# psample test
+# - use psample to observe packets
+test_psample() {
+	sbx_add "test_psample" || return $?
+
+	# Add a datapath with per-vport dispatching.
+	ovs_add_dp "test_psample" psample -V 2:1 || return 1
+
+	info "create namespaces"
+	ovs_add_netns_and_veths "test_psample" "psample" \
+		client c0 c1 172.31.110.10/24 -u || return 1
+	ovs_add_netns_and_veths "test_psample" "psample" \
+		server s0 s1 172.31.110.20/24 -u || return 1
+
+	# Check if psample actions can be configured.
+	ovs_add_flow "test_psample" psample \
+	'in_port(1),eth(),eth_type(0x0806),arp()' 'psample(group=1)'
+	if [ $? == 1 ]; then
+		info "no support for psample - skipping"
+		ovs_exit_sig
+		return $ksft_skip
+	fi
+
+	ovs_del_flows "test_psample" psample
+
+	# Test action verification.
+	OLDIFS=$IFS
+	IFS='*'
+	min_key='in_port(1),eth(),eth_type(0x0800),ipv4()'
+	for testcase in \
+		"cookie to large"*"psample(group=1,cookie=1615141312111009080706050403020100)" \
+		"no group with cookie"*"psample(cookie=abcd)" \
+		"no group"*"psample()";
+	do
+		set -- $testcase;
+		ovs_test_flow_fails "test_psample" psample $min_key $2
+		if [ $? == 1 ]; then
+			info "failed - $1"
+			return 1
+		fi
+	done
+	IFS=$OLDIFS
+
+	ovs_del_flows "test_psample" psample
+	# Allow ARP
+	ovs_add_flow "test_psample" psample \
+		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
+	ovs_add_flow "test_psample" psample \
+		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
+
+	# Sample first 14 bytes of all traffic.
+	ovs_add_flow "test_psample" psample \
+	    "in_port(1),eth(),eth_type(0x0800),ipv4()" \
+            "trunc(14),psample(group=1,cookie=c0ffee),2"
+
+	# Sample all traffic. In this case, use a sample() action with both
+	# psample and an upcall emulating simultaneous local sampling and
+	# sFlow / IPFIX.
+	nlpid=$(grep -E "listening on upcall packet handler" \
+            $ovs_dir/s0.out | cut -d ":" -f 2 | tr -d ' ')
+
+	ovs_add_flow "test_psample" psample \
+            "in_port(2),eth(),eth_type(0x0800),ipv4()" \
+            "sample(sample=100%,actions(psample(group=2,cookie=eeff0c),userspace(pid=${nlpid},userdata=eeff0c))),1"
+
+	# Record psample data.
+	ovs_spawn_daemon "test_psample" python3 $ovs_base/ovs-dpctl.py psample-events
+
+	# Send a single ping.
+	sleep 1
+	ovs_sbx "test_psample" ip netns exec client ping -I c1 172.31.110.20 -c 1 || return 1
+	sleep 1
+
+	# We should have received one userspace action upcall and 2 psample packets.
+	grep -E "userspace action command" $ovs_dir/s0.out >/dev/null 2>&1 || return 1
+
+	# client -> server samples should only contain the first 14 bytes of the packet.
+	grep -E "rate:4294967295,group:1,cookie:c0ffee data:[0-9a-f]{28}$" \
+			 $ovs_dir/stdout >/dev/null 2>&1 || return 1
+	grep -E "rate:4294967295,group:2,cookie:eeff0c" \
+			 $ovs_dir/stdout >/dev/null 2>&1 || return 1
+
+	return 0
+}
+
 # drop_reason test
 # - drop packets and verify the right drop reason is reported
 test_drop_reason() {
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index e8dc9af10d4d..1e15b0818074 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -28,8 +28,10 @@ try:
     from pyroute2.netlink import genlmsg
     from pyroute2.netlink import nla
     from pyroute2.netlink import nlmsg_atoms
+    from pyroute2.netlink.event import EventSocket
     from pyroute2.netlink.exceptions import NetlinkError
     from pyroute2.netlink.generic import GenericNetlinkSocket
+    from pyroute2.netlink.nlsocket import Marshal
     import pyroute2
     import pyroute2.iproute
 
@@ -2460,10 +2462,70 @@ class OvsFlow(GenericNetlinkSocket):
         print("MISS upcall[%d/%s]: %s" % (seq, pktpres, keystr), flush=True)
 
     def execute(self, packetmsg):
-        print("userspace execute command")
+        print("userspace execute command", flush=True)
 
     def action(self, packetmsg):
-        print("userspace action command")
+        print("userspace action command", flush=True)
+
+
+class psample_sample(genlmsg):
+    nla_map = (
+        ("PSAMPLE_ATTR_IIFINDEX", "none"),
+        ("PSAMPLE_ATTR_OIFINDEX", "none"),
+        ("PSAMPLE_ATTR_ORIGSIZE", "none"),
+        ("PSAMPLE_ATTR_SAMPLE_GROUP", "uint32"),
+        ("PSAMPLE_ATTR_GROUP_SEQ", "none"),
+        ("PSAMPLE_ATTR_SAMPLE_RATE", "uint32"),
+        ("PSAMPLE_ATTR_DATA", "array(uint8)"),
+        ("PSAMPLE_ATTR_GROUP_REFCOUNT", "none"),
+        ("PSAMPLE_ATTR_TUNNEL", "none"),
+        ("PSAMPLE_ATTR_PAD", "none"),
+        ("PSAMPLE_ATTR_OUT_TC", "none"),
+        ("PSAMPLE_ATTR_OUT_TC_OCC", "none"),
+        ("PSAMPLE_ATTR_LATENCY", "none"),
+        ("PSAMPLE_ATTR_TIMESTAMP", "none"),
+        ("PSAMPLE_ATTR_PROTO", "none"),
+        ("PSAMPLE_ATTR_USER_COOKIE", "array(uint8)"),
+    )
+
+    def dpstr(self):
+        fields = []
+        data = ""
+        for (attr, value) in self["attrs"]:
+            if attr == "PSAMPLE_ATTR_SAMPLE_GROUP":
+                fields.append("group:%d" % value)
+            if attr == "PSAMPLE_ATTR_SAMPLE_RATE":
+                fields.append("rate:%d" % value)
+            if attr == "PSAMPLE_ATTR_USER_COOKIE":
+                value = "".join(format(x, "02x") for x in value)
+                fields.append("cookie:%s" % value)
+            if attr == "PSAMPLE_ATTR_DATA" and len(value) > 0:
+                data = "data:%s" % "".join(format(x, "02x") for x in value)
+
+        return ("%s %s" % (",".join(fields), data)).strip()
+
+
+class psample_msg(Marshal):
+    PSAMPLE_CMD_SAMPLE = 0
+    PSAMPLE_CMD_GET_GROUP = 1
+    PSAMPLE_CMD_NEW_GROUP = 2
+    PSAMPLE_CMD_DEL_GROUP = 3
+    PSAMPLE_CMD_SET_FILTER = 4
+    msg_map = {PSAMPLE_CMD_SAMPLE: psample_sample}
+
+
+class PsampleEvent(EventSocket):
+    genl_family = "psample"
+    mcast_groups = ["packets"]
+    marshal_class = psample_msg
+
+    def read_samples(self):
+        while True:
+            try:
+                for msg in self.get():
+                    print(msg.dpstr(), flush=True)
+            except NetlinkError as ne:
+                raise ne
 
 
 def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
@@ -2530,7 +2592,7 @@ def main(argv):
         help="Increment 'verbose' output counter.",
         default=0,
     )
-    subparsers = parser.add_subparsers()
+    subparsers = parser.add_subparsers(dest="subcommand")
 
     showdpcmd = subparsers.add_parser("show")
     showdpcmd.add_argument(
@@ -2605,6 +2667,8 @@ def main(argv):
     delfscmd = subparsers.add_parser("del-flows")
     delfscmd.add_argument("flsbr", help="Datapath name")
 
+    subparsers.add_parser("psample-events")
+
     args = parser.parse_args()
 
     if args.verbose > 0:
@@ -2619,6 +2683,9 @@ def main(argv):
 
     sys.setrecursionlimit(100000)
 
+    if args.subcommand == "psample-events":
+        PsampleEvent().read_samples()
+
     if hasattr(args, "showdp"):
         found = False
         for iface in ndb.interfaces:
-- 
2.45.2

Re: [PATCH net-next v7 01/10] net: psample: add user cookie

[Michal Kubiak]

On Sun, Jun 30, 2024 at 09:57:22PM +0200, Adrian Moreno wrote:
> Add a user cookie to the sample metadata so that sample emitters can
> provide more contextual information to samples.
> 
> If present, send the user cookie in a new attribute:
> PSAMPLE_ATTR_USER_COOKIE.
> 
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
> 

Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Michal Kubiak]

On Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote:
> Add support for a new action: psample.
> 
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
> 
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.
> 
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
>  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
>  net/openvswitch/Kconfig                   |  1 +
>  net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
>  net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
>  5 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -727,6 +727,12 @@ attribute-sets:
>          name: dec-ttl
>          type: nest
>          nested-attributes: dec-ttl-attrs
> +      -
> +        name: psample
> +        type: nest
> +        nested-attributes: psample-attrs
> +        doc: |
> +          Sends a packet sample to psample for external observation.
>    -
>      name: tunnel-key-attrs
>      enum-name: ovs-tunnel-key-attr
> @@ -938,6 +944,17 @@ attribute-sets:
>        -
>          name: gbp
>          type: u32
> +  -
> +    name: psample-attrs
> +    enum-name: ovs-psample-attr
> +    name-prefix: ovs-psample-attr-
> +    attributes:
> +      -
> +        name: group
> +        type: u32
> +      -
> +        name: cookie
> +        type: binary
>  
>  operations:
>    name-prefix: ovs-flow-cmd-
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index efc82c318fa2..3dd653748725 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>  };
>  #endif
>  
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16

In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your
cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE,
so this size will be validated correctly.
But how likely is that those 2 constants will have different values in the
future?
Would it be reasonable to create more strict dependency between those
macros, e.g.:

#define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE

or, at least, add a comment that the size shouldn't be bigger than
TC_COOKIE_MAX_SIZE?
I'm just considering the risk of exceeding the array from the patch #2 when
somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future.

Thanks,
Michal

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Adrián Moreno]

On Mon, Jul 01, 2024 at 01:40:39PM GMT, Michal Kubiak wrote:
> On Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote:
> > Add support for a new action: psample.
> >
> > This action accepts a u32 group id and a variable-length cookie and uses
> > the psample multicast group to make the packet available for
> > observability.
> >
> > The maximum length of the user-defined cookie is set to 16, same as
> > tc_cookie, to discourage using cookies that will not be offloadable.
> >
> > Acked-by: Eelco Chaudron <echaudro@redhat.com>
> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > ---
> >  Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
> >  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
> >  net/openvswitch/Kconfig                   |  1 +
> >  net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
> >  net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
> >  5 files changed, 124 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> > index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> > --- a/Documentation/netlink/specs/ovs_flow.yaml
> > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> > @@ -727,6 +727,12 @@ attribute-sets:
> >          name: dec-ttl
> >          type: nest
> >          nested-attributes: dec-ttl-attrs
> > +      -
> > +        name: psample
> > +        type: nest
> > +        nested-attributes: psample-attrs
> > +        doc: |
> > +          Sends a packet sample to psample for external observation.
> >    -
> >      name: tunnel-key-attrs
> >      enum-name: ovs-tunnel-key-attr
> > @@ -938,6 +944,17 @@ attribute-sets:
> >        -
> >          name: gbp
> >          type: u32
> > +  -
> > +    name: psample-attrs
> > +    enum-name: ovs-psample-attr
> > +    name-prefix: ovs-psample-attr-
> > +    attributes:
> > +      -
> > +        name: group
> > +        type: u32
> > +      -
> > +        name: cookie
> > +        type: binary
> >
> >  operations:
> >    name-prefix: ovs-flow-cmd-
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index efc82c318fa2..3dd653748725 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> >  };
> >  #endif
> >
> > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
>
> In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your
> cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE,
> so this size will be validated correctly.
> But how likely is that those 2 constants will have different values in the
> future?
> Would it be reasonable to create more strict dependency between those
> macros, e.g.:
>
> #define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE
>
> or, at least, add a comment that the size shouldn't be bigger than
> TC_COOKIE_MAX_SIZE?
> I'm just considering the risk of exceeding the array from the patch #2 when
> somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future.
>
> Thanks,
> Michal
>

Hi Michal,

Thanks for sharing your thoughts.

I tried to keep the dependency between both cookie sizes loose.

I don't want a change in TC_COOKIE_MAX_SIZE to inadvertently modify
OVS_PSAMPLE_COOKIE_MAX_SIZE because OVS might not need a bigger cookie
and even if it does, backwards compatibility needs to be guaranteed:
meaning OVS userspace will have to detect the new size and use it or
fall back to a smaller cookie for older kernels. All this needs to be
known and worked on in userspace.

On the other hand, I intentionally made OVS's "psample" action as
similar as possible to act_psample, including setting the same cookie
size to begin with. The reason is that I think we should try to implement
tc-flower offloading of this action using act_sample, plus 16 seemed a
very reasonable max value.

When we decide to support offloading the "psample" action, this must
be done entirely in userspace. OVS must create a act_sample action
(instead of the OVS "psample" one) via netlink. In no circumstances the
openvswitch kmod interacts with tc directly.

Now, back to your concern. If OVS_PSAMPLE_COOKIE_MAX_SIZE grows and
TC_COOKIE_MAX_SIZE does not *and* we already support offloading this
action to tc, the only consequence is that OVS userspace has a
problem because the tc's netlink interface will reject cookies larger
than TC_COOKIE_MAX_SIZE [1].
This guarantees that the array in patch #2 is never overflown.

OVS will have to deal with the different sizes and try to squeeze the
data into TC_COOKIE_MAX_SIZE or fail to offload the action altogether.

Psample does not have a size limit so different parts of the kernel can
use psample with different internal max-sizes without any restriction.

I hope this clears your concerns.

[1] https://github.com/torvalds/linux/blob/master/net/sched/act_api.c#L1299

Thanks.
Adrián

Re: [PATCH net-next v7 01/10] net: psample: add user cookie

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> Add a user cookie to the sample metadata so that sample emitters can
> provide more contextual information to samples.
>
> If present, send the user cookie in a new attribute:
> PSAMPLE_ATTR_USER_COOKIE.
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

>  include/net/psample.h        | 2 ++
>  include/uapi/linux/psample.h | 1 +
>  net/psample/psample.c        | 9 ++++++++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/psample.h b/include/net/psample.h
> index 0509d2d6be67..2ac71260a546 100644
> --- a/include/net/psample.h
> +++ b/include/net/psample.h
> @@ -25,6 +25,8 @@ struct psample_metadata {
>  	   out_tc_occ_valid:1,
>  	   latency_valid:1,
>  	   unused:5;
> +	const u8 *user_cookie;
> +	u32 user_cookie_len;
>  };
>  
>  struct psample_group *psample_group_get(struct net *net, u32 group_num);
> diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
> index e585db5bf2d2..e80637e1d97b 100644
> --- a/include/uapi/linux/psample.h
> +++ b/include/uapi/linux/psample.h
> @@ -19,6 +19,7 @@ enum {
>  	PSAMPLE_ATTR_LATENCY,		/* u64, nanoseconds */
>  	PSAMPLE_ATTR_TIMESTAMP,		/* u64, nanoseconds */
>  	PSAMPLE_ATTR_PROTO,		/* u16 */
> +	PSAMPLE_ATTR_USER_COOKIE,	/* binary, user provided data */
>  
>  	__PSAMPLE_ATTR_MAX
>  };
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index a5d9b8446f77..b37488f426bc 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -386,7 +386,9 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
>  		   nla_total_size(sizeof(u32)) +	/* group_num */
>  		   nla_total_size(sizeof(u32)) +	/* seq */
>  		   nla_total_size_64bit(sizeof(u64)) +	/* timestamp */
> -		   nla_total_size(sizeof(u16));		/* protocol */
> +		   nla_total_size(sizeof(u16)) +	/* protocol */
> +		   (md->user_cookie_len ?
> +		    nla_total_size(md->user_cookie_len) : 0); /* user cookie */
>  
>  #ifdef CONFIG_INET
>  	tun_info = skb_tunnel_info(skb);
> @@ -486,6 +488,11 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
>  	}
>  #endif
>  
> +	if (md->user_cookie && md->user_cookie_len &&
> +	    nla_put(nl_skb, PSAMPLE_ATTR_USER_COOKIE, md->user_cookie_len,
> +		    md->user_cookie))
> +		goto error;
> +
>  	genlmsg_end(nl_skb, data);
>  	genlmsg_multicast_netns(&psample_nl_family, group->net, nl_skb, 0,
>  				PSAMPLE_NL_MCGRP_SAMPLE, GFP_ATOMIC);

Re: [PATCH net-next v7 02/10] net: sched: act_sample: add action cookie to sample

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> If the action has a user_cookie, pass it along to the sample so it can
> be easily identified.
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

>  net/sched/act_sample.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> index a69b53d54039..2ceb4d141b71 100644
> --- a/net/sched/act_sample.c
> +++ b/net/sched/act_sample.c
> @@ -167,7 +167,9 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
>  {
>  	struct tcf_sample *s = to_sample(a);
>  	struct psample_group *psample_group;
> +	u8 cookie_data[TC_COOKIE_MAX_SIZE];
>  	struct psample_metadata md = {};
> +	struct tc_cookie *user_cookie;
>  	int retval;
>  
>  	tcf_lastuse_update(&s->tcf_tm);
> @@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
>  		if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
>  			skb_push(skb, skb->mac_len);
>  
> +		rcu_read_lock();
> +		user_cookie = rcu_dereference(a->user_cookie);
> +		if (user_cookie) {
> +			memcpy(cookie_data, user_cookie->data,
> +			       user_cookie->len);
> +			md.user_cookie = cookie_data;
> +			md.user_cookie_len = user_cookie->len;
> +		}
> +		rcu_read_unlock();
> +
>  		md.trunc_size = s->truncate ? s->trunc_size : skb->len;
>  		psample_sample_packet(psample_group, skb, s->rate, &md);

Re: [PATCH net-next v7 03/10] net: psample: skip packet copy if no listeners

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> If nobody is listening on the multicast group, generating the sample,
> which involves copying packet data, seems completely unnecessary.
>
> Return fast in this case.
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

>  net/psample/psample.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index b37488f426bc..1c76f3e48dcd 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -376,6 +376,10 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
>  	void *data;
>  	int ret;
>  
> +	if (!genl_has_listeners(&psample_nl_family, group->net,
> +				PSAMPLE_NL_MCGRP_SAMPLE))
> +		return;
> +
>  	meta_len = (in_ifindex ? nla_total_size(sizeof(u16)) : 0) +
>  		   (out_ifindex ? nla_total_size(sizeof(u16)) : 0) +
>  		   (md->out_tc_valid ? nla_total_size(sizeof(u16)) : 0) +

Re: [PATCH net-next v7 04/10] net: psample: allow using rate as probability

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> Although not explicitly documented in the psample module itself, the
> definition of PSAMPLE_ATTR_SAMPLE_RATE seems inherited from act_sample.
>
> Quoting tc-sample(8):
> "RATE of 100 will lead to an average of one sampled packet out of every
> 100 observed."
>
> With this semantics, the rates that we can express with an unsigned
> 32-bits number are very unevenly distributed and concentrated towards
> "sampling few packets".
> For example, we can express a probability of 2.32E-8% but we
> cannot express anything between 100% and 50%.
>
> For sampling applications that are capable of sampling a decent
> amount of packets, this sampling rate semantics is not very useful.
>
> Add a new flag to the uAPI that indicates that the sampling rate is
> expressed in scaled probability, this is:
> - 0 is 0% probability, no packets get sampled.
> - U32_MAX is 100% probability, all packets get sampled.
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

>  include/net/psample.h        |  3 ++-
>  include/uapi/linux/psample.h | 10 +++++++++-
>  net/psample/psample.c        |  3 +++
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/psample.h b/include/net/psample.h
> index 2ac71260a546..c52e9ebd88dd 100644
> --- a/include/net/psample.h
> +++ b/include/net/psample.h
> @@ -24,7 +24,8 @@ struct psample_metadata {
>  	u8 out_tc_valid:1,
>  	   out_tc_occ_valid:1,
>  	   latency_valid:1,
> -	   unused:5;
> +	   rate_as_probability:1,
> +	   unused:4;
>  	const u8 *user_cookie;
>  	u32 user_cookie_len;
>  };
> diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
> index e80637e1d97b..b765f0e81f20 100644
> --- a/include/uapi/linux/psample.h
> +++ b/include/uapi/linux/psample.h
> @@ -8,7 +8,11 @@ enum {
>  	PSAMPLE_ATTR_ORIGSIZE,
>  	PSAMPLE_ATTR_SAMPLE_GROUP,
>  	PSAMPLE_ATTR_GROUP_SEQ,
> -	PSAMPLE_ATTR_SAMPLE_RATE,
> +	PSAMPLE_ATTR_SAMPLE_RATE,	/* u32, ratio between observed and
> +					 * sampled packets or scaled probability
> +					 * if PSAMPLE_ATTR_SAMPLE_PROBABILITY
> +					 * is set.
> +					 */
>  	PSAMPLE_ATTR_DATA,
>  	PSAMPLE_ATTR_GROUP_REFCOUNT,
>  	PSAMPLE_ATTR_TUNNEL,
> @@ -20,6 +24,10 @@ enum {
>  	PSAMPLE_ATTR_TIMESTAMP,		/* u64, nanoseconds */
>  	PSAMPLE_ATTR_PROTO,		/* u16 */
>  	PSAMPLE_ATTR_USER_COOKIE,	/* binary, user provided data */
> +	PSAMPLE_ATTR_SAMPLE_PROBABILITY,/* no argument, interpret rate in
> +					 * PSAMPLE_ATTR_SAMPLE_RATE as a
> +					 * probability scaled 0 - U32_MAX.
> +					 */
>  
>  	__PSAMPLE_ATTR_MAX
>  };
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index 1c76f3e48dcd..f48b5b9cd409 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -497,6 +497,9 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
>  		    md->user_cookie))
>  		goto error;
>  
> +	if (md->rate_as_probability)
> +		nla_put_flag(skb, PSAMPLE_ATTR_SAMPLE_PROBABILITY);
> +
>  	genlmsg_end(nl_skb, data);
>  	genlmsg_multicast_netns(&psample_nl_family, group->net, nl_skb, 0,
>  				PSAMPLE_NL_MCGRP_SAMPLE, GFP_ATOMIC);

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Ilya Maximets]

On 6/30/24 21:57, Adrian Moreno wrote:
> Add support for a new action: psample.
> 
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
> 
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.
> 
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
>  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
>  net/openvswitch/Kconfig                   |  1 +
>  net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
>  net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
>  5 files changed, 124 insertions(+), 1 deletion(-)

Thanks for addressing the comments!  The new name also
seems reasonable to me.

Reviewed-by: Ilya Maximets <i.maximets@ovn.org>

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> Add support for a new action: psample.
>
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
>
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Hi Adrian,

Just some nits below.

>  Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
>  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
>  net/openvswitch/Kconfig                   |  1 +
>  net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
>  net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
>  5 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -727,6 +727,12 @@ attribute-sets:
>          name: dec-ttl
>          type: nest
>          nested-attributes: dec-ttl-attrs
> +      -
> +        name: psample
> +        type: nest
> +        nested-attributes: psample-attrs
> +        doc: |
> +          Sends a packet sample to psample for external observation.
>    -
>      name: tunnel-key-attrs
>      enum-name: ovs-tunnel-key-attr
> @@ -938,6 +944,17 @@ attribute-sets:
>        -
>          name: gbp
>          type: u32
> +  -
> +    name: psample-attrs
> +    enum-name: ovs-psample-attr
> +    name-prefix: ovs-psample-attr-
> +    attributes:
> +      -
> +        name: group
> +        type: u32
> +      -
> +        name: cookie
> +        type: binary
>  
>  operations:
>    name-prefix: ovs-flow-cmd-
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index efc82c318fa2..3dd653748725 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>  };
>  #endif
>  
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> + * action.
> + *
> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
> + * contains user-defined metadata. The maximum length is
> + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> + *
> + * Sends the packet to the psample multicast group with the specified group and
> + * cookie. It is possible to combine this action with the
> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> + */
> +enum ovs_psample_attr {
> +	OVS_PSAMPLE_ATTR_GROUP = 1,	/* u32 number. */
> +	OVS_PSAMPLE_ATTR_COOKIE,	/* Optional, user specified cookie. */
> +
> +	/* private: */
> +	__OVS_PSAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> +
>  /**
>   * enum ovs_action_attr - Action types.
>   *
> @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
>   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>   * argument.
>   * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external observers
> + * via psample.
>   *
>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
>   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
>  	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
>  	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
>  	OVS_ACTION_ATTR_DROP,         /* u32 error code. */
> +	OVS_ACTION_ATTR_PSAMPLE,      /* Nested OVS_PSAMPLE_ATTR_*. */
>  
>  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
>  				       * from userspace. */
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 29a7081858cd..2535f3f9f462 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -10,6 +10,7 @@ config OPENVSWITCH
>  		   (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>  				     (!NF_NAT || NF_NAT) && \
>  				     (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT)))
> +	depends on PSAMPLE || !PSAMPLE
>  	select LIBCRC32C
>  	select MPLS
>  	select NET_MPLS_GSO
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 964225580824..a035b7e677dd 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -24,6 +24,11 @@
>  #include <net/checksum.h>
>  #include <net/dsfield.h>
>  #include <net/mpls.h>
> +
> +#if IS_ENABLED(CONFIG_PSAMPLE)
> +#include <net/psample.h>
> +#endif
> +
>  #include <net/sctp/checksum.h>
>  
>  #include "datapath.h"
> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_PSAMPLE)
> +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> +			    const struct nlattr *attr)
> +{
> +	struct psample_group psample_group = {};
> +	struct psample_metadata md = {};
> +	const struct nlattr *a;
> +	int rem;
> +
> +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> +		switch (nla_type(a)) {
> +		case OVS_PSAMPLE_ATTR_GROUP:
> +			psample_group.group_num = nla_get_u32(a);
> +			break;
> +
> +		case OVS_PSAMPLE_ATTR_COOKIE:
> +			md.user_cookie = nla_data(a);
> +			md.user_cookie_len = nla_len(a);
> +			break;
> +		}
> +	}
> +
> +	psample_group.net = ovs_dp_get_net(dp);
> +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> +
> +	psample_sample_packet(&psample_group, skb, 0, &md);
> +}
> +#else
> +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> +				   const struct nlattr *attr) {}

I noticed that this got flagged in patchwork since it is 'static inline'
while being part of a complete translation unit - but I also see some
other places where that has been done.  I guess it should be just
'static' though.  I don't feel very strongly about it.

> +#endif
> +
>  /* Execute a list of actions against 'skb'. */
>  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  			      struct sw_flow_key *key,
> @@ -1502,6 +1540,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>  			ovs_kfree_skb_reason(skb, reason);
>  			return 0;
>  		}
> +
> +		case OVS_ACTION_ATTR_PSAMPLE:
> +			execute_psample(dp, skb, a);
> +			OVS_CB(skb)->cutlen = 0;

We may want to document that trunc is also impacted by psample calls.
Right now, it is only mentioned for a single OUTPUT action.
Alternatively, we could either ignore trunc, or not reset here.

> +			if (nla_is_last(a, rem)) {
> +				consume_skb(skb);
> +				return 0;
> +			}
> +			break;
>  		}
>  
>  		if (unlikely(err)) {
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index f224d9bcea5e..c92bdc4dfe19 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
>  		case OVS_ACTION_ATTR_TRUNC:
>  		case OVS_ACTION_ATTR_USERSPACE:
>  		case OVS_ACTION_ATTR_DROP:
> +		case OVS_ACTION_ATTR_PSAMPLE:
>  			break;
>  
>  		case OVS_ACTION_ATTR_CT:
> @@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
>  	/* Whenever new actions are added, the need to update this
>  	 * function should be considered.
>  	 */
> -	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
> +	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25);
>  
>  	if (!actions)
>  		return;
> @@ -3157,6 +3158,28 @@ static int validate_and_copy_check_pkt_len(struct net *net,
>  	return 0;
>  }
>  
> +static int validate_psample(const struct nlattr *attr)
> +{
> +	static const struct nla_policy policy[OVS_PSAMPLE_ATTR_MAX + 1] = {
> +		[OVS_PSAMPLE_ATTR_GROUP] = { .type = NLA_U32 },
> +		[OVS_PSAMPLE_ATTR_COOKIE] = {
> +			.type = NLA_BINARY,
> +			.len = OVS_PSAMPLE_COOKIE_MAX_SIZE,
> +		},
> +	};
> +	struct nlattr *a[OVS_PSAMPLE_ATTR_MAX + 1];
> +	int err;
> +
> +	if (!IS_ENABLED(CONFIG_PSAMPLE))
> +		return -EOPNOTSUPP;
> +
> +	err = nla_parse_nested(a, OVS_PSAMPLE_ATTR_MAX, attr, policy, NULL);
> +	if (err)
> +		return err;
> +
> +	return a[OVS_PSAMPLE_ATTR_GROUP] ? 0 : -EINVAL;
> +}
> +
>  static int copy_action(const struct nlattr *from,
>  		       struct sw_flow_actions **sfa, bool log)
>  {
> @@ -3212,6 +3235,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
>  			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
>  			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
> +			[OVS_ACTION_ATTR_PSAMPLE] = (u32)-1,
>  		};
>  		const struct ovs_action_push_vlan *vlan;
>  		int type = nla_type(a);
> @@ -3490,6 +3514,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>  				return -EINVAL;
>  			break;
>  
> +		case OVS_ACTION_ATTR_PSAMPLE:
> +			err = validate_psample(a);
> +			if (err)
> +				return err;
> +			break;
> +
>  		default:
>  			OVS_NLERR(log, "Unknown Action type %d", type);
>  			return -EINVAL;

Re: [PATCH net-next v7 06/10] net: openvswitch: store sampling probability in cb.

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> When a packet sample is observed, the sampling rate that was used is
> important to estimate the real frequency of such event.
>
> Store the probability of the parent sample action in the skb's cb area
> and use it in psample action to pass it down to psample module.
>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

>  include/uapi/linux/openvswitch.h |  3 ++-
>  net/openvswitch/actions.c        | 20 +++++++++++++++++---
>  net/openvswitch/datapath.h       |  3 +++
>  net/openvswitch/vport.c          |  1 +
>  4 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index 3dd653748725..3a701bd1f31b 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -649,7 +649,8 @@ enum ovs_flow_attr {
>   * Actions are passed as nested attributes.
>   *
>   * Executes the specified actions with the given probability on a per-packet
> - * basis.
> + * basis. Nested actions will be able to access the probability value of the
> + * parent @OVS_ACTION_ATTR_SAMPLE.
>   */
>  enum ovs_sample_attr {
>  	OVS_SAMPLE_ATTR_UNSPEC,
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index a035b7e677dd..34af6bce4085 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -1048,12 +1048,15 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
>  	struct nlattr *sample_arg;
>  	int rem = nla_len(attr);
>  	const struct sample_arg *arg;
> +	u32 init_probability;
>  	bool clone_flow_key;
> +	int err;
>  
>  	/* The first action is always 'OVS_SAMPLE_ATTR_ARG'. */
>  	sample_arg = nla_data(attr);
>  	arg = nla_data(sample_arg);
>  	actions = nla_next(sample_arg, &rem);
> +	init_probability = OVS_CB(skb)->probability;
>  
>  	if ((arg->probability != U32_MAX) &&
>  	    (!arg->probability || get_random_u32() > arg->probability)) {
> @@ -1062,9 +1065,16 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
>  		return 0;
>  	}
>  
> +	OVS_CB(skb)->probability = arg->probability;
> +
>  	clone_flow_key = !arg->exec;
> -	return clone_execute(dp, skb, key, 0, actions, rem, last,
> -			     clone_flow_key);
> +	err = clone_execute(dp, skb, key, 0, actions, rem, last,
> +			    clone_flow_key);
> +
> +	if (!last)
> +		OVS_CB(skb)->probability = init_probability;
> +
> +	return err;
>  }
>  
>  /* When 'last' is true, clone() should always consume the 'skb'.
> @@ -1311,6 +1321,7 @@ static void execute_psample(struct datapath *dp, struct sk_buff *skb,
>  	struct psample_group psample_group = {};
>  	struct psample_metadata md = {};
>  	const struct nlattr *a;
> +	u32 rate;
>  	int rem;
>  
>  	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> @@ -1329,8 +1340,11 @@ static void execute_psample(struct datapath *dp, struct sk_buff *skb,
>  	psample_group.net = ovs_dp_get_net(dp);
>  	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
>  	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> +	md.rate_as_probability = 1;
> +
> +	rate = OVS_CB(skb)->probability ? OVS_CB(skb)->probability : U32_MAX;
>  
> -	psample_sample_packet(&psample_group, skb, 0, &md);
> +	psample_sample_packet(&psample_group, skb, rate, &md);
>  }
>  #else
>  static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
> index 0cd29971a907..9ca6231ea647 100644
> --- a/net/openvswitch/datapath.h
> +++ b/net/openvswitch/datapath.h
> @@ -115,12 +115,15 @@ struct datapath {
>   * fragmented.
>   * @acts_origlen: The netlink size of the flow actions applied to this skb.
>   * @cutlen: The number of bytes from the packet end to be removed.
> + * @probability: The sampling probability that was applied to this skb; 0 means
> + * no sampling has occurred; U32_MAX means 100% probability.
>   */
>  struct ovs_skb_cb {
>  	struct vport		*input_vport;
>  	u16			mru;
>  	u16			acts_origlen;
>  	u32			cutlen;
> +	u32			probability;
>  };
>  #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
>  
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index 972ae01a70f7..8732f6e51ae5 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -500,6 +500,7 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
>  	OVS_CB(skb)->input_vport = vport;
>  	OVS_CB(skb)->mru = 0;
>  	OVS_CB(skb)->cutlen = 0;
> +	OVS_CB(skb)->probability = 0;
>  	if (unlikely(dev_net(skb->dev) != ovs_dp_get_net(vport->dp))) {
>  		u32 mark;

Re: [PATCH net-next v7 10/10] selftests: openvswitch: add psample test

[Ilya Maximets]

On 6/30/24 21:57, Adrian Moreno wrote:
> Add a test to verify sampling packets via psample works.
> 
> In order to do that, create a subcommand in ovs-dpctl.py to listen to
> on the psample multicast group and print samples.
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  .../selftests/net/openvswitch/openvswitch.sh  | 115 +++++++++++++++++-
>  .../selftests/net/openvswitch/ovs-dpctl.py    |  73 ++++++++++-
>  2 files changed, 182 insertions(+), 6 deletions(-)


This version seems to work correctly with and without arguments.

Tested-by: Ilya Maximets <i.maximets@ovn.org>

Re: [PATCH net-next v7 10/10] selftests: openvswitch: add psample test

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> Add a test to verify sampling packets via psample works.
>
> In order to do that, create a subcommand in ovs-dpctl.py to listen to
> on the psample multicast group and print samples.
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  .../selftests/net/openvswitch/openvswitch.sh  | 115 +++++++++++++++++-
>  .../selftests/net/openvswitch/ovs-dpctl.py    |  73 ++++++++++-
>  2 files changed, 182 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index 15bca0708717..02a366e01004 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -20,7 +20,8 @@ tests="
>  	nat_related_v4				ip4-nat-related: ICMP related matches work with SNAT
>  	netlink_checks				ovsnl: validate netlink attrs and settings
>  	upcall_interfaces			ovs: test the upcall interfaces
> -	drop_reason				drop: test drop reasons are emitted"
> +	drop_reason				drop: test drop reasons are emitted
> +	psample					psample: Sampling packets with psample"
>  
>  info() {
>      [ $VERBOSE = 0 ] || echo $*
> @@ -102,12 +103,21 @@ ovs_netns_spawn_daemon() {
>  	shift
>  	netns=$1
>  	shift
> -	info "spawning cmd: $*"
> -	ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
> +	if [ "$netns" == "_default" ]; then
> +		$*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
> +	else
> +		ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
> +	fi
>  	pid=$!
>  	ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
>  }
>  
> +ovs_spawn_daemon() {
> +	sbx=$1
> +	shift
> +	ovs_netns_spawn_daemon $sbx "_default" $*
> +}
> +
>  ovs_add_netns_and_veths () {
>  	info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
>  	ovs_sbx "$1" ip netns add "$3" || return 1
> @@ -170,6 +180,19 @@ ovs_drop_reason_count()
>  	return `echo "$perf_output" | grep "$pattern" | wc -l`
>  }
>  
> +ovs_test_flow_fails () {
> +	ERR_MSG="Flow actions may not be safe on all matching packets"
> +
> +	PRE_TEST=$(dmesg | grep -c "${ERR_MSG}")
> +	ovs_add_flow $@ &> /dev/null $@ && return 1
> +	POST_TEST=$(dmesg | grep -c "${ERR_MSG}")
> +
> +	if [ "$PRE_TEST" == "$POST_TEST" ]; then
> +		return 1
> +	fi
> +	return 0
> +}
> +
>  usage() {
>  	echo
>  	echo "$0 [OPTIONS] [TEST]..."
> @@ -184,6 +207,92 @@ usage() {
>  	exit 1
>  }
>  
> +
> +# psample test
> +# - use psample to observe packets
> +test_psample() {
> +	sbx_add "test_psample" || return $?
> +
> +	# Add a datapath with per-vport dispatching.
> +	ovs_add_dp "test_psample" psample -V 2:1 || return 1
> +
> +	info "create namespaces"
> +	ovs_add_netns_and_veths "test_psample" "psample" \
> +		client c0 c1 172.31.110.10/24 -u || return 1
> +	ovs_add_netns_and_veths "test_psample" "psample" \
> +		server s0 s1 172.31.110.20/24 -u || return 1
> +
> +	# Check if psample actions can be configured.
> +	ovs_add_flow "test_psample" psample \
> +	'in_port(1),eth(),eth_type(0x0806),arp()' 'psample(group=1)'

Might be good to redirect this stdout/stderr line to /dev/null -
otherwise on an unsupported system there will be the following extra
splat:

  Traceback (most recent call last):
    File "/home/aconole/git/linux/tools/testing/selftests/net/openvswitch/ovs-dpctl.py", line 2774, in <module>
      sys.exit(main(sys.argv))
   ...
    File "/usr/lib/python3.12/site-packages/pyroute2/netlink/nlsocket.py", line 489, in get
      raise msg['header']['error']
  pyroute2.netlink.exceptions.NetlinkError: (22, 'Invalid argument')

> +	if [ $? == 1 ]; then
> +		info "no support for psample - skipping"
> +		ovs_exit_sig
> +		return $ksft_skip
> +	fi
> +
> +	ovs_del_flows "test_psample" psample
> +
> +	# Test action verification.
> +	OLDIFS=$IFS
> +	IFS='*'
> +	min_key='in_port(1),eth(),eth_type(0x0800),ipv4()'
> +	for testcase in \
> +		"cookie to large"*"psample(group=1,cookie=1615141312111009080706050403020100)" \
> +		"no group with cookie"*"psample(cookie=abcd)" \
> +		"no group"*"psample()";
> +	do
> +		set -- $testcase;
> +		ovs_test_flow_fails "test_psample" psample $min_key $2
> +		if [ $? == 1 ]; then
> +			info "failed - $1"
> +			return 1
> +		fi
> +	done
> +	IFS=$OLDIFS
> +
> +	ovs_del_flows "test_psample" psample
> +	# Allow ARP
> +	ovs_add_flow "test_psample" psample \
> +		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
> +	ovs_add_flow "test_psample" psample \
> +		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
> +
> +	# Sample first 14 bytes of all traffic.
> +	ovs_add_flow "test_psample" psample \
> +	    "in_port(1),eth(),eth_type(0x0800),ipv4()" \
> +            "trunc(14),psample(group=1,cookie=c0ffee),2"
> +
> +	# Sample all traffic. In this case, use a sample() action with both
> +	# psample and an upcall emulating simultaneous local sampling and
> +	# sFlow / IPFIX.
> +	nlpid=$(grep -E "listening on upcall packet handler" \
> +            $ovs_dir/s0.out | cut -d ":" -f 2 | tr -d ' ')
> +
> +	ovs_add_flow "test_psample" psample \
> +            "in_port(2),eth(),eth_type(0x0800),ipv4()" \
> +            "sample(sample=100%,actions(psample(group=2,cookie=eeff0c),userspace(pid=${nlpid},userdata=eeff0c))),1"
> +
> +	# Record psample data.
> +	ovs_spawn_daemon "test_psample" python3 $ovs_base/ovs-dpctl.py psample-events
> +
> +	# Send a single ping.
> +	sleep 1
> +	ovs_sbx "test_psample" ip netns exec client ping -I c1 172.31.110.20 -c 1 || return 1
> +	sleep 1
> +
> +	# We should have received one userspace action upcall and 2 psample packets.
> +	grep -E "userspace action command" $ovs_dir/s0.out >/dev/null 2>&1 || return 1

I wonder if it would be better to check a few times instead of the one
shot sleep.  There are some constrained environments that may run this
test, and if you're worried about some kinds of races, maybe it makes
sense to check a few times?

Outside of that:

Reviewed-by: Aaron Conole <aconole@redhat.com>

> +
> +	# client -> server samples should only contain the first 14 bytes of the packet.
> +	grep -E "rate:4294967295,group:1,cookie:c0ffee data:[0-9a-f]{28}$" \
> +			 $ovs_dir/stdout >/dev/null 2>&1 || return 1
> +	grep -E "rate:4294967295,group:2,cookie:eeff0c" \
> +			 $ovs_dir/stdout >/dev/null 2>&1 || return 1
> +
> +	return 0
> +}
> +
>  # drop_reason test
>  # - drop packets and verify the right drop reason is reported
>  test_drop_reason() {
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index e8dc9af10d4d..1e15b0818074 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -28,8 +28,10 @@ try:
>      from pyroute2.netlink import genlmsg
>      from pyroute2.netlink import nla
>      from pyroute2.netlink import nlmsg_atoms
> +    from pyroute2.netlink.event import EventSocket
>      from pyroute2.netlink.exceptions import NetlinkError
>      from pyroute2.netlink.generic import GenericNetlinkSocket
> +    from pyroute2.netlink.nlsocket import Marshal
>      import pyroute2
>      import pyroute2.iproute
>  
> @@ -2460,10 +2462,70 @@ class OvsFlow(GenericNetlinkSocket):
>          print("MISS upcall[%d/%s]: %s" % (seq, pktpres, keystr), flush=True)
>  
>      def execute(self, packetmsg):
> -        print("userspace execute command")
> +        print("userspace execute command", flush=True)
>  
>      def action(self, packetmsg):
> -        print("userspace action command")
> +        print("userspace action command", flush=True)
> +
> +
> +class psample_sample(genlmsg):
> +    nla_map = (
> +        ("PSAMPLE_ATTR_IIFINDEX", "none"),
> +        ("PSAMPLE_ATTR_OIFINDEX", "none"),
> +        ("PSAMPLE_ATTR_ORIGSIZE", "none"),
> +        ("PSAMPLE_ATTR_SAMPLE_GROUP", "uint32"),
> +        ("PSAMPLE_ATTR_GROUP_SEQ", "none"),
> +        ("PSAMPLE_ATTR_SAMPLE_RATE", "uint32"),
> +        ("PSAMPLE_ATTR_DATA", "array(uint8)"),
> +        ("PSAMPLE_ATTR_GROUP_REFCOUNT", "none"),
> +        ("PSAMPLE_ATTR_TUNNEL", "none"),
> +        ("PSAMPLE_ATTR_PAD", "none"),
> +        ("PSAMPLE_ATTR_OUT_TC", "none"),
> +        ("PSAMPLE_ATTR_OUT_TC_OCC", "none"),
> +        ("PSAMPLE_ATTR_LATENCY", "none"),
> +        ("PSAMPLE_ATTR_TIMESTAMP", "none"),
> +        ("PSAMPLE_ATTR_PROTO", "none"),
> +        ("PSAMPLE_ATTR_USER_COOKIE", "array(uint8)"),
> +    )
> +
> +    def dpstr(self):
> +        fields = []
> +        data = ""
> +        for (attr, value) in self["attrs"]:
> +            if attr == "PSAMPLE_ATTR_SAMPLE_GROUP":
> +                fields.append("group:%d" % value)
> +            if attr == "PSAMPLE_ATTR_SAMPLE_RATE":
> +                fields.append("rate:%d" % value)
> +            if attr == "PSAMPLE_ATTR_USER_COOKIE":
> +                value = "".join(format(x, "02x") for x in value)
> +                fields.append("cookie:%s" % value)
> +            if attr == "PSAMPLE_ATTR_DATA" and len(value) > 0:
> +                data = "data:%s" % "".join(format(x, "02x") for x in value)
> +
> +        return ("%s %s" % (",".join(fields), data)).strip()
> +
> +
> +class psample_msg(Marshal):
> +    PSAMPLE_CMD_SAMPLE = 0
> +    PSAMPLE_CMD_GET_GROUP = 1
> +    PSAMPLE_CMD_NEW_GROUP = 2
> +    PSAMPLE_CMD_DEL_GROUP = 3
> +    PSAMPLE_CMD_SET_FILTER = 4
> +    msg_map = {PSAMPLE_CMD_SAMPLE: psample_sample}
> +
> +
> +class PsampleEvent(EventSocket):
> +    genl_family = "psample"
> +    mcast_groups = ["packets"]
> +    marshal_class = psample_msg
> +
> +    def read_samples(self):
> +        while True:
> +            try:
> +                for msg in self.get():
> +                    print(msg.dpstr(), flush=True)
> +            except NetlinkError as ne:
> +                raise ne
>  
>  
>  def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
> @@ -2530,7 +2592,7 @@ def main(argv):
>          help="Increment 'verbose' output counter.",
>          default=0,
>      )
> -    subparsers = parser.add_subparsers()
> +    subparsers = parser.add_subparsers(dest="subcommand")
>  
>      showdpcmd = subparsers.add_parser("show")
>      showdpcmd.add_argument(
> @@ -2605,6 +2667,8 @@ def main(argv):
>      delfscmd = subparsers.add_parser("del-flows")
>      delfscmd.add_argument("flsbr", help="Datapath name")
>  
> +    subparsers.add_parser("psample-events")
> +
>      args = parser.parse_args()
>  
>      if args.verbose > 0:
> @@ -2619,6 +2683,9 @@ def main(argv):
>  
>      sys.setrecursionlimit(100000)
>  
> +    if args.subcommand == "psample-events":
> +        PsampleEvent().read_samples()
> +
>      if hasattr(args, "showdp"):
>          found = False
>          for iface in ndb.interfaces:

Re: [PATCH net-next v7 07/10] selftests: openvswitch: add psample action

[Aaron Conole]

Adrian Moreno <amorenoz@redhat.com> writes:

> Add sample and psample action support to ovs-dpctl.py.
>
> Refactor common attribute parsing logic into an external function.
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

>  .../selftests/net/openvswitch/ovs-dpctl.py    | 162 +++++++++++++++++-
>  1 file changed, 161 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 182a09975975..dcc400a21a22 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -8,6 +8,7 @@ import argparse
>  import errno
>  import ipaddress
>  import logging
> +import math
>  import multiprocessing
>  import re
>  import socket
> @@ -60,6 +61,7 @@ OVS_FLOW_CMD_DEL = 2
>  OVS_FLOW_CMD_GET = 3
>  OVS_FLOW_CMD_SET = 4
>  
> +UINT32_MAX = 0xFFFFFFFF
>  
>  def macstr(mac):
>      outstr = ":".join(["%02X" % i for i in mac])
> @@ -281,6 +283,75 @@ def parse_extract_field(
>      return str_skipped, data
>  
>  
> +def parse_attrs(actstr, attr_desc):
> +    """Parses the given action string and returns a list of netlink
> +    attributes based on a list of attribute descriptions.
> +
> +    Each element in the attribute description list is a tuple such as:
> +        (name, attr_name, parse_func)
> +    where:
> +        name: is the string representing the attribute
> +        attr_name: is the name of the attribute as defined in the uAPI.
> +        parse_func: is a callable accepting a string and returning either
> +            a single object (the parsed attribute value) or a tuple of
> +            two values (the parsed attribute value and the remaining string)
> +
> +    Returns a list of attributes and the remaining string.
> +    """
> +    def parse_attr(actstr, key, func):
> +        actstr = actstr[len(key) :]
> +
> +        if not func:
> +            return None, actstr
> +
> +        delim = actstr[0]
> +        actstr = actstr[1:]
> +
> +        if delim == "=":
> +            pos = strcspn(actstr, ",)")
> +            ret = func(actstr[:pos])
> +        else:
> +            ret = func(actstr)
> +
> +        if isinstance(ret, tuple):
> +            (datum, actstr) = ret
> +        else:
> +            datum = ret
> +            actstr = actstr[strcspn(actstr, ",)"):]
> +
> +        if delim == "(":
> +            if not actstr or actstr[0] != ")":
> +                raise ValueError("Action contains unbalanced parentheses")
> +
> +            actstr = actstr[1:]
> +
> +        actstr = actstr[strspn(actstr, ", ") :]
> +
> +        return datum, actstr
> +
> +    attrs = []
> +    attr_desc = list(attr_desc)
> +    while actstr and actstr[0] != ")" and attr_desc:
> +        found = False
> +        for i, (key, attr, func) in enumerate(attr_desc):
> +            if actstr.startswith(key):
> +                datum, actstr = parse_attr(actstr, key, func)
> +                attrs.append([attr, datum])
> +                found = True
> +                del attr_desc[i]
> +
> +        if not found:
> +            raise ValueError("Unknown attribute: '%s'" % actstr)
> +
> +        actstr = actstr[strspn(actstr, ", ") :]
> +
> +    if actstr[0] != ")":
> +        raise ValueError("Action string contains extra garbage or has "
> +                         "unbalanced parenthesis: '%s'" % actstr)
> +
> +    return attrs, actstr[1:]
> +
> +
>  class ovs_dp_msg(genlmsg):
>      # include the OVS version
>      # We need a custom header rather than just being able to rely on
> @@ -299,7 +370,7 @@ class ovsactions(nla):
>          ("OVS_ACTION_ATTR_SET", "ovskey"),
>          ("OVS_ACTION_ATTR_PUSH_VLAN", "none"),
>          ("OVS_ACTION_ATTR_POP_VLAN", "flag"),
> -        ("OVS_ACTION_ATTR_SAMPLE", "none"),
> +        ("OVS_ACTION_ATTR_SAMPLE", "sample"),
>          ("OVS_ACTION_ATTR_RECIRC", "uint32"),
>          ("OVS_ACTION_ATTR_HASH", "none"),
>          ("OVS_ACTION_ATTR_PUSH_MPLS", "none"),
> @@ -318,8 +389,85 @@ class ovsactions(nla):
>          ("OVS_ACTION_ATTR_ADD_MPLS", "none"),
>          ("OVS_ACTION_ATTR_DEC_TTL", "none"),
>          ("OVS_ACTION_ATTR_DROP", "uint32"),
> +        ("OVS_ACTION_ATTR_PSAMPLE", "psample"),
>      )
>  
> +    class psample(nla):
> +        nla_flags = NLA_F_NESTED
> +
> +        nla_map = (
> +            ("OVS_PSAMPLE_ATTR_UNSPEC", "none"),
> +            ("OVS_PSAMPLE_ATTR_GROUP", "uint32"),
> +            ("OVS_PSAMPLE_ATTR_COOKIE", "array(uint8)"),
> +        )
> +
> +        def dpstr(self, more=False):
> +            args = "group=%d" % self.get_attr("OVS_PSAMPLE_ATTR_GROUP")
> +
> +            cookie = self.get_attr("OVS_PSAMPLE_ATTR_COOKIE")
> +            if cookie:
> +                args += ",cookie(%s)" % \
> +                        "".join(format(x, "02x") for x in cookie)
> +
> +            return "psample(%s)" % args
> +
> +        def parse(self, actstr):
> +            desc = (
> +                ("group", "OVS_PSAMPLE_ATTR_GROUP", int),
> +                ("cookie", "OVS_PSAMPLE_ATTR_COOKIE",
> +                    lambda x: list(bytearray.fromhex(x)))
> +            )
> +
> +            attrs, actstr = parse_attrs(actstr, desc)
> +
> +            for attr in attrs:
> +                self["attrs"].append(attr)
> +
> +            return actstr
> +
> +    class sample(nla):
> +        nla_flags = NLA_F_NESTED
> +
> +        nla_map = (
> +            ("OVS_SAMPLE_ATTR_UNSPEC", "none"),
> +            ("OVS_SAMPLE_ATTR_PROBABILITY", "uint32"),
> +            ("OVS_SAMPLE_ATTR_ACTIONS", "ovsactions"),
> +        )
> +
> +        def dpstr(self, more=False):
> +            args = []
> +
> +            args.append("sample={:.2f}%".format(
> +                100 * self.get_attr("OVS_SAMPLE_ATTR_PROBABILITY") /
> +                UINT32_MAX))
> +
> +            actions = self.get_attr("OVS_SAMPLE_ATTR_ACTIONS")
> +            if actions:
> +                args.append("actions(%s)" % actions.dpstr(more))
> +
> +            return "sample(%s)" % ",".join(args)
> +
> +        def parse(self, actstr):
> +            def parse_nested_actions(actstr):
> +                subacts = ovsactions()
> +                parsed_len = subacts.parse(actstr)
> +                return subacts, actstr[parsed_len :]
> +
> +            def percent_to_rate(percent):
> +                percent = float(percent.strip('%'))
> +                return int(math.floor(UINT32_MAX * (percent / 100.0) + .5))
> +
> +            desc = (
> +                ("sample", "OVS_SAMPLE_ATTR_PROBABILITY", percent_to_rate),
> +                ("actions", "OVS_SAMPLE_ATTR_ACTIONS", parse_nested_actions),
> +            )
> +            attrs, actstr = parse_attrs(actstr, desc)
> +
> +            for attr in attrs:
> +                self["attrs"].append(attr)
> +
> +            return actstr
> +
>      class ctact(nla):
>          nla_flags = NLA_F_NESTED
>  
> @@ -683,6 +831,18 @@ class ovsactions(nla):
>                  self["attrs"].append(["OVS_ACTION_ATTR_CT", ctact])
>                  parsed = True
>  
> +            elif parse_starts_block(actstr, "sample(", False):
> +                sampleact = self.sample()
> +                actstr = sampleact.parse(actstr[len("sample(") : ])
> +                self["attrs"].append(["OVS_ACTION_ATTR_SAMPLE", sampleact])
> +                parsed = True
> +
> +            elif parse_starts_block(actstr, "psample(", False):
> +                psampleact = self.psample()
> +                actstr = psampleact.parse(actstr[len("psample(") : ])
> +                self["attrs"].append(["OVS_ACTION_ATTR_PSAMPLE", psampleact])
> +                parsed = True
> +
>              actstr = actstr[strspn(actstr, ", ") :]
>              while parencount > 0:
>                  parencount -= 1

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Adrián Moreno]

On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> Adrian Moreno <amorenoz@redhat.com> writes:
>
> > Add support for a new action: psample.
> >
> > This action accepts a u32 group id and a variable-length cookie and uses
> > the psample multicast group to make the packet available for
> > observability.
> >
> > The maximum length of the user-defined cookie is set to 16, same as
> > tc_cookie, to discourage using cookies that will not be offloadable.
> >
> > Acked-by: Eelco Chaudron <echaudro@redhat.com>
> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > ---
>
> Hi Adrian,
>
> Just some nits below.
>
> >  Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
> >  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
> >  net/openvswitch/Kconfig                   |  1 +
> >  net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
> >  net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
> >  5 files changed, 124 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> > index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> > --- a/Documentation/netlink/specs/ovs_flow.yaml
> > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> > @@ -727,6 +727,12 @@ attribute-sets:
> >          name: dec-ttl
> >          type: nest
> >          nested-attributes: dec-ttl-attrs
> > +      -
> > +        name: psample
> > +        type: nest
> > +        nested-attributes: psample-attrs
> > +        doc: |
> > +          Sends a packet sample to psample for external observation.
> >    -
> >      name: tunnel-key-attrs
> >      enum-name: ovs-tunnel-key-attr
> > @@ -938,6 +944,17 @@ attribute-sets:
> >        -
> >          name: gbp
> >          type: u32
> > +  -
> > +    name: psample-attrs
> > +    enum-name: ovs-psample-attr
> > +    name-prefix: ovs-psample-attr-
> > +    attributes:
> > +      -
> > +        name: group
> > +        type: u32
> > +      -
> > +        name: cookie
> > +        type: binary
> >
> >  operations:
> >    name-prefix: ovs-flow-cmd-
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index efc82c318fa2..3dd653748725 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> >  };
> >  #endif
> >
> > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> > + * action.
> > + *
> > + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > + * sample.
> > + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
> > + * contains user-defined metadata. The maximum length is
> > + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> > + *
> > + * Sends the packet to the psample multicast group with the specified group and
> > + * cookie. It is possible to combine this action with the
> > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> > + */
> > +enum ovs_psample_attr {
> > +	OVS_PSAMPLE_ATTR_GROUP = 1,	/* u32 number. */
> > +	OVS_PSAMPLE_ATTR_COOKIE,	/* Optional, user specified cookie. */
> > +
> > +	/* private: */
> > +	__OVS_PSAMPLE_ATTR_MAX
> > +};
> > +
> > +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> > +
> >  /**
> >   * enum ovs_action_attr - Action types.
> >   *
> > @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
> >   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> >   * argument.
> >   * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> > + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external observers
> > + * via psample.
> >   *
> >   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
> >   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> > @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
> >  	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
> >  	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
> >  	OVS_ACTION_ATTR_DROP,         /* u32 error code. */
> > +	OVS_ACTION_ATTR_PSAMPLE,      /* Nested OVS_PSAMPLE_ATTR_*. */
> >
> >  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
> >  				       * from userspace. */
> > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> > index 29a7081858cd..2535f3f9f462 100644
> > --- a/net/openvswitch/Kconfig
> > +++ b/net/openvswitch/Kconfig
> > @@ -10,6 +10,7 @@ config OPENVSWITCH
> >  		   (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
> >  				     (!NF_NAT || NF_NAT) && \
> >  				     (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT)))
> > +	depends on PSAMPLE || !PSAMPLE
> >  	select LIBCRC32C
> >  	select MPLS
> >  	select NET_MPLS_GSO
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index 964225580824..a035b7e677dd 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -24,6 +24,11 @@
> >  #include <net/checksum.h>
> >  #include <net/dsfield.h>
> >  #include <net/mpls.h>
> > +
> > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > +#include <net/psample.h>
> > +#endif
> > +
> >  #include <net/sctp/checksum.h>
> >
> >  #include "datapath.h"
> > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> >  	return 0;
> >  }
> >
> > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > +			    const struct nlattr *attr)
> > +{
> > +	struct psample_group psample_group = {};
> > +	struct psample_metadata md = {};
> > +	const struct nlattr *a;
> > +	int rem;
> > +
> > +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> > +		switch (nla_type(a)) {
> > +		case OVS_PSAMPLE_ATTR_GROUP:
> > +			psample_group.group_num = nla_get_u32(a);
> > +			break;
> > +
> > +		case OVS_PSAMPLE_ATTR_COOKIE:
> > +			md.user_cookie = nla_data(a);
> > +			md.user_cookie_len = nla_len(a);
> > +			break;
> > +		}
> > +	}
> > +
> > +	psample_group.net = ovs_dp_get_net(dp);
> > +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> > +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> > +
> > +	psample_sample_packet(&psample_group, skb, 0, &md);
> > +}
> > +#else
> > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > +				   const struct nlattr *attr) {}
>
> I noticed that this got flagged in patchwork since it is 'static inline'
> while being part of a complete translation unit - but I also see some
> other places where that has been done.  I guess it should be just
> 'static' though.  I don't feel very strongly about it.
>

We had a bit of discussion about this with Ilya. It seems "static
inline" is a common pattern around the kernel. The coding style
documentation says:
"Generally, inline functions are preferable to macros resembling functions."

So I think this "inline" is correct but I might be missing something.

> > +#endif
> > +
> >  /* Execute a list of actions against 'skb'. */
> >  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >  			      struct sw_flow_key *key,
> > @@ -1502,6 +1540,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >  			ovs_kfree_skb_reason(skb, reason);
> >  			return 0;
> >  		}
> > +
> > +		case OVS_ACTION_ATTR_PSAMPLE:
> > +			execute_psample(dp, skb, a);
> > +			OVS_CB(skb)->cutlen = 0;
>
> We may want to document that trunc is also impacted by psample calls.
> Right now, it is only mentioned for a single OUTPUT action.
> Alternatively, we could either ignore trunc, or not reset here.

The uAPI header says:

"
Sends the packet to the psample multicast group with the specified group and
cookie. It is possible to combine this action with the
%OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
"

Isn't this enough? What else would you add to make it even more clear?

>
> > +			if (nla_is_last(a, rem)) {
> > +				consume_skb(skb);
> > +				return 0;
> > +			}
> > +			break;
> >  		}
> >
> >  		if (unlikely(err)) {
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index f224d9bcea5e..c92bdc4dfe19 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> >  		case OVS_ACTION_ATTR_TRUNC:
> >  		case OVS_ACTION_ATTR_USERSPACE:
> >  		case OVS_ACTION_ATTR_DROP:
> > +		case OVS_ACTION_ATTR_PSAMPLE:
> >  			break;
> >
> >  		case OVS_ACTION_ATTR_CT:
> > @@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
> >  	/* Whenever new actions are added, the need to update this
> >  	 * function should be considered.
> >  	 */
> > -	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
> > +	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25);
> >
> >  	if (!actions)
> >  		return;
> > @@ -3157,6 +3158,28 @@ static int validate_and_copy_check_pkt_len(struct net *net,
> >  	return 0;
> >  }
> >
> > +static int validate_psample(const struct nlattr *attr)
> > +{
> > +	static const struct nla_policy policy[OVS_PSAMPLE_ATTR_MAX + 1] = {
> > +		[OVS_PSAMPLE_ATTR_GROUP] = { .type = NLA_U32 },
> > +		[OVS_PSAMPLE_ATTR_COOKIE] = {
> > +			.type = NLA_BINARY,
> > +			.len = OVS_PSAMPLE_COOKIE_MAX_SIZE,
> > +		},
> > +	};
> > +	struct nlattr *a[OVS_PSAMPLE_ATTR_MAX + 1];
> > +	int err;
> > +
> > +	if (!IS_ENABLED(CONFIG_PSAMPLE))
> > +		return -EOPNOTSUPP;
> > +
> > +	err = nla_parse_nested(a, OVS_PSAMPLE_ATTR_MAX, attr, policy, NULL);
> > +	if (err)
> > +		return err;
> > +
> > +	return a[OVS_PSAMPLE_ATTR_GROUP] ? 0 : -EINVAL;
> > +}
> > +
> >  static int copy_action(const struct nlattr *from,
> >  		       struct sw_flow_actions **sfa, bool log)
> >  {
> > @@ -3212,6 +3235,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >  			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
> >  			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
> >  			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
> > +			[OVS_ACTION_ATTR_PSAMPLE] = (u32)-1,
> >  		};
> >  		const struct ovs_action_push_vlan *vlan;
> >  		int type = nla_type(a);
> > @@ -3490,6 +3514,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> >  				return -EINVAL;
> >  			break;
> >
> > +		case OVS_ACTION_ATTR_PSAMPLE:
> > +			err = validate_psample(a);
> > +			if (err)
> > +				return err;
> > +			break;
> > +
> >  		default:
> >  			OVS_NLERR(log, "Unknown Action type %d", type);
> >  			return -EINVAL;
>

Re: [PATCH net-next v7 10/10] selftests: openvswitch: add psample test

[Adrián Moreno]

On Mon, Jul 01, 2024 at 02:38:44PM GMT, Aaron Conole wrote:
> Adrian Moreno <amorenoz@redhat.com> writes:
>
> > Add a test to verify sampling packets via psample works.
> >
> > In order to do that, create a subcommand in ovs-dpctl.py to listen to
> > on the psample multicast group and print samples.
> >
> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > ---
> >  .../selftests/net/openvswitch/openvswitch.sh  | 115 +++++++++++++++++-
> >  .../selftests/net/openvswitch/ovs-dpctl.py    |  73 ++++++++++-
> >  2 files changed, 182 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > index 15bca0708717..02a366e01004 100755
> > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > @@ -20,7 +20,8 @@ tests="
> >  	nat_related_v4				ip4-nat-related: ICMP related matches work with SNAT
> >  	netlink_checks				ovsnl: validate netlink attrs and settings
> >  	upcall_interfaces			ovs: test the upcall interfaces
> > -	drop_reason				drop: test drop reasons are emitted"
> > +	drop_reason				drop: test drop reasons are emitted
> > +	psample					psample: Sampling packets with psample"
> >
> >  info() {
> >      [ $VERBOSE = 0 ] || echo $*
> > @@ -102,12 +103,21 @@ ovs_netns_spawn_daemon() {
> >  	shift
> >  	netns=$1
> >  	shift
> > -	info "spawning cmd: $*"
> > -	ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
> > +	if [ "$netns" == "_default" ]; then
> > +		$*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
> > +	else
> > +		ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
> > +	fi
> >  	pid=$!
> >  	ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
> >  }
> >
> > +ovs_spawn_daemon() {
> > +	sbx=$1
> > +	shift
> > +	ovs_netns_spawn_daemon $sbx "_default" $*
> > +}
> > +
> >  ovs_add_netns_and_veths () {
> >  	info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
> >  	ovs_sbx "$1" ip netns add "$3" || return 1
> > @@ -170,6 +180,19 @@ ovs_drop_reason_count()
> >  	return `echo "$perf_output" | grep "$pattern" | wc -l`
> >  }
> >
> > +ovs_test_flow_fails () {
> > +	ERR_MSG="Flow actions may not be safe on all matching packets"
> > +
> > +	PRE_TEST=$(dmesg | grep -c "${ERR_MSG}")
> > +	ovs_add_flow $@ &> /dev/null $@ && return 1
> > +	POST_TEST=$(dmesg | grep -c "${ERR_MSG}")
> > +
> > +	if [ "$PRE_TEST" == "$POST_TEST" ]; then
> > +		return 1
> > +	fi
> > +	return 0
> > +}
> > +
> >  usage() {
> >  	echo
> >  	echo "$0 [OPTIONS] [TEST]..."
> > @@ -184,6 +207,92 @@ usage() {
> >  	exit 1
> >  }
> >
> > +
> > +# psample test
> > +# - use psample to observe packets
> > +test_psample() {
> > +	sbx_add "test_psample" || return $?
> > +
> > +	# Add a datapath with per-vport dispatching.
> > +	ovs_add_dp "test_psample" psample -V 2:1 || return 1
> > +
> > +	info "create namespaces"
> > +	ovs_add_netns_and_veths "test_psample" "psample" \
> > +		client c0 c1 172.31.110.10/24 -u || return 1
> > +	ovs_add_netns_and_veths "test_psample" "psample" \
> > +		server s0 s1 172.31.110.20/24 -u || return 1
> > +
> > +	# Check if psample actions can be configured.
> > +	ovs_add_flow "test_psample" psample \
> > +	'in_port(1),eth(),eth_type(0x0806),arp()' 'psample(group=1)'
>
> Might be good to redirect this stdout/stderr line to /dev/null -
> otherwise on an unsupported system there will be the following extra
> splat:
>
>   Traceback (most recent call last):
>     File "/home/aconole/git/linux/tools/testing/selftests/net/openvswitch/ovs-dpctl.py", line 2774, in <module>
>       sys.exit(main(sys.argv))
>    ...
>     File "/usr/lib/python3.12/site-packages/pyroute2/netlink/nlsocket.py", line 489, in get
>       raise msg['header']['error']
>   pyroute2.netlink.exceptions.NetlinkError: (22, 'Invalid argument')
>

I thought knowing the return value was kind of useful but sure, we can
redirect it to /dev/null.

> > +	if [ $? == 1 ]; then
> > +		info "no support for psample - skipping"
> > +		ovs_exit_sig
> > +		return $ksft_skip
> > +	fi
> > +
> > +	ovs_del_flows "test_psample" psample
> > +
> > +	# Test action verification.
> > +	OLDIFS=$IFS
> > +	IFS='*'
> > +	min_key='in_port(1),eth(),eth_type(0x0800),ipv4()'
> > +	for testcase in \
> > +		"cookie to large"*"psample(group=1,cookie=1615141312111009080706050403020100)" \
> > +		"no group with cookie"*"psample(cookie=abcd)" \
> > +		"no group"*"psample()";
> > +	do
> > +		set -- $testcase;
> > +		ovs_test_flow_fails "test_psample" psample $min_key $2
> > +		if [ $? == 1 ]; then
> > +			info "failed - $1"
> > +			return 1
> > +		fi
> > +	done
> > +	IFS=$OLDIFS
> > +
> > +	ovs_del_flows "test_psample" psample
> > +	# Allow ARP
> > +	ovs_add_flow "test_psample" psample \
> > +		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
> > +	ovs_add_flow "test_psample" psample \
> > +		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
> > +
> > +	# Sample first 14 bytes of all traffic.
> > +	ovs_add_flow "test_psample" psample \
> > +	    "in_port(1),eth(),eth_type(0x0800),ipv4()" \
> > +            "trunc(14),psample(group=1,cookie=c0ffee),2"
> > +
> > +	# Sample all traffic. In this case, use a sample() action with both
> > +	# psample and an upcall emulating simultaneous local sampling and
> > +	# sFlow / IPFIX.
> > +	nlpid=$(grep -E "listening on upcall packet handler" \
> > +            $ovs_dir/s0.out | cut -d ":" -f 2 | tr -d ' ')
> > +
> > +	ovs_add_flow "test_psample" psample \
> > +            "in_port(2),eth(),eth_type(0x0800),ipv4()" \
> > +            "sample(sample=100%,actions(psample(group=2,cookie=eeff0c),userspace(pid=${nlpid},userdata=eeff0c))),1"
> > +
> > +	# Record psample data.
> > +	ovs_spawn_daemon "test_psample" python3 $ovs_base/ovs-dpctl.py psample-events
> > +
> > +	# Send a single ping.
> > +	sleep 1
> > +	ovs_sbx "test_psample" ip netns exec client ping -I c1 172.31.110.20 -c 1 || return 1
> > +	sleep 1
> > +
> > +	# We should have received one userspace action upcall and 2 psample packets.
> > +	grep -E "userspace action command" $ovs_dir/s0.out >/dev/null 2>&1 || return 1
>
> I wonder if it would be better to check a few times instead of the one
> shot sleep.  There are some constrained environments that may run this
> test, and if you're worried about some kinds of races, maybe it makes
> sense to check a few times?

Yes. I thought about that. There are other sleeps in this file that I
was planning to replace with a "wait_until()" function. Should we do
this as a follow-up patch to also cover the other instances?

>
> Outside of that:
>
> Reviewed-by: Aaron Conole <aconole@redhat.com>
>
> > +
> > +	# client -> server samples should only contain the first 14 bytes of the packet.
> > +	grep -E "rate:4294967295,group:1,cookie:c0ffee data:[0-9a-f]{28}$" \
> > +			 $ovs_dir/stdout >/dev/null 2>&1 || return 1
> > +	grep -E "rate:4294967295,group:2,cookie:eeff0c" \
> > +			 $ovs_dir/stdout >/dev/null 2>&1 || return 1
> > +
> > +	return 0
> > +}
> > +
> >  # drop_reason test
> >  # - drop packets and verify the right drop reason is reported
> >  test_drop_reason() {
> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > index e8dc9af10d4d..1e15b0818074 100644
> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> > @@ -28,8 +28,10 @@ try:
> >      from pyroute2.netlink import genlmsg
> >      from pyroute2.netlink import nla
> >      from pyroute2.netlink import nlmsg_atoms
> > +    from pyroute2.netlink.event import EventSocket
> >      from pyroute2.netlink.exceptions eimport NetlinkError
> >      from pyroute2.netlink.generic import GenericNetlinkSocket
> > +    from pyroute2.netlink.nlsocket import Marshal
> >      import pyroute2
> >      import pyroute2.iproute
> >
> > @@ -2460,10 +2462,70 @@ class OvsFlow(GenericNetlinkSocket):
> >          print("MISS upcall[%d/%s]: %s" % (seq, pktpres, keystr), flush=True)
> >
> >      def execute(self, packetmsg):
> > -        print("userspace execute command")
> > +        print("userspace execute command", flush=True)
> >
> >      def action(self, packetmsg):
> > -        print("userspace action command")
> > +        print("userspace action command", flush=True)
> > +
> > +
> > +class psample_sample(genlmsg):
> > +    nla_map = (
> > +        ("PSAMPLE_ATTR_IIFINDEX", "none"),
> > +        ("PSAMPLE_ATTR_OIFINDEX", "none"),
> > +        ("PSAMPLE_ATTR_ORIGSIZE", "none"),
> > +        ("PSAMPLE_ATTR_SAMPLE_GROUP", "uint32"),
> > +        ("PSAMPLE_ATTR_GROUP_SEQ", "none"),
> > +        ("PSAMPLE_ATTR_SAMPLE_RATE", "uint32"),
> > +        ("PSAMPLE_ATTR_DATA", "array(uint8)"),
> > +        ("PSAMPLE_ATTR_GROUP_REFCOUNT", "none"),
> > +        ("PSAMPLE_ATTR_TUNNEL", "none"),
> > +        ("PSAMPLE_ATTR_PAD", "none"),
> > +        ("PSAMPLE_ATTR_OUT_TC", "none"),
> > +        ("PSAMPLE_ATTR_OUT_TC_OCC", "none"),
> > +        ("PSAMPLE_ATTR_LATENCY", "none"),
> > +        ("PSAMPLE_ATTR_TIMESTAMP", "none"),
> > +        ("PSAMPLE_ATTR_PROTO", "none"),
> > +        ("PSAMPLE_ATTR_USER_COOKIE", "array(uint8)"),
> > +    )
> > +
> > +    def dpstr(self):
> > +        fields = []
> > +        data = ""
> > +        for (attr, value) in self["attrs"]:
> > +            if attr == "PSAMPLE_ATTR_SAMPLE_GROUP":
> > +                fields.append("group:%d" % value)
> > +            if attr == "PSAMPLE_ATTR_SAMPLE_RATE":
> > +                fields.append("rate:%d" % value)
> > +            if attr == "PSAMPLE_ATTR_USER_COOKIE":
> > +                value = "".join(format(x, "02x") for x in value)
> > +                fields.append("cookie:%s" % value)
> > +            if attr == "PSAMPLE_ATTR_DATA" and len(value) > 0:
> > +                data = "data:%s" % "".join(format(x, "02x") for x in value)
> > +
> > +        return ("%s %s" % (",".join(fields), data)).strip()
> > +
> > +
> > +class psample_msg(Marshal):
> > +    PSAMPLE_CMD_SAMPLE = 0
> > +    PSAMPLE_CMD_GET_GROUP = 1
> > +    PSAMPLE_CMD_NEW_GROUP = 2
> > +    PSAMPLE_CMD_DEL_GROUP = 3
> > +    PSAMPLE_CMD_SET_FILTER = 4
> > +    msg_map = {PSAMPLE_CMD_SAMPLE: psample_sample}
> > +
> > +
> > +class PsampleEvent(EventSocket):
> > +    genl_family = "psample"
> > +    mcast_groups = ["packets"]
> > +    marshal_class = psample_msg
> > +
> > +    def read_samples(self):
> > +        while True:
> > +            try:
> > +                for msg in self.get():
> > +                    print(msg.dpstr(), flush=True)
> > +            except NetlinkError as ne:
> > +                raise ne
> >
> >
> >  def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
> > @@ -2530,7 +2592,7 @@ def main(argv):
> >          help="Increment 'verbose' output counter.",
> >          default=0,
> >      )
> > -    subparsers = parser.add_subparsers()
> > +    subparsers = parser.add_subparsers(dest="subcommand")
> >
> >      showdpcmd = subparsers.add_parser("show")
> >      showdpcmd.add_argument(
> > @@ -2605,6 +2667,8 @@ def main(argv):
> >      delfscmd = subparsers.add_parser("del-flows")
> >      delfscmd.add_argument("flsbr", help="Datapath name")
> >
> > +    subparsers.add_parser("psample-events")
> > +
> >      args = parser.parse_args()
> >
> >      if args.verbose > 0:
> > @@ -2619,6 +2683,9 @@ def main(argv):
> >
> >      sys.setrecursionlimit(100000)
> >
> > +    if args.subcommand == "psample-events":
> > +        PsampleEvent().read_samples()
> > +
> >      if hasattr(args, "showdp"):
> >          found = False
> >          for iface in ndb.interfaces:
>

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Adrián Moreno]

On Tue, Jul 02, 2024 at 03:05:02AM GMT, Adrián Moreno wrote:
> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> > Adrian Moreno <amorenoz@redhat.com> writes:
> >
> > > Add support for a new action: psample.
> > >
> > > This action accepts a u32 group id and a variable-length cookie and uses
> > > the psample multicast group to make the packet available for
> > > observability.
> > >
> > > The maximum length of the user-defined cookie is set to 16, same as
> > > tc_cookie, to discourage using cookies that will not be offloadable.
> > >
> > > Acked-by: Eelco Chaudron <echaudro@redhat.com>
> > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > > ---
> >
> > Hi Adrian,
> >
> > Just some nits below.
> >
> > >  Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
> > >  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
> > >  net/openvswitch/Kconfig                   |  1 +
> > >  net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
> > >  net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
> > >  5 files changed, 124 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
> > > index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> > > --- a/Documentation/netlink/specs/ovs_flow.yaml
> > > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> > > @@ -727,6 +727,12 @@ attribute-sets:
> > >          name: dec-ttl
> > >          type: nest
> > >          nested-attributes: dec-ttl-attrs
> > > +      -
> > > +        name: psample
> > > +        type: nest
> > > +        nested-attributes: psample-attrs
> > > +        doc: |
> > > +          Sends a packet sample to psample for external observation.
> > >    -
> > >      name: tunnel-key-attrs
> > >      enum-name: ovs-tunnel-key-attr
> > > @@ -938,6 +944,17 @@ attribute-sets:
> > >        -
> > >          name: gbp
> > >          type: u32
> > > +  -
> > > +    name: psample-attrs
> > > +    enum-name: ovs-psample-attr
> > > +    name-prefix: ovs-psample-attr-
> > > +    attributes:
> > > +      -
> > > +        name: group
> > > +        type: u32
> > > +      -
> > > +        name: cookie
> > > +        type: binary
> > >
> > >  operations:
> > >    name-prefix: ovs-flow-cmd-
> > > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > > index efc82c318fa2..3dd653748725 100644
> > > --- a/include/uapi/linux/openvswitch.h
> > > +++ b/include/uapi/linux/openvswitch.h
> > > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> > >  };
> > >  #endif
> > >
> > > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> > > +/**
> > > + * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> > > + * action.
> > > + *
> > > + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > > + * sample.
> > > + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
> > > + * contains user-defined metadata. The maximum length is
> > > + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> > > + *
> > > + * Sends the packet to the psample multicast group with the specified group and
> > > + * cookie. It is possible to combine this action with the
> > > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> > > + */
> > > +enum ovs_psample_attr {
> > > +	OVS_PSAMPLE_ATTR_GROUP = 1,	/* u32 number. */
> > > +	OVS_PSAMPLE_ATTR_COOKIE,	/* Optional, user specified cookie. */
> > > +
> > > +	/* private: */
> > > +	__OVS_PSAMPLE_ATTR_MAX
> > > +};
> > > +
> > > +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> > > +
> > >  /**
> > >   * enum ovs_action_attr - Action types.
> > >   *
> > > @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
> > >   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> > >   * argument.
> > >   * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> > > + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external observers
> > > + * via psample.
> > >   *
> > >   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
> > >   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> > > @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
> > >  	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
> > >  	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
> > >  	OVS_ACTION_ATTR_DROP,         /* u32 error code. */
> > > +	OVS_ACTION_ATTR_PSAMPLE,      /* Nested OVS_PSAMPLE_ATTR_*. */
> > >
> > >  	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
> > >  				       * from userspace. */
> > > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> > > index 29a7081858cd..2535f3f9f462 100644
> > > --- a/net/openvswitch/Kconfig
> > > +++ b/net/openvswitch/Kconfig
> > > @@ -10,6 +10,7 @@ config OPENVSWITCH
> > >  		   (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
> > >  				     (!NF_NAT || NF_NAT) && \
> > >  				     (!NETFILTER_CONNCOUNT || NETFILTER_CONNCOUNT)))
> > > +	depends on PSAMPLE || !PSAMPLE
> > >  	select LIBCRC32C
> > >  	select MPLS
> > >  	select NET_MPLS_GSO
> > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > > index 964225580824..a035b7e677dd 100644
> > > --- a/net/openvswitch/actions.c
> > > +++ b/net/openvswitch/actions.c
> > > @@ -24,6 +24,11 @@
> > >  #include <net/checksum.h>
> > >  #include <net/dsfield.h>
> > >  #include <net/mpls.h>
> > > +
> > > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > > +#include <net/psample.h>
> > > +#endif
> > > +
> > >  #include <net/sctp/checksum.h>
> > >
> > >  #include "datapath.h"
> > > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> > >  	return 0;
> > >  }
> > >
> > > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > > +			    const struct nlattr *attr)
> > > +{
> > > +	struct psample_group psample_group = {};
> > > +	struct psample_metadata md = {};
> > > +	const struct nlattr *a;
> > > +	int rem;
> > > +
> > > +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> > > +		switch (nla_type(a)) {
> > > +		case OVS_PSAMPLE_ATTR_GROUP:
> > > +			psample_group.group_num = nla_get_u32(a);
> > > +			break;
> > > +
> > > +		case OVS_PSAMPLE_ATTR_COOKIE:
> > > +			md.user_cookie = nla_data(a);
> > > +			md.user_cookie_len = nla_len(a);
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	psample_group.net = ovs_dp_get_net(dp);
> > > +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> > > +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> > > +
> > > +	psample_sample_packet(&psample_group, skb, 0, &md);
> > > +}
> > > +#else
> > > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > > +				   const struct nlattr *attr) {}
> >
> > I noticed that this got flagged in patchwork since it is 'static inline'
> > while being part of a complete translation unit - but I also see some
> > other places where that has been done.  I guess it should be just
> > 'static' though.  I don't feel very strongly about it.
> >
>
> We had a bit of discussion about this with Ilya. It seems "static
> inline" is a common pattern around the kernel. The coding style
> documentation says:
> "Generally, inline functions are preferable to macros resembling functions."
>
> So I think this "inline" is correct but I might be missing something.
>
> > > +#endif
> > > +
> > >  /* Execute a list of actions against 'skb'. */
> > >  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> > >  			      struct sw_flow_key *key,
> > > @@ -1502,6 +1540,15 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> > >  			ovs_kfree_skb_reason(skb, reason);
> > >  			return 0;
> > >  		}
> > > +
> > > +		case OVS_ACTION_ATTR_PSAMPLE:
> > > +			execute_psample(dp, skb, a);
> > > +			OVS_CB(skb)->cutlen = 0;
> >
> > We may want to document that trunc is also impacted by psample calls.
> > Right now, it is only mentioned for a single OUTPUT action.
> > Alternatively, we could either ignore trunc, or not reset here.
>
> The uAPI header says:
>
> "
> Sends the packet to the psample multicast group with the specified group and
> cookie. It is possible to combine this action with the
> %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> "
>
> Isn't this enough? What else would you add to make it even more clear?
>

BTW trunc also affects userspace and it's not explicitly documented.

> >
> > > +			if (nla_is_last(a, rem)) {
> > > +				consume_skb(skb);
> > > +				return 0;
> > > +			}
> > > +			break;
> > >  		}
> > >
> > >  		if (unlikely(err)) {
> > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > > index f224d9bcea5e..c92bdc4dfe19 100644
> > > --- a/net/openvswitch/flow_netlink.c
> > > +++ b/net/openvswitch/flow_netlink.c
> > > @@ -64,6 +64,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> > >  		case OVS_ACTION_ATTR_TRUNC:
> > >  		case OVS_ACTION_ATTR_USERSPACE:
> > >  		case OVS_ACTION_ATTR_DROP:
> > > +		case OVS_ACTION_ATTR_PSAMPLE:
> > >  			break;
> > >
> > >  		case OVS_ACTION_ATTR_CT:
> > > @@ -2409,7 +2410,7 @@ static void ovs_nla_free_nested_actions(const struct nlattr *actions, int len)
> > >  	/* Whenever new actions are added, the need to update this
> > >  	 * function should be considered.
> > >  	 */
> > > -	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
> > > +	BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 25);
> > >
> > >  	if (!actions)
> > >  		return;
> > > @@ -3157,6 +3158,28 @@ static int validate_and_copy_check_pkt_len(struct net *net,
> > >  	return 0;
> > >  }
> > >
> > > +static int validate_psample(const struct nlattr *attr)
> > > +{
> > > +	static const struct nla_policy policy[OVS_PSAMPLE_ATTR_MAX + 1] = {
> > > +		[OVS_PSAMPLE_ATTR_GROUP] = { .type = NLA_U32 },
> > > +		[OVS_PSAMPLE_ATTR_COOKIE] = {
> > > +			.type = NLA_BINARY,
> > > +			.len = OVS_PSAMPLE_COOKIE_MAX_SIZE,
> > > +		},
> > > +	};
> > > +	struct nlattr *a[OVS_PSAMPLE_ATTR_MAX + 1];
> > > +	int err;
> > > +
> > > +	if (!IS_ENABLED(CONFIG_PSAMPLE))
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	err = nla_parse_nested(a, OVS_PSAMPLE_ATTR_MAX, attr, policy, NULL);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	return a[OVS_PSAMPLE_ATTR_GROUP] ? 0 : -EINVAL;
> > > +}
> > > +
> > >  static int copy_action(const struct nlattr *from,
> > >  		       struct sw_flow_actions **sfa, bool log)
> > >  {
> > > @@ -3212,6 +3235,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > >  			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
> > >  			[OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
> > >  			[OVS_ACTION_ATTR_DROP] = sizeof(u32),
> > > +			[OVS_ACTION_ATTR_PSAMPLE] = (u32)-1,
> > >  		};
> > >  		const struct ovs_action_push_vlan *vlan;
> > >  		int type = nla_type(a);
> > > @@ -3490,6 +3514,12 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
> > >  				return -EINVAL;
> > >  			break;
> > >
> > > +		case OVS_ACTION_ATTR_PSAMPLE:
> > > +			err = validate_psample(a);
> > > +			if (err)
> > > +				return err;
> > > +			break;
> > > +
> > >  		default:
> > >  			OVS_NLERR(log, "Unknown Action type %d", type);
> > >  			return -EINVAL;
> >

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Simon Horman]

On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> > Adrian Moreno <amorenoz@redhat.com> writes:

...

> > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c

...

> > > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> > >  	return 0;
> > >  }
> > >
> > > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > > +			    const struct nlattr *attr)
> > > +{
> > > +	struct psample_group psample_group = {};
> > > +	struct psample_metadata md = {};
> > > +	const struct nlattr *a;
> > > +	int rem;
> > > +
> > > +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> > > +		switch (nla_type(a)) {
> > > +		case OVS_PSAMPLE_ATTR_GROUP:
> > > +			psample_group.group_num = nla_get_u32(a);
> > > +			break;
> > > +
> > > +		case OVS_PSAMPLE_ATTR_COOKIE:
> > > +			md.user_cookie = nla_data(a);
> > > +			md.user_cookie_len = nla_len(a);
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	psample_group.net = ovs_dp_get_net(dp);
> > > +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> > > +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> > > +
> > > +	psample_sample_packet(&psample_group, skb, 0, &md);
> > > +}
> > > +#else
> > > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > > +				   const struct nlattr *attr) {}
> >
> > I noticed that this got flagged in patchwork since it is 'static inline'
> > while being part of a complete translation unit - but I also see some
> > other places where that has been done.  I guess it should be just
> > 'static' though.  I don't feel very strongly about it.
> >
> 
> We had a bit of discussion about this with Ilya. It seems "static
> inline" is a common pattern around the kernel. The coding style
> documentation says:
> "Generally, inline functions are preferable to macros resembling functions."
> 
> So I think this "inline" is correct but I might be missing something.

Hi Adrián,

TL;DR: Please remove this inline keyword

For Kernel networking code at least it is strongly preferred not
to use inline in .c files unless there is a demonstrable - usually
performance - reason to do so. Rather, it is preferred to let the
compiler decide when to inline such functions. OTOH, the inline
keyword in .h files is fine.

...

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Adrián Moreno]

On Tue, Jul 02, 2024 at 10:37:26AM GMT, Simon Horman wrote:
> On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
> > On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> > > Adrian Moreno <amorenoz@redhat.com> writes:
>
> ...
>
> > > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>
> ...
>
> > > > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > > > +			    const struct nlattr *attr)
> > > > +{
> > > > +	struct psample_group psample_group = {};
> > > > +	struct psample_metadata md = {};
> > > > +	const struct nlattr *a;
> > > > +	int rem;
> > > > +
> > > > +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> > > > +		switch (nla_type(a)) {
> > > > +		case OVS_PSAMPLE_ATTR_GROUP:
> > > > +			psample_group.group_num = nla_get_u32(a);
> > > > +			break;
> > > > +
> > > > +		case OVS_PSAMPLE_ATTR_COOKIE:
> > > > +			md.user_cookie = nla_data(a);
> > > > +			md.user_cookie_len = nla_len(a);
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	psample_group.net = ovs_dp_get_net(dp);
> > > > +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> > > > +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> > > > +
> > > > +	psample_sample_packet(&psample_group, skb, 0, &md);
> > > > +}
> > > > +#else
> > > > +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > > > +				   const struct nlattr *attr) {}
> > >
> > > I noticed that this got flagged in patchwork since it is 'static inline'
> > > while being part of a complete translation unit - but I also see some
> > > other places where that has been done.  I guess it should be just
> > > 'static' though.  I don't feel very strongly about it.
> > >
> >
> > We had a bit of discussion about this with Ilya. It seems "static
> > inline" is a common pattern around the kernel. The coding style
> > documentation says:
> > "Generally, inline functions are preferable to macros resembling functions."
> >
> > So I think this "inline" is correct but I might be missing something.
>
> Hi Adrián,
>
> TL;DR: Please remove this inline keyword
>
> For Kernel networking code at least it is strongly preferred not
> to use inline in .c files unless there is a demonstrable - usually
> performance - reason to do so. Rather, it is preferred to let the
> compiler decide when to inline such functions. OTOH, the inline
> keyword in .h files is fine.
>

Ok. I'll send a new version.

Thanks.
Adrián

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Ilya Maximets]

On 7/2/24 11:37, Simon Horman wrote:
> On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
>> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
>>> Adrian Moreno <amorenoz@redhat.com> writes:
> 
> ...
> 
>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> 
> ...
> 
>>>> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
>>>>  	return 0;
>>>>  }
>>>>
>>>> +#if IS_ENABLED(CONFIG_PSAMPLE)
>>>> +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
>>>> +			    const struct nlattr *attr)
>>>> +{
>>>> +	struct psample_group psample_group = {};
>>>> +	struct psample_metadata md = {};
>>>> +	const struct nlattr *a;
>>>> +	int rem;
>>>> +
>>>> +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
>>>> +		switch (nla_type(a)) {
>>>> +		case OVS_PSAMPLE_ATTR_GROUP:
>>>> +			psample_group.group_num = nla_get_u32(a);
>>>> +			break;
>>>> +
>>>> +		case OVS_PSAMPLE_ATTR_COOKIE:
>>>> +			md.user_cookie = nla_data(a);
>>>> +			md.user_cookie_len = nla_len(a);
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	psample_group.net = ovs_dp_get_net(dp);
>>>> +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
>>>> +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
>>>> +
>>>> +	psample_sample_packet(&psample_group, skb, 0, &md);
>>>> +}
>>>> +#else
>>>> +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
>>>> +				   const struct nlattr *attr) {}
>>>
>>> I noticed that this got flagged in patchwork since it is 'static inline'
>>> while being part of a complete translation unit - but I also see some
>>> other places where that has been done.  I guess it should be just
>>> 'static' though.  I don't feel very strongly about it.
>>>
>>
>> We had a bit of discussion about this with Ilya. It seems "static
>> inline" is a common pattern around the kernel. The coding style
>> documentation says:
>> "Generally, inline functions are preferable to macros resembling functions."
>>
>> So I think this "inline" is correct but I might be missing something.
> 
> Hi Adrián,
> 
> TL;DR: Please remove this inline keyword
> 
> For Kernel networking code at least it is strongly preferred not
> to use inline in .c files unless there is a demonstrable - usually
> performance - reason to do so. Rather, it is preferred to let the
> compiler decide when to inline such functions. OTOH, the inline
> keyword in .h files is fine.

FWIW, the main reason for 'inline' here is not performance, but silencing
compiler's potential 'maybe unused' warnings:

 Function-like macros with unused parameters should be replaced by static
 inline functions to avoid the issue of unused variables

I think, the rule for static inline functions in .c files is at odds with
the 'Conditional Compilation' section of coding style.  The section does
recommend to avoid conditional function declaration in .c files, but I'm not
sure it is reasonable to export internal static functions for that reason.

In this particular case we can either define a macro, which is discouraged
by the coding style:

 Generally, inline functions are preferable to macros resembling functions.

Or create a static inline function, that is against rule of no static
inline functions in .c files.

Or create a simple static function and mark all the arguments as unused,
which kind of compliant to the coding style, but the least pretty.

Best regards, Ilya Maximets.

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Michal Kubiak]

On Mon, Jul 01, 2024 at 12:56:43PM +0000, Adrián Moreno wrote:
> On Mon, Jul 01, 2024 at 01:40:39PM GMT, Michal Kubiak wrote:
> > On Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote:
> > > Add support for a new action: psample.
> > >
> > > This action accepts a u32 group id and a variable-length cookie and uses
> > > the psample multicast group to make the packet available for
> > > observability.
> > >
> > > The maximum length of the user-defined cookie is set to 16, same as
> > > tc_cookie, to discourage using cookies that will not be offloadable.
> > >
> > > Acked-by: Eelco Chaudron <echaudro@redhat.com>
> > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > > ---
> > >  Documentation/netlink/specs/ovs_flow.yaml | 17 ++++++++
> > >  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
> > >  net/openvswitch/Kconfig                   |  1 +
> > >  net/openvswitch/actions.c                 | 47 +++++++++++++++++++++++
> > >  net/openvswitch/flow_netlink.c            | 32 ++++++++++++++-
> > >  5 files changed, 124 insertions(+), 1 deletion(-)
> > >

[...]

> > > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> > >  };
> > >  #endif
> > >
> > > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> >
> > In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your
> > cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE,
> > so this size will be validated correctly.
> > But how likely is that those 2 constants will have different values in the
> > future?
> > Would it be reasonable to create more strict dependency between those
> > macros, e.g.:
> >
> > #define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE
> >
> > or, at least, add a comment that the size shouldn't be bigger than
> > TC_COOKIE_MAX_SIZE?
> > I'm just considering the risk of exceeding the array from the patch #2 when
> > somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future.
> >
> > Thanks,
> > Michal
> >
> 
> Hi Michal,
> 
> Thanks for sharing your thoughts.
> 
> I tried to keep the dependency between both cookie sizes loose.
> 
> I don't want a change in TC_COOKIE_MAX_SIZE to inadvertently modify
> OVS_PSAMPLE_COOKIE_MAX_SIZE because OVS might not need a bigger cookie
> and even if it does, backwards compatibility needs to be guaranteed:
> meaning OVS userspace will have to detect the new size and use it or
> fall back to a smaller cookie for older kernels. All this needs to be
> known and worked on in userspace.
> 
> On the other hand, I intentionally made OVS's "psample" action as
> similar as possible to act_psample, including setting the same cookie
> size to begin with. The reason is that I think we should try to implement
> tc-flower offloading of this action using act_sample, plus 16 seemed a
> very reasonable max value.
> 
> When we decide to support offloading the "psample" action, this must
> be done entirely in userspace. OVS must create a act_sample action
> (instead of the OVS "psample" one) via netlink. In no circumstances the
> openvswitch kmod interacts with tc directly.
> 
> Now, back to your concern. If OVS_PSAMPLE_COOKIE_MAX_SIZE grows and
> TC_COOKIE_MAX_SIZE does not *and* we already support offloading this
> action to tc, the only consequence is that OVS userspace has a
> problem because the tc's netlink interface will reject cookies larger
> than TC_COOKIE_MAX_SIZE [1].
> This guarantees that the array in patch #2 is never overflown.
> 
> OVS will have to deal with the different sizes and try to squeeze the
> data into TC_COOKIE_MAX_SIZE or fail to offload the action altogether.
> 
> Psample does not have a size limit so different parts of the kernel can
> use psample with different internal max-sizes without any restriction.
> 
> I hope this clears your concerns.
> 
> [1] https://github.com/torvalds/linux/blob/master/net/sched/act_api.c#L1299
> 
> Thanks.
> Adrián
> 

Thank you, Adrian, for your detailed explanation. I wasn't aware of the
internal validation of that parameter using the mechanism from [1].
Sorry for asking the questions I should have answered by studying the
code more carefully.

I have no concerns about it now.

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Re: [PATCH net-next v7 10/10] selftests: openvswitch: add psample test

[Aaron Conole]

Adrián Moreno <amorenoz@redhat.com> writes:

> On Mon, Jul 01, 2024 at 02:38:44PM GMT, Aaron Conole wrote:
>> Adrian Moreno <amorenoz@redhat.com> writes:
>>
>> > Add a test to verify sampling packets via psample works.
>> >
>> > In order to do that, create a subcommand in ovs-dpctl.py to listen to
>> > on the psample multicast group and print samples.
>> >
>> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> > ---
>> >  .../selftests/net/openvswitch/openvswitch.sh  | 115 +++++++++++++++++-
>> >  .../selftests/net/openvswitch/ovs-dpctl.py    |  73 ++++++++++-
>> >  2 files changed, 182 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > index 15bca0708717..02a366e01004 100755
>> > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > @@ -20,7 +20,8 @@ tests="
>> >  	nat_related_v4				ip4-nat-related: ICMP related matches work with SNAT
>> >  	netlink_checks				ovsnl: validate netlink attrs and settings
>> >  	upcall_interfaces			ovs: test the upcall interfaces
>> > -	drop_reason				drop: test drop reasons are emitted"
>> > +	drop_reason				drop: test drop reasons are emitted
>> > +	psample					psample: Sampling packets with psample"
>> >
>> >  info() {
>> >      [ $VERBOSE = 0 ] || echo $*
>> > @@ -102,12 +103,21 @@ ovs_netns_spawn_daemon() {
>> >  	shift
>> >  	netns=$1
>> >  	shift
>> > -	info "spawning cmd: $*"
>> > -	ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
>> > +	if [ "$netns" == "_default" ]; then
>> > +		$*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
>> > +	else
>> > +		ip netns exec $netns $*  >> $ovs_dir/stdout  2>> $ovs_dir/stderr &
>> > +	fi
>> >  	pid=$!
>> >  	ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
>> >  }
>> >
>> > +ovs_spawn_daemon() {
>> > +	sbx=$1
>> > +	shift
>> > +	ovs_netns_spawn_daemon $sbx "_default" $*
>> > +}
>> > +
>> >  ovs_add_netns_and_veths () {
>> >  	info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
>> >  	ovs_sbx "$1" ip netns add "$3" || return 1
>> > @@ -170,6 +180,19 @@ ovs_drop_reason_count()
>> >  	return `echo "$perf_output" | grep "$pattern" | wc -l`
>> >  }
>> >
>> > +ovs_test_flow_fails () {
>> > +	ERR_MSG="Flow actions may not be safe on all matching packets"
>> > +
>> > +	PRE_TEST=$(dmesg | grep -c "${ERR_MSG}")
>> > +	ovs_add_flow $@ &> /dev/null $@ && return 1
>> > +	POST_TEST=$(dmesg | grep -c "${ERR_MSG}")
>> > +
>> > +	if [ "$PRE_TEST" == "$POST_TEST" ]; then
>> > +		return 1
>> > +	fi
>> > +	return 0
>> > +}
>> > +
>> >  usage() {
>> >  	echo
>> >  	echo "$0 [OPTIONS] [TEST]..."
>> > @@ -184,6 +207,92 @@ usage() {
>> >  	exit 1
>> >  }
>> >
>> > +
>> > +# psample test
>> > +# - use psample to observe packets
>> > +test_psample() {
>> > +	sbx_add "test_psample" || return $?
>> > +
>> > +	# Add a datapath with per-vport dispatching.
>> > +	ovs_add_dp "test_psample" psample -V 2:1 || return 1
>> > +
>> > +	info "create namespaces"
>> > +	ovs_add_netns_and_veths "test_psample" "psample" \
>> > +		client c0 c1 172.31.110.10/24 -u || return 1
>> > +	ovs_add_netns_and_veths "test_psample" "psample" \
>> > +		server s0 s1 172.31.110.20/24 -u || return 1
>> > +
>> > +	# Check if psample actions can be configured.
>> > +	ovs_add_flow "test_psample" psample \
>> > +	'in_port(1),eth(),eth_type(0x0806),arp()' 'psample(group=1)'
>>
>> Might be good to redirect this stdout/stderr line to /dev/null -
>> otherwise on an unsupported system there will be the following extra
>> splat:
>>
>>   Traceback (most recent call last):
>>     File "/home/aconole/git/linux/tools/testing/selftests/net/openvswitch/ovs-dpctl.py", line 2774, in <module>
>>       sys.exit(main(sys.argv))
>>    ...
>>     File "/usr/lib/python3.12/site-packages/pyroute2/netlink/nlsocket.py", line 489, in get
>>       raise msg['header']['error']
>>   pyroute2.netlink.exceptions.NetlinkError: (22, 'Invalid argument')
>>
>
> I thought knowing the return value was kind of useful but sure, we can
> redirect it to /dev/null.
>
>> > +	if [ $? == 1 ]; then
>> > +		info "no support for psample - skipping"
>> > +		ovs_exit_sig
>> > +		return $ksft_skip
>> > +	fi
>> > +
>> > +	ovs_del_flows "test_psample" psample
>> > +
>> > +	# Test action verification.
>> > +	OLDIFS=$IFS
>> > +	IFS='*'
>> > +	min_key='in_port(1),eth(),eth_type(0x0800),ipv4()'
>> > +	for testcase in \
>> > + "cookie to
>> > large"*"psample(group=1,cookie=1615141312111009080706050403020100)"
>> > \
>> > +		"no group with cookie"*"psample(cookie=abcd)" \
>> > +		"no group"*"psample()";
>> > +	do
>> > +		set -- $testcase;
>> > +		ovs_test_flow_fails "test_psample" psample $min_key $2
>> > +		if [ $? == 1 ]; then
>> > +			info "failed - $1"
>> > +			return 1
>> > +		fi
>> > +	done
>> > +	IFS=$OLDIFS
>> > +
>> > +	ovs_del_flows "test_psample" psample
>> > +	# Allow ARP
>> > +	ovs_add_flow "test_psample" psample \
>> > +		'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
>> > +	ovs_add_flow "test_psample" psample \
>> > +		'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
>> > +
>> > +	# Sample first 14 bytes of all traffic.
>> > +	ovs_add_flow "test_psample" psample \
>> > +	    "in_port(1),eth(),eth_type(0x0800),ipv4()" \
>> > +            "trunc(14),psample(group=1,cookie=c0ffee),2"
>> > +
>> > +	# Sample all traffic. In this case, use a sample() action with both
>> > +	# psample and an upcall emulating simultaneous local sampling and
>> > +	# sFlow / IPFIX.
>> > +	nlpid=$(grep -E "listening on upcall packet handler" \
>> > +            $ovs_dir/s0.out | cut -d ":" -f 2 | tr -d ' ')
>> > +
>> > +	ovs_add_flow "test_psample" psample \
>> > +            "in_port(2),eth(),eth_type(0x0800),ipv4()" \
>> > +
>> > "sample(sample=100%,actions(psample(group=2,cookie=eeff0c),userspace(pid=${nlpid},userdata=eeff0c))),1"
>> > +
>> > +	# Record psample data.
>> > + ovs_spawn_daemon "test_psample" python3 $ovs_base/ovs-dpctl.py
>> > psample-events
>> > +
>> > +	# Send a single ping.
>> > +	sleep 1
>> > + ovs_sbx "test_psample" ip netns exec client ping -I c1
>> > 172.31.110.20 -c 1 || return 1
>> > +	sleep 1
>> > +
>> > + # We should have received one userspace action upcall and 2
>> > psample packets.
>> > + grep -E "userspace action command" $ovs_dir/s0.out >/dev/null
>> > 2>&1 || return 1
>>
>> I wonder if it would be better to check a few times instead of the one
>> shot sleep.  There are some constrained environments that may run this
>> test, and if you're worried about some kinds of races, maybe it makes
>> sense to check a few times?
>
> Yes. I thought about that. There are other sleeps in this file that I
> was planning to replace with a "wait_until()" function. Should we do
> this as a follow-up patch to also cover the other instances?

That makes sense to me.

>>
>> Outside of that:
>>
>> Reviewed-by: Aaron Conole <aconole@redhat.com>
>>
>> > +
>> > +	# client -> server samples should only contain the first 14 bytes of the packet.
>> > +	grep -E "rate:4294967295,group:1,cookie:c0ffee data:[0-9a-f]{28}$" \
>> > +			 $ovs_dir/stdout >/dev/null 2>&1 || return 1
>> > +	grep -E "rate:4294967295,group:2,cookie:eeff0c" \
>> > +			 $ovs_dir/stdout >/dev/null 2>&1 || return 1
>> > +
>> > +	return 0
>> > +}
>> > +
>> >  # drop_reason test
>> >  # - drop packets and verify the right drop reason is reported
>> >  test_drop_reason() {
>> > diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > index e8dc9af10d4d..1e15b0818074 100644
>> > --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
>> > @@ -28,8 +28,10 @@ try:
>> >      from pyroute2.netlink import genlmsg
>> >      from pyroute2.netlink import nla
>> >      from pyroute2.netlink import nlmsg_atoms
>> > +    from pyroute2.netlink.event import EventSocket
>> >      from pyroute2.netlink.exceptions eimport NetlinkError
>> >      from pyroute2.netlink.generic import GenericNetlinkSocket
>> > +    from pyroute2.netlink.nlsocket import Marshal
>> >      import pyroute2
>> >      import pyroute2.iproute
>> >
>> > @@ -2460,10 +2462,70 @@ class OvsFlow(GenericNetlinkSocket):
>> >          print("MISS upcall[%d/%s]: %s" % (seq, pktpres, keystr), flush=True)
>> >
>> >      def execute(self, packetmsg):
>> > -        print("userspace execute command")
>> > +        print("userspace execute command", flush=True)
>> >
>> >      def action(self, packetmsg):
>> > -        print("userspace action command")
>> > +        print("userspace action command", flush=True)
>> > +
>> > +
>> > +class psample_sample(genlmsg):
>> > +    nla_map = (
>> > +        ("PSAMPLE_ATTR_IIFINDEX", "none"),
>> > +        ("PSAMPLE_ATTR_OIFINDEX", "none"),
>> > +        ("PSAMPLE_ATTR_ORIGSIZE", "none"),
>> > +        ("PSAMPLE_ATTR_SAMPLE_GROUP", "uint32"),
>> > +        ("PSAMPLE_ATTR_GROUP_SEQ", "none"),
>> > +        ("PSAMPLE_ATTR_SAMPLE_RATE", "uint32"),
>> > +        ("PSAMPLE_ATTR_DATA", "array(uint8)"),
>> > +        ("PSAMPLE_ATTR_GROUP_REFCOUNT", "none"),
>> > +        ("PSAMPLE_ATTR_TUNNEL", "none"),
>> > +        ("PSAMPLE_ATTR_PAD", "none"),
>> > +        ("PSAMPLE_ATTR_OUT_TC", "none"),
>> > +        ("PSAMPLE_ATTR_OUT_TC_OCC", "none"),
>> > +        ("PSAMPLE_ATTR_LATENCY", "none"),
>> > +        ("PSAMPLE_ATTR_TIMESTAMP", "none"),
>> > +        ("PSAMPLE_ATTR_PROTO", "none"),
>> > +        ("PSAMPLE_ATTR_USER_COOKIE", "array(uint8)"),
>> > +    )
>> > +
>> > +    def dpstr(self):
>> > +        fields = []
>> > +        data = ""
>> > +        for (attr, value) in self["attrs"]:
>> > +            if attr == "PSAMPLE_ATTR_SAMPLE_GROUP":
>> > +                fields.append("group:%d" % value)
>> > +            if attr == "PSAMPLE_ATTR_SAMPLE_RATE":
>> > +                fields.append("rate:%d" % value)
>> > +            if attr == "PSAMPLE_ATTR_USER_COOKIE":
>> > +                value = "".join(format(x, "02x") for x in value)
>> > +                fields.append("cookie:%s" % value)
>> > +            if attr == "PSAMPLE_ATTR_DATA" and len(value) > 0:
>> > +                data = "data:%s" % "".join(format(x, "02x") for x in value)
>> > +
>> > +        return ("%s %s" % (",".join(fields), data)).strip()
>> > +
>> > +
>> > +class psample_msg(Marshal):
>> > +    PSAMPLE_CMD_SAMPLE = 0
>> > +    PSAMPLE_CMD_GET_GROUP = 1
>> > +    PSAMPLE_CMD_NEW_GROUP = 2
>> > +    PSAMPLE_CMD_DEL_GROUP = 3
>> > +    PSAMPLE_CMD_SET_FILTER = 4
>> > +    msg_map = {PSAMPLE_CMD_SAMPLE: psample_sample}
>> > +
>> > +
>> > +class PsampleEvent(EventSocket):
>> > +    genl_family = "psample"
>> > +    mcast_groups = ["packets"]
>> > +    marshal_class = psample_msg
>> > +
>> > +    def read_samples(self):
>> > +        while True:
>> > +            try:
>> > +                for msg in self.get():
>> > +                    print(msg.dpstr(), flush=True)
>> > +            except NetlinkError as ne:
>> > +                raise ne
>> >
>> >
>> >  def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
>> > @@ -2530,7 +2592,7 @@ def main(argv):
>> >          help="Increment 'verbose' output counter.",
>> >          default=0,
>> >      )
>> > -    subparsers = parser.add_subparsers()
>> > +    subparsers = parser.add_subparsers(dest="subcommand")
>> >
>> >      showdpcmd = subparsers.add_parser("show")
>> >      showdpcmd.add_argument(
>> > @@ -2605,6 +2667,8 @@ def main(argv):
>> >      delfscmd = subparsers.add_parser("del-flows")
>> >      delfscmd.add_argument("flsbr", help="Datapath name")
>> >
>> > +    subparsers.add_parser("psample-events")
>> > +
>> >      args = parser.parse_args()
>> >
>> >      if args.verbose > 0:
>> > @@ -2619,6 +2683,9 @@ def main(argv):
>> >
>> >      sys.setrecursionlimit(100000)
>> >
>> > +    if args.subcommand == "psample-events":
>> > +        PsampleEvent().read_samples()
>> > +
>> >      if hasattr(args, "showdp"):
>> >          found = False
>> >          for iface in ndb.interfaces:
>>

Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Simon Horman]

On Tue, Jul 02, 2024 at 11:53:01AM +0200, Ilya Maximets wrote:
> On 7/2/24 11:37, Simon Horman wrote:
> > On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
> >> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> >>> Adrian Moreno <amorenoz@redhat.com> writes:
> > 
> > ...
> > 
> >>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > 
> > ...
> > 
> >>>> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> >>>>  	return 0;
> >>>>  }
> >>>>
> >>>> +#if IS_ENABLED(CONFIG_PSAMPLE)
> >>>> +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> >>>> +			    const struct nlattr *attr)
> >>>> +{
> >>>> +	struct psample_group psample_group = {};
> >>>> +	struct psample_metadata md = {};
> >>>> +	const struct nlattr *a;
> >>>> +	int rem;
> >>>> +
> >>>> +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> >>>> +		switch (nla_type(a)) {
> >>>> +		case OVS_PSAMPLE_ATTR_GROUP:
> >>>> +			psample_group.group_num = nla_get_u32(a);
> >>>> +			break;
> >>>> +
> >>>> +		case OVS_PSAMPLE_ATTR_COOKIE:
> >>>> +			md.user_cookie = nla_data(a);
> >>>> +			md.user_cookie_len = nla_len(a);
> >>>> +			break;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	psample_group.net = ovs_dp_get_net(dp);
> >>>> +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> >>>> +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> >>>> +
> >>>> +	psample_sample_packet(&psample_group, skb, 0, &md);
> >>>> +}
> >>>> +#else
> >>>> +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> >>>> +				   const struct nlattr *attr) {}
> >>>
> >>> I noticed that this got flagged in patchwork since it is 'static inline'
> >>> while being part of a complete translation unit - but I also see some
> >>> other places where that has been done.  I guess it should be just
> >>> 'static' though.  I don't feel very strongly about it.
> >>>
> >>
> >> We had a bit of discussion about this with Ilya. It seems "static
> >> inline" is a common pattern around the kernel. The coding style
> >> documentation says:
> >> "Generally, inline functions are preferable to macros resembling functions."
> >>
> >> So I think this "inline" is correct but I might be missing something.
> > 
> > Hi Adrián,
> > 
> > TL;DR: Please remove this inline keyword
> > 
> > For Kernel networking code at least it is strongly preferred not
> > to use inline in .c files unless there is a demonstrable - usually
> > performance - reason to do so. Rather, it is preferred to let the
> > compiler decide when to inline such functions. OTOH, the inline
> > keyword in .h files is fine.
> 
> FWIW, the main reason for 'inline' here is not performance, but silencing
> compiler's potential 'maybe unused' warnings:
> 
>  Function-like macros with unused parameters should be replaced by static
>  inline functions to avoid the issue of unused variables
> 
> I think, the rule for static inline functions in .c files is at odds with
> the 'Conditional Compilation' section of coding style.  The section does
> recommend to avoid conditional function declaration in .c files, but I'm not
> sure it is reasonable to export internal static functions for that reason.
> 
> In this particular case we can either define a macro, which is discouraged
> by the coding style:
> 
>  Generally, inline functions are preferable to macros resembling functions.
> 
> Or create a static inline function, that is against rule of no static
> inline functions in .c files.
> 
> Or create a simple static function and mark all the arguments as unused,
> which kind of compliant to the coding style, but the least pretty.

Hi Ilya,

I guess I would lean towards the last option.
But in any case, thanks for pointing out that this is complex:
I had not realised that when I wrote my previous email.

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Jakub Kicinski]

On Tue, 2 Jul 2024 11:53:01 +0200 Ilya Maximets wrote:
> Or create a simple static function and mark all the arguments as unused,
> which kind of compliant to the coding style, but the least pretty.

To be clear - kernel explicitly disables warnings about unused
arguments. Unused arguments are not a concern.

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Ilya Maximets]

On 7/2/24 20:06, Jakub Kicinski wrote:
> On Tue, 2 Jul 2024 11:53:01 +0200 Ilya Maximets wrote:
>> Or create a simple static function and mark all the arguments as unused,
>> which kind of compliant to the coding style, but the least pretty.
> 
> To be clear - kernel explicitly disables warnings about unused
> arguments. Unused arguments are not a concern.

OK.  Good to know.

Though I think, the '12) Macros, Enums and RTL' section of the
coding style document needs some rephrasing in that case.

Best regards, Ilya Maximets.

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Jakub Kicinski]

On Tue, 2 Jul 2024 20:16:51 +0200 Ilya Maximets wrote:
> On 7/2/24 20:06, Jakub Kicinski wrote:
> > On Tue, 2 Jul 2024 11:53:01 +0200 Ilya Maximets wrote:  
> >> Or create a simple static function and mark all the arguments as unused,
> >> which kind of compliant to the coding style, but the least pretty.  
> > 
> > To be clear - kernel explicitly disables warnings about unused
> > arguments. Unused arguments are not a concern.  
> 
> OK.  Good to know.
> 
> Though I think, the '12) Macros, Enums and RTL' section of the
> coding style document needs some rephrasing in that case.

Do you mean something like:

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 7e768c65aa92..0516b7009688 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -828,7 +828,7 @@ Generally, inline functions are preferable to macros resembling functions.
 		} while (0)
 
 Function-like macros with unused parameters should be replaced by static
-inline functions to avoid the issue of unused variables:
+inline functions to avoid the issue of unused variables in the caller:
 
 .. code-block:: c
 
?

Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Adrián Moreno]

On Tue, Jul 02, 2024 at 01:33:01PM GMT, Simon Horman wrote:
> On Tue, Jul 02, 2024 at 11:53:01AM +0200, Ilya Maximets wrote:
> > On 7/2/24 11:37, Simon Horman wrote:
> > > On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
> > >> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> > >>> Adrian Moreno <amorenoz@redhat.com> writes:
> > >
> > > ...
> > >
> > >>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > >
> > > ...
> > >
> > >>>> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> > >>>>  	return 0;
> > >>>>  }
> > >>>>
> > >>>> +#if IS_ENABLED(CONFIG_PSAMPLE)
> > >>>> +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > >>>> +			    const struct nlattr *attr)
> > >>>> +{
> > >>>> +	struct psample_group psample_group = {};
> > >>>> +	struct psample_metadata md = {};
> > >>>> +	const struct nlattr *a;
> > >>>> +	int rem;
> > >>>> +
> > >>>> +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> > >>>> +		switch (nla_type(a)) {
> > >>>> +		case OVS_PSAMPLE_ATTR_GROUP:
> > >>>> +			psample_group.group_num = nla_get_u32(a);
> > >>>> +			break;
> > >>>> +
> > >>>> +		case OVS_PSAMPLE_ATTR_COOKIE:
> > >>>> +			md.user_cookie = nla_data(a);
> > >>>> +			md.user_cookie_len = nla_len(a);
> > >>>> +			break;
> > >>>> +		}
> > >>>> +	}
> > >>>> +
> > >>>> +	psample_group.net = ovs_dp_get_net(dp);
> > >>>> +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> > >>>> +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> > >>>> +
> > >>>> +	psample_sample_packet(&psample_group, skb, 0, &md);
> > >>>> +}
> > >>>> +#else
> > >>>> +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > >>>> +				   const struct nlattr *attr) {}
> > >>>
> > >>> I noticed that this got flagged in patchwork since it is 'static inline'
> > >>> while being part of a complete translation unit - but I also see some
> > >>> other places where that has been done.  I guess it should be just
> > >>> 'static' though.  I don't feel very strongly about it.
> > >>>
> > >>
> > >> We had a bit of discussion about this with Ilya. It seems "static
> > >> inline" is a common pattern around the kernel. The coding style
> > >> documentation says:
> > >> "Generally, inline functions are preferable to macros resembling functions."
> > >>
> > >> So I think this "inline" is correct but I might be missing something.
> > >
> > > Hi Adrián,
> > >
> > > TL;DR: Please remove this inline keyword
> > >
> > > For Kernel networking code at least it is strongly preferred not
> > > to use inline in .c files unless there is a demonstrable - usually
> > > performance - reason to do so. Rather, it is preferred to let the
> > > compiler decide when to inline such functions. OTOH, the inline
> > > keyword in .h files is fine.
> >
> > FWIW, the main reason for 'inline' here is not performance, but silencing
> > compiler's potential 'maybe unused' warnings:
> >
> >  Function-like macros with unused parameters should be replaced by static
> >  inline functions to avoid the issue of unused variables
> >
> > I think, the rule for static inline functions in .c files is at odds with
> > the 'Conditional Compilation' section of coding style.  The section does
> > recommend to avoid conditional function declaration in .c files, but I'm not
> > sure it is reasonable to export internal static functions for that reason.
> >
> > In this particular case we can either define a macro, which is discouraged
> > by the coding style:
> >
> >  Generally, inline functions are preferable to macros resembling functions.
> >
> > Or create a static inline function, that is against rule of no static
> > inline functions in .c files.
> >
> > Or create a simple static function and mark all the arguments as unused,
> > which kind of compliant to the coding style, but the least pretty.
>
> Hi Ilya,
>
> I guess I would lean towards the last option.
> But in any case, thanks for pointing out that this is complex:
> I had not realised that when I wrote my previous email.
>

In that case this version (v7) should be good to go? Are there any other
comments? Or is v8 preferred (the only change is between them is
dropping the "inline")?

Thanks.
Adrián

Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

[Ilya Maximets]

On 7/2/24 20:24, Jakub Kicinski wrote:
> On Tue, 2 Jul 2024 20:16:51 +0200 Ilya Maximets wrote:
>> On 7/2/24 20:06, Jakub Kicinski wrote:
>>> On Tue, 2 Jul 2024 11:53:01 +0200 Ilya Maximets wrote:  
>>>> Or create a simple static function and mark all the arguments as unused,
>>>> which kind of compliant to the coding style, but the least pretty.  
>>>
>>> To be clear - kernel explicitly disables warnings about unused
>>> arguments. Unused arguments are not a concern.  
>>
>> OK.  Good to know.
>>
>> Though I think, the '12) Macros, Enums and RTL' section of the
>> coding style document needs some rephrasing in that case.
> 
> Do you mean something like:
> 
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 7e768c65aa92..0516b7009688 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -828,7 +828,7 @@ Generally, inline functions are preferable to macros resembling functions.
>  		} while (0)
>  
>  Function-like macros with unused parameters should be replaced by static
> -inline functions to avoid the issue of unused variables:
> +inline functions to avoid the issue of unused variables in the caller:
>  
>  .. code-block:: c
>  
> ?

Yes, that makes the logic behind the sentence way more clear.  At least
for me.  Without this change it's hard to tell if the doc is talking about
unused function arguments or unused variables in the caller.



There is another issue though that this particular phrase directly leads
a developer to declaring 'static inline' functions in .c files.  And even
'15) The inline disease' cites this use case as appropriate.  And it is,
with the exception for a macro that is a no-op stub version of some function.
Having an example on how to write a stub version of the function in both
.h and .c files might be a good addition to '21) Conditional Compilation'
section breaking the tie for this particular use case.  This section also
discourages from conditional compilation in .c files, but it doesn't seem
reasonable to export a static function outside for that reason.  I suppose
that section is mostly concerned about use of other modules.

Best regards, Ilya Maximets.