[RFC PATCH 0/4] net: openvswitch: Add sample multicasting.

[RFC PATCH 0/4] 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.

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.

** Proposal **
In this RFC, I'd like to request feedback on an attempt to fix this
situation by adding a flag to the userspace action to indicate the
upcall should be sent to a netlink multicast group instead of unicasted
to ovs-vswitchd.

This would allow for other processes to read samples directly, freeing
the netlink sockets and handler threads to process packet upcalls.

** Notes on tc-offloading **
I am aware of the efforts being made to offload the sample action with
the help of psample.
I did consider using psample to multicast the samples. However, I
found a limitation that I'd like to discuss:
I would like to support OVN-driven per-flow (IPFIX) sampling because
it allows OVN to insert two 32-bit values (obs_domain_id and
ovs_point_id) that can be used to enrich the sample with "high level"
controller metadata (see debug_drop_domain_id NBDB option in ovn-nb(5)).

The existing fields in psample_metadata are not enough to carry this
information. Would it be possible to extend this struct to make room for
some extra "application-specific" metadata?

** Alternatives **
An alternative approach that I'm considering (apart from using psample
as explained above) is to use a brand-new action. This lead to a cleaner
separation of concerns with existing userspace action (used for slow
paths and OFP_CONTROLLER actions) and cleaner statistics.
Also, ovs-vswitchd could more easily make the layout of this
new userdata part of the public API, allowing third party sample
collectors to decode it.

I am currently exploring this alternative but wanted to send the RFC to
get some early feedback, guidance or ideas.

Adrian Moreno (4):
  net:openvswitch: Support multicasting userspace ...
  openvswitch:trace: Add ovs_dp_monitor tracepoint.
  net:openvswitch: Avoid extra copy if no listeners.
  net:openvswitch: Add multicasted packets to stats

 include/uapi/linux/openvswitch.h    |  8 +++-
 net/openvswitch/actions.c           |  5 ++
 net/openvswitch/datapath.c          | 33 ++++++++++++--
 net/openvswitch/datapath.h          |  1 +
 net/openvswitch/flow_netlink.c      |  6 ++-
 net/openvswitch/openvswitch_trace.h | 71 +++++++++++++++++++++++++++++
 net/openvswitch/vport.c             |  8 ++++
 net/openvswitch/vport.h             |  1 +
 8 files changed, 125 insertions(+), 8 deletions(-)

-- 
2.44.0

[RFC PATCH 1/4] net:openvswitch: Support multicasting userspace ...

[Adrian Moreno]

actions.

Some userspace actions, such as the ones derived from OFP_CONTROLLER
action or slow path, have to be handled by ovs-vswitchd, so they are
unicasted through the netlink socket that corresponds.

However, some other userspace actions require little processing by
ovs-vswitchd and their end consumer is typically some external entity.
This is the case for IPFIX sampling which can provide very useful
observability on the OVS datapath.

Having these samples share the netlink socket and the userspace
cpu time with flow misses can easily lead to higher latency and packet
drops. This is clearly a price too high to pay for observability.

In order to allow observability applications safely consume data that
include OVN metadata, this patch makes the existing "ovs_packet"
netlink family also contain a multicast group and adds a new attribute
to the userspace action so that ovs-vswitchd can indicate that an
action must be multicasted.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/uapi/linux/openvswitch.h |  6 +++++-
 net/openvswitch/actions.c        |  5 +++++
 net/openvswitch/datapath.c       | 14 +++++++++++++-
 net/openvswitch/datapath.h       |  1 +
 net/openvswitch/flow_netlink.c   |  6 ++++--
 5 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index efc82c318fa2..77525a1c648a 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -144,6 +144,7 @@ struct ovs_vport_stats {
 /* Packet transfer. */
 
 #define OVS_PACKET_FAMILY "ovs_packet"
+#define OVS_PACKET_MCGROUP "ovs_packet"
 #define OVS_PACKET_VERSION 0x1
 
 enum ovs_packet_cmd {
@@ -678,7 +679,8 @@ struct sample_arg {
 /**
  * enum ovs_userspace_attr - Attributes for %OVS_ACTION_ATTR_USERSPACE action.
  * @OVS_USERSPACE_ATTR_PID: u32 Netlink PID to which the %OVS_PACKET_CMD_ACTION
- * message should be sent.  Required.
+ * message should be sent. If the PID is 0, the message will be sent to the
+ * "ovs_packet" netlink multicast group. Required.
  * @OVS_USERSPACE_ATTR_USERDATA: If present, its variable-length argument is
  * copied to the %OVS_PACKET_CMD_ACTION message as %OVS_PACKET_ATTR_USERDATA.
  * @OVS_USERSPACE_ATTR_EGRESS_TUN_PORT: If present, u32 output port to get
@@ -692,6 +694,8 @@ enum ovs_userspace_attr {
 	OVS_USERSPACE_ATTR_EGRESS_TUN_PORT,  /* Optional, u32 output port
 					      * to get tunnel info. */
 	OVS_USERSPACE_ATTR_ACTIONS,   /* Optional flag to get actions. */
+	OVS_USERSPACE_ATTR_MCAST,     /* Optional flag to send the packet to
+					 the "ovs_packet" multicast group. */
 	__OVS_USERSPACE_ATTR_MAX
 };
 
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 6fcd7e2ca81f..c5774613faeb 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1004,6 +1004,11 @@ static int output_userspace(struct datapath *dp, struct sk_buff *skb,
 			break;
 		}
 
+		case OVS_USERSPACE_ATTR_MCAST: {
+			upcall.portid = MCAST_PID;
+			break;
+		}
+
 		} /* End of switch. */
 	}
 
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 11c69415c605..15bad6f4b645 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -70,6 +70,10 @@ static const struct genl_multicast_group ovs_dp_vport_multicast_group = {
 	.name = OVS_VPORT_MCGROUP,
 };
 
+static const struct genl_multicast_group ovs_dp_packet_multicast_group = {
+	.name = OVS_PACKET_MCGROUP,
+};
+
 /* Check if need to build a reply message.
  * OVS userspace sets the NLM_F_ECHO flag if it needs the reply. */
 static bool ovs_must_notify(struct genl_family *family, struct genl_info *info,
@@ -577,7 +581,13 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
 
 	((struct nlmsghdr *) user_skb->data)->nlmsg_len = user_skb->len;
 
-	err = genlmsg_unicast(ovs_dp_get_net(dp), user_skb, upcall_info->portid);
+	if (upcall_info->portid == MCAST_PID)
+		err = genlmsg_multicast_netns(&dp_packet_genl_family,
+			ovs_dp_get_net(dp), user_skb, 0, 0, GFP_KERNEL);
+	else
+		err = genlmsg_unicast(ovs_dp_get_net(dp),
+				      user_skb, upcall_info->portid);
+
 	user_skb = NULL;
 out:
 	if (err)
@@ -717,6 +727,8 @@ static struct genl_family dp_packet_genl_family __ro_after_init = {
 	.small_ops = dp_packet_genl_ops,
 	.n_small_ops = ARRAY_SIZE(dp_packet_genl_ops),
 	.resv_start_op = OVS_PACKET_CMD_EXECUTE + 1,
+	.mcgrps = &ovs_dp_packet_multicast_group,
+	.n_mcgrps = 1,
 	.module = THIS_MODULE,
 };
 
diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
index 0cd29971a907..d0b1b8afafbb 100644
--- a/net/openvswitch/datapath.h
+++ b/net/openvswitch/datapath.h
@@ -124,6 +124,7 @@ struct ovs_skb_cb {
 };
 #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
 
+#define MCAST_PID 0xFFFFFFFF
 /**
  * struct dp_upcall - metadata to include with a packet to send to userspace
  * @cmd: One of %OVS_PACKET_CMD_*.
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index ebc5728aab4e..4c95fa1aa15d 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -3043,6 +3043,8 @@ static int validate_userspace(const struct nlattr *attr)
 		[OVS_USERSPACE_ATTR_PID] = {.type = NLA_U32 },
 		[OVS_USERSPACE_ATTR_USERDATA] = {.type = NLA_UNSPEC },
 		[OVS_USERSPACE_ATTR_EGRESS_TUN_PORT] = {.type = NLA_U32 },
+		[OVS_USERSPACE_ATTR_ACTIONS] = {.type = NLA_FLAG },
+		[OVS_USERSPACE_ATTR_MCAST] = {.type = NLA_FLAG },
 	};
 	struct nlattr *a[OVS_USERSPACE_ATTR_MAX + 1];
 	int error;
@@ -3052,8 +3054,8 @@ static int validate_userspace(const struct nlattr *attr)
 	if (error)
 		return error;
 
-	if (!a[OVS_USERSPACE_ATTR_PID] ||
-	    !nla_get_u32(a[OVS_USERSPACE_ATTR_PID]))
+	if (!a[OVS_USERSPACE_ATTR_MCAST] && (!a[OVS_USERSPACE_ATTR_PID] ||
+	    !nla_get_u32(a[OVS_USERSPACE_ATTR_PID])))
 		return -EINVAL;
 
 	return 0;
-- 
2.44.0

[RFC PATCH 2/4] openvswitch:trace: Add ovs_dp_monitor tracepoint.

[Adrian Moreno]

The existing dp_upcall tracepoint was intented to provide visibility on
flow-misses (what we typically refer as upcalls). It's used to measure
things like upcall latency.

However, if a monitoring userspace action (such as IPFIX) is
multicasted, using the same tracepoint will only add confusion as
ovs-vswithcd will not receive this upcall.

In order to make things clearer, create a new tracepoint called
"ovs_dp_monitor" and use it instead of the existing one for multicasted
packets.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 net/openvswitch/datapath.c          |  6 ++-
 net/openvswitch/openvswitch_trace.h | 71 +++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 15bad6f4b645..5a2c0b3b4112 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -316,12 +316,16 @@ int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb,
 		  const struct dp_upcall_info *upcall_info,
 		  uint32_t cutlen)
 {
+	const bool mcast = upcall_info->portid == MCAST_PID;
 	struct dp_stats_percpu *stats;
 	int err;
 
-	if (trace_ovs_dp_upcall_enabled())
+	if (!mcast && trace_ovs_dp_upcall_enabled())
 		trace_ovs_dp_upcall(dp, skb, key, upcall_info);
 
+	if (mcast && trace_ovs_dp_monitor_enabled())
+		trace_ovs_dp_monitor(dp, skb, key, upcall_info);
+
 	if (upcall_info->portid == 0) {
 		err = -ENOTCONN;
 		goto err;
diff --git a/net/openvswitch/openvswitch_trace.h b/net/openvswitch/openvswitch_trace.h
index 3eb35d9eb700..76e9612e3555 100644
--- a/net/openvswitch/openvswitch_trace.h
+++ b/net/openvswitch/openvswitch_trace.h
@@ -148,6 +148,77 @@ TRACE_EVENT(ovs_dp_upcall,
 		  __entry->upcall_mru)
 );
 
+TRACE_EVENT(ovs_dp_monitor,
+
+	TP_PROTO(struct datapath *dp, struct sk_buff *skb,
+		 const struct sw_flow_key *key,
+		 const struct dp_upcall_info *upcall_info),
+
+	TP_ARGS(dp, skb, key, upcall_info),
+
+	TP_STRUCT__entry(
+		__field(	void *,		dpaddr			)
+		__string(	dp_name,	ovs_dp_name(dp)		)
+		__string(	dev_name,	skb->dev->name		)
+		__field(	void *,		skbaddr			)
+		__field(	unsigned int,	len			)
+		__field(	unsigned int,	data_len		)
+		__field(	unsigned int,	truesize		)
+		__field(	u8,		nr_frags		)
+		__field(	u16,		gso_size		)
+		__field(	u16,		gso_type		)
+		__field(	u32,		ovs_flow_hash		)
+		__field(	u32,		recirc_id		)
+		__field(	const void *,	keyaddr			)
+		__field(	u16,		key_eth_type		)
+		__field(	u8,		key_ct_state		)
+		__field(	u8,		key_ct_orig_proto	)
+		__field(	u16,		key_ct_zone		)
+		__field(	unsigned int,	flow_key_valid		)
+		__field(	u32,		upcall_port		)
+		__field(	void *,		upcall_udata		)
+		__field(	u16,		upcall_ulen		)
+	),
+
+	TP_fast_assign(
+		__entry->dpaddr = dp;
+		__assign_str(dp_name, ovs_dp_name(dp));
+		__assign_str(dev_name, skb->dev->name);
+		__entry->skbaddr = skb;
+		__entry->len = skb->len;
+		__entry->data_len = skb->data_len;
+		__entry->truesize = skb->truesize;
+		__entry->nr_frags = skb_shinfo(skb)->nr_frags;
+		__entry->gso_size = skb_shinfo(skb)->gso_size;
+		__entry->gso_type = skb_shinfo(skb)->gso_type;
+		__entry->ovs_flow_hash = key->ovs_flow_hash;
+		__entry->recirc_id = key->recirc_id;
+		__entry->keyaddr = key;
+		__entry->key_eth_type = key->eth.type;
+		__entry->key_ct_state = key->ct_state;
+		__entry->key_ct_orig_proto = key->ct_orig_proto;
+		__entry->key_ct_zone = key->ct_zone;
+		__entry->flow_key_valid =  !(key->mac_proto & SW_FLOW_KEY_INVALID);
+		__entry->upcall_port = upcall_info->portid;
+		__entry->upcall_udata = upcall_info->userdata ?
+			nla_data(upcall_info->userdata): NULL;
+		__entry->upcall_ulen = upcall_info->userdata ?
+			nla_len(upcall_info->userdata): 0;
+	),
+
+	TP_printk("dpaddr=%p dp_name=%s dev=%s skbaddr=%p len=%u data_len=%u truesize=%u nr_frags=%d gso_size=%d gso_type=%#x ovs_flow_hash=0x%08x recirc_id=0x%08x keyaddr=%p eth_type=0x%04x ct_state=%02x ct_orig_proto=%02x ct_zone=%04x flow_key_valid=%d upcall_port=%u upcall_udata=%p upcall_ulen=%d",
+		  __entry->dpaddr, __get_str(dp_name), __get_str(dev_name),
+		  __entry->skbaddr, __entry->len, __entry->data_len,
+		  __entry->truesize, __entry->nr_frags, __entry->gso_size,
+		  __entry->gso_type, __entry->ovs_flow_hash,
+		  __entry->recirc_id, __entry->keyaddr, __entry->key_eth_type,
+		  __entry->key_ct_state, __entry->key_ct_orig_proto,
+		  __entry->key_ct_zone,
+		  __entry->flow_key_valid,
+		  __entry->upcall_port,
+		  __entry->upcall_udata, __entry->upcall_ulen)
+);
+
 #endif /* _TRACE_OPENVSWITCH_H */
 
 /* This part must be outside protection */
-- 
2.44.0

[RFC PATCH 3/4] net:openvswitch: Avoid extra copy if no listeners.

[Adrian Moreno]

If there are no listeneres in the multicast group, there is no need for
building the upcall packet. Exit early in that case.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 net/openvswitch/datapath.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 5a2c0b3b4112..5171aefa6a7c 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -455,6 +455,10 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
 	if (!dp_ifindex)
 		return -ENODEV;
 
+	if (upcall_info->portid == MCAST_PID &&
+	    !genl_has_listeners(&dp_packet_genl_family, ovs_dp_get_net(dp), 0))
+		return 0;
+
 	if (skb_vlan_tag_present(skb)) {
 		nskb = skb_clone(skb, GFP_ATOMIC);
 		if (!nskb)
-- 
2.44.0

[RFC PATCH 4/4] net:openvswitch: Add multicasted packets to stats

[Adrian Moreno]

If we mix multicasted and unicasted statistics, there could be a serious
discrepancy between the stats reported by the kernel and the ones read
by userspace, leading to increased confusion.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/uapi/linux/openvswitch.h | 2 ++
 net/openvswitch/datapath.c       | 9 ++++++---
 net/openvswitch/vport.c          | 8 ++++++++
 net/openvswitch/vport.h          | 1 +
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 77525a1c648a..25e35b627fe5 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -288,10 +288,12 @@ enum ovs_vport_attr {
  * enum ovs_vport_upcall_attr - attributes for %OVS_VPORT_UPCALL* commands
  * @OVS_VPORT_UPCALL_SUCCESS: 64-bit upcall success packets.
  * @OVS_VPORT_UPCALL_FAIL: 64-bit upcall fail packets.
+ * @OVS_VPORT_UPCALL_MCAST: 64-bit multicasted upcall packets.
  */
 enum ovs_vport_upcall_attr {
 	OVS_VPORT_UPCALL_ATTR_SUCCESS,
 	OVS_VPORT_UPCALL_ATTR_FAIL,
+	OVS_VPORT_UPCALL_ATTR_MCAST,
 	__OVS_VPORT_UPCALL_ATTR_MAX
 };
 
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 5171aefa6a7c..a457a07adb52 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -217,7 +217,7 @@ static struct vport *new_vport(const struct vport_parms *parms)
 
 static void ovs_vport_update_upcall_stats(struct sk_buff *skb,
 					  const struct dp_upcall_info *upcall_info,
-					  bool upcall_result)
+					  bool mcast, bool upcall_result)
 {
 	struct vport *p = OVS_CB(skb)->input_vport;
 	struct vport_upcall_stats_percpu *stats;
@@ -229,7 +229,10 @@ static void ovs_vport_update_upcall_stats(struct sk_buff *skb,
 	stats = this_cpu_ptr(p->upcall_stats);
 	u64_stats_update_begin(&stats->syncp);
 	if (upcall_result)
-		u64_stats_inc(&stats->n_success);
+		if (mcast)
+			u64_stats_inc(&stats->n_mcast);
+		else
+			u64_stats_inc(&stats->n_success);
 	else
 		u64_stats_inc(&stats->n_fail);
 	u64_stats_update_end(&stats->syncp);
@@ -336,7 +339,7 @@ int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb,
 	else
 		err = queue_gso_packets(dp, skb, key, upcall_info, cutlen);
 
-	ovs_vport_update_upcall_stats(skb, upcall_info, !err);
+	ovs_vport_update_upcall_stats(skb, upcall_info, mcast, !err);
 	if (err)
 		goto err;
 
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 972ae01a70f7..b78287e443d1 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -315,6 +315,7 @@ int ovs_vport_get_upcall_stats(struct vport *vport, struct sk_buff *skb)
 
 	__u64 tx_success = 0;
 	__u64 tx_fail = 0;
+	__u64 mcast = 0;
 
 	for_each_possible_cpu(i) {
 		const struct vport_upcall_stats_percpu *stats;
@@ -325,6 +326,7 @@ int ovs_vport_get_upcall_stats(struct vport *vport, struct sk_buff *skb)
 			start = u64_stats_fetch_begin(&stats->syncp);
 			tx_success += u64_stats_read(&stats->n_success);
 			tx_fail += u64_stats_read(&stats->n_fail);
+			mcast  += u64_stats_read(&stats->n_mcast);
 		} while (u64_stats_fetch_retry(&stats->syncp, start));
 	}
 
@@ -343,6 +345,12 @@ int ovs_vport_get_upcall_stats(struct vport *vport, struct sk_buff *skb)
 		nla_nest_cancel(skb, nla);
 		return -EMSGSIZE;
 	}
+
+	if (nla_put_u64_64bit(skb, OVS_VPORT_UPCALL_ATTR_MCAST, mcast,
+			      OVS_VPORT_ATTR_PAD)) {
+		nla_nest_cancel(skb, nla);
+		return -EMSGSIZE;
+	}
 	nla_nest_end(skb, nla);
 
 	return 0;
diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
index 3e71ca8ad8a7..e9817b2b3b61 100644
--- a/net/openvswitch/vport.h
+++ b/net/openvswitch/vport.h
@@ -151,6 +151,7 @@ struct vport_upcall_stats_percpu {
 	struct u64_stats_sync syncp;
 	u64_stats_t n_success;
 	u64_stats_t n_fail;
+	u64_stats_t n_mcast;
 };
 
 struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *,
-- 
2.44.0

Re: [RFC PATCH 0/4] net: openvswitch: Add sample multicasting.

[Ilya Maximets]

On 3/7/24 16:18, Adrian Moreno wrote:
> ** 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.
> 
> 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.
> 
> ** Proposal **
> In this RFC, I'd like to request feedback on an attempt to fix this
> situation by adding a flag to the userspace action to indicate the
> upcall should be sent to a netlink multicast group instead of unicasted
> to ovs-vswitchd.
> 
> This would allow for other processes to read samples directly, freeing
> the netlink sockets and handler threads to process packet upcalls.
> 
> ** Notes on tc-offloading **
> I am aware of the efforts being made to offload the sample action with
> the help of psample.
> I did consider using psample to multicast the samples. However, I
> found a limitation that I'd like to discuss:
> I would like to support OVN-driven per-flow (IPFIX) sampling because
> it allows OVN to insert two 32-bit values (obs_domain_id and
> ovs_point_id) that can be used to enrich the sample with "high level"
> controller metadata (see debug_drop_domain_id NBDB option in ovn-nb(5)).
> 
> The existing fields in psample_metadata are not enough to carry this
> information. Would it be possible to extend this struct to make room for
> some extra "application-specific" metadata?
> 
> ** Alternatives **
> An alternative approach that I'm considering (apart from using psample
> as explained above) is to use a brand-new action. This lead to a cleaner
> separation of concerns with existing userspace action (used for slow
> paths and OFP_CONTROLLER actions) and cleaner statistics.
> Also, ovs-vswitchd could more easily make the layout of this
> new userdata part of the public API, allowing third party sample
> collectors to decode it.
> 
> I am currently exploring this alternative but wanted to send the RFC to
> get some early feedback, guidance or ideas.


Hi, Adrian.  Thanks for the patches!

Though I'm not sure if broadcasting is generally the best approach.
These messages contain opaque information that is not actually
parsable by any other entity than a process that created the action.
And I don't think the structure of these opaque fields should become
part of uAPI in neither kernel nor OVS in userspace.

The userspace() action already has a OVS_USERSPACE_ATTR_PID argument.
And it is not actually used when OVS_DP_F_DISPATCH_UPCALL_PER_CPU is
enabled.  All known users of OVS_DP_F_DISPATCH_UPCALL_PER_CPU are
setting the OVS_USERSPACE_ATTR_PID to UINT32_MAX, which is not a pid
that kernel could generate.

So, with a minimal and pretty much backward compatible change in
output_userspace() function, we can honor OVS_USERSPACE_ATTR_PID if
it's not U32_MAX.  This way userspace process can open a separate
socket and configure sampling to redirect all packets there while
normal MISS upcalls would still arrive to per-cpu sockets.  This
should cover the performance concern.

For the case without per-cpu dispatch, the feature comes for free
if userspace application wants to use it.  However, there is no
currently supported version of OVS that doesn't use per-cpu dispatch
when available.

What do you think?

Best regards, Ilya Maximets.

Re: [RFC PATCH 0/4] net: openvswitch: Add sample multicasting.

[Adrian Moreno]

On 3/7/24 17:54, Ilya Maximets wrote:
> On 3/7/24 16:18, Adrian Moreno wrote:
>> ** 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.
>>
>> 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.
>>
>> ** Proposal **
>> In this RFC, I'd like to request feedback on an attempt to fix this
>> situation by adding a flag to the userspace action to indicate the
>> upcall should be sent to a netlink multicast group instead of unicasted
>> to ovs-vswitchd.
>>
>> This would allow for other processes to read samples directly, freeing
>> the netlink sockets and handler threads to process packet upcalls.
>>
>> ** Notes on tc-offloading **
>> I am aware of the efforts being made to offload the sample action with
>> the help of psample.
>> I did consider using psample to multicast the samples. However, I
>> found a limitation that I'd like to discuss:
>> I would like to support OVN-driven per-flow (IPFIX) sampling because
>> it allows OVN to insert two 32-bit values (obs_domain_id and
>> ovs_point_id) that can be used to enrich the sample with "high level"
>> controller metadata (see debug_drop_domain_id NBDB option in ovn-nb(5)).
>>
>> The existing fields in psample_metadata are not enough to carry this
>> information. Would it be possible to extend this struct to make room for
>> some extra "application-specific" metadata?
>>
>> ** Alternatives **
>> An alternative approach that I'm considering (apart from using psample
>> as explained above) is to use a brand-new action. This lead to a cleaner
>> separation of concerns with existing userspace action (used for slow
>> paths and OFP_CONTROLLER actions) and cleaner statistics.
>> Also, ovs-vswitchd could more easily make the layout of this
>> new userdata part of the public API, allowing third party sample
>> collectors to decode it.
>>
>> I am currently exploring this alternative but wanted to send the RFC to
>> get some early feedback, guidance or ideas.
> 
> 
> Hi, Adrian.  Thanks for the patches!
> 

Thanks for the quick feedback.
Also adding Dumitru who I missed to include in the original CC list.

> Though I'm not sure if broadcasting is generally the best approach.
> These messages contain opaque information that is not actually
> parsable by any other entity than a process that created the action.
> And I don't think the structure of these opaque fields should become
> part of uAPI in neither kernel nor OVS in userspace.
> 

I understand this can be cumbersome, specially given the opaque field is 
currently also used for some purely-internal OVS actions (e.g: CONTROLLER).

However, for features such as OVN-driven per-flow sampling, where OVN-generated 
identifiers are placed in obs_domain_id and obs_point_id, it would be _really_ 
useful if this opaque value could be somehow decoded by external programs.

Two ideas come to mind to try to alleviate the potential maintainability issues:
- As I suggested, using a new action maybe makes things easier. By splitting the 
current "user_action_cookie" in two, one for internal actions and one for 
"observability" actions, we could expose the latter in the OVS userspace API 
without having to expose the former.
- Exposing functions in OVS that decode the opaque value. Third party 
applications could link against, say, libopenvswitch.so and use it to extract 
obs_{domain,point}_ids.

What do you think?

> The userspace() action already has a OVS_USERSPACE_ATTR_PID argument.
> And it is not actually used when OVS_DP_F_DISPATCH_UPCALL_PER_CPU is
> enabled.  All known users of OVS_DP_F_DISPATCH_UPCALL_PER_CPU are
> setting the OVS_USERSPACE_ATTR_PID to UINT32_MAX, which is not a pid
> that kernel could generate.
> 
> So, with a minimal and pretty much backward compatible change in
> output_userspace() function, we can honor OVS_USERSPACE_ATTR_PID if
> it's not U32_MAX.  This way userspace process can open a separate
> socket and configure sampling to redirect all packets there while
> normal MISS upcalls would still arrive to per-cpu sockets.  This
> should cover the performance concern.
>

Do you mean creating a new thread to process samples or using handlers?
The latter would still have performance impact and the former would likely fail 
to process all samples in a timely manner if there are many.

Besides, the current userspace tc-offloading series uses netlink broadcast with 
psample, can't we do the same for non-offloaded actions? It enable building 
external observability applications without overloading OVS.


> For the case without per-cpu dispatch, the feature comes for free
> if userspace application wants to use it.  However, there is no
> currently supported version of OVS that doesn't use per-cpu dispatch
> when available.
>  > What do you think?
>  > Best regards, Ilya Maximets.
> 

-- 
Adrián Moreno

Re: [RFC PATCH 0/4] net: openvswitch: Add sample multicasting.

[Ilya Maximets]

On 3/7/24 21:59, Adrian Moreno wrote:
> 
> 
> On 3/7/24 17:54, Ilya Maximets wrote:
>> On 3/7/24 16:18, Adrian Moreno wrote:
>>> ** 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.
>>>
>>> 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.
>>>
>>> ** Proposal **
>>> In this RFC, I'd like to request feedback on an attempt to fix this
>>> situation by adding a flag to the userspace action to indicate the
>>> upcall should be sent to a netlink multicast group instead of unicasted
>>> to ovs-vswitchd.
>>>
>>> This would allow for other processes to read samples directly, freeing
>>> the netlink sockets and handler threads to process packet upcalls.
>>>
>>> ** Notes on tc-offloading **
>>> I am aware of the efforts being made to offload the sample action with
>>> the help of psample.
>>> I did consider using psample to multicast the samples. However, I
>>> found a limitation that I'd like to discuss:
>>> I would like to support OVN-driven per-flow (IPFIX) sampling because
>>> it allows OVN to insert two 32-bit values (obs_domain_id and
>>> ovs_point_id) that can be used to enrich the sample with "high level"
>>> controller metadata (see debug_drop_domain_id NBDB option in ovn-nb(5)).
>>>
>>> The existing fields in psample_metadata are not enough to carry this
>>> information. Would it be possible to extend this struct to make room for
>>> some extra "application-specific" metadata?
>>>
>>> ** Alternatives **
>>> An alternative approach that I'm considering (apart from using psample
>>> as explained above) is to use a brand-new action. This lead to a cleaner
>>> separation of concerns with existing userspace action (used for slow
>>> paths and OFP_CONTROLLER actions) and cleaner statistics.
>>> Also, ovs-vswitchd could more easily make the layout of this
>>> new userdata part of the public API, allowing third party sample
>>> collectors to decode it.
>>>
>>> I am currently exploring this alternative but wanted to send the RFC to
>>> get some early feedback, guidance or ideas.
>>
>>
>> Hi, Adrian.  Thanks for the patches!
>>
> 
> Thanks for the quick feedback.
> Also adding Dumitru who I missed to include in the original CC list.
> 
>> Though I'm not sure if broadcasting is generally the best approach.
>> These messages contain opaque information that is not actually
>> parsable by any other entity than a process that created the action.
>> And I don't think the structure of these opaque fields should become
>> part of uAPI in neither kernel nor OVS in userspace.
>>
> 
> I understand this can be cumbersome, specially given the opaque field is 
> currently also used for some purely-internal OVS actions (e.g: CONTROLLER).
> 
> However, for features such as OVN-driven per-flow sampling, where OVN-generated 
> identifiers are placed in obs_domain_id and obs_point_id, it would be _really_ 
> useful if this opaque value could be somehow decoded by external programs.
> 
> Two ideas come to mind to try to alleviate the potential maintainability issues:
> - As I suggested, using a new action maybe makes things easier. By splitting the 
> current "user_action_cookie" in two, one for internal actions and one for 
> "observability" actions, we could expose the latter in the OVS userspace API 
> without having to expose the former.
> - Exposing functions in OVS that decode the opaque value. Third party 
> applications could link against, say, libopenvswitch.so and use it to extract 
> obs_{domain,point}_ids.

Linking with OVS libraries is practically the same as just exposing
the internal structure, because once the external application is
running it either must have the same library version as the process
that installs the action, or it may not be able to parse the message.

Any form of exposing to an external application will freeze the
opaque arguments and effectively make them a form of uAPI.

The separate action with a defined uAPI solves this problem by just
creating a new uAPI, but I'm not sure why it is needed.

> 
> What do you think?
> 
>> The userspace() action already has a OVS_USERSPACE_ATTR_PID argument.
>> And it is not actually used when OVS_DP_F_DISPATCH_UPCALL_PER_CPU is
>> enabled.  All known users of OVS_DP_F_DISPATCH_UPCALL_PER_CPU are
>> setting the OVS_USERSPACE_ATTR_PID to UINT32_MAX, which is not a pid
>> that kernel could generate.
>>
>> So, with a minimal and pretty much backward compatible change in
>> output_userspace() function, we can honor OVS_USERSPACE_ATTR_PID if
>> it's not U32_MAX.  This way userspace process can open a separate
>> socket and configure sampling to redirect all packets there while
>> normal MISS upcalls would still arrive to per-cpu sockets.  This
>> should cover the performance concern.
>>
> 
> Do you mean creating a new thread to process samples or using handlers?
> The latter would still have performance impact and the former would likely fail 
> to process all samples in a timely manner if there are many.
> 
> Besides, the current userspace tc-offloading series uses netlink broadcast with 
> psample, can't we do the same for non-offloaded actions? It enable building 
> external observability applications without overloading OVS.

Creating a separate thread solves the performance issue.  But you can
also write a separate application that would communicate its PID to the
running OVS daemon.  Let's say the same application that configures
sampling in the OVS database can also write a PID there.

The thing is that existence of external application immediately breaks
opacity of the arguments and forces us to define uAPI.  However, if
there is an explicit communication between that application and OVS
userpsace daemon, then we can establish a contract (structure of opaque
values) between these two userspace applications without defining that
contract in the kernel uAPI.  But if we're going with multicast, that
anyone can subscribe to, then we have to define that contract in the
kernel uAPI.

Also, in order for this observability to work with userspace datapath
we'll have to implement userspace-to-userspace netlink multicast (does
that even exist?).  Running the sample collection within OVS as a thread
will be much less painful.

One other thing worth mentioning is that the PID approach I suggested
is just a minor tweak of what is already supported in the kernel.  It
doesn't prohibit introduction of a new action or a multicast group in
the future.  While premature uAPI definition may end up with another
action that nobody uses.  It can be added later if end up being
actually necessary.

Best regards, Ilya Maximets.

> 
> 
>> For the case without per-cpu dispatch, the feature comes for free
>> if userspace application wants to use it.  However, there is no
>> currently supported version of OVS that doesn't use per-cpu dispatch
>> when available.
>>  > What do you think?
>>  > Best regards, Ilya Maximets.
>>
> 

Re: [RFC PATCH 0/4] net: openvswitch: Add sample multicasting.

[Ilya Maximets]

On 3/7/24 22:29, Ilya Maximets wrote:
> On 3/7/24 21:59, Adrian Moreno wrote:
>> 
>> 
>> On 3/7/24 17:54, Ilya Maximets wrote:
>>> On 3/7/24 16:18, Adrian Moreno wrote:
>>>> ** 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.
>>>> 
>>>> 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.
>>>> 
>>>> ** Proposal ** In this RFC, I'd like to request feedback on an 
>>>> attempt to fix this situation by adding a flag to the userspace
>>>> action to indicate the upcall should be sent to a netlink
>>>> multicast group instead of unicasted to ovs-vswitchd.
>>>> 
>>>> This would allow for other processes to read samples directly, 
>>>> freeing the netlink sockets and handler threads to process 
>>>> packet upcalls.
>>>> 
>>>> ** Notes on tc-offloading ** I am aware of the efforts being 
>>>> made to offload the sample action with the help of psample. I 
>>>> did consider using psample to multicast the samples. However, I
>>>> found a limitation that I'd like to discuss: I would like to 
>>>> support OVN-driven per-flow (IPFIX) sampling because it allows 
>>>> OVN to insert two 32-bit values (obs_domain_id and 
>>>> ovs_point_id) that can be used to enrich the sample with "high 
>>>> level" controller metadata (see debug_drop_domain_id NBDB 
>>>> option in ovn-nb(5)).
>>>> 
>>>> The existing fields in psample_metadata are not enough to
>>>> carry this information. Would it be possible to extend this
>>>> struct to make room for some extra "application-specific"
>>>> metadata?
>>>> 
>>>> ** Alternatives ** An alternative approach that I'm
>>>> considering (apart from using psample as explained above) is to
>>>> use a brand-new action. This lead to a cleaner separation of
>>>> concerns with existing userspace action (used for slow paths
>>>> and OFP_CONTROLLER actions) and cleaner statistics. Also, 
>>>> ovs-vswitchd could more easily make the layout of this new 
>>>> userdata part of the public API, allowing third party sample 
>>>> collectors to decode it.
>>>> 
>>>> I am currently exploring this alternative but wanted to send 
>>>> the RFC to get some early feedback, guidance or ideas.
>>> 
>>> 
>>> Hi, Adrian.  Thanks for the patches!
>>> 
>> 
>> Thanks for the quick feedback. Also adding Dumitru who I missed to 
>> include in the original CC list.
>> 
>>> Though I'm not sure if broadcasting is generally the best 
>>> approach. These messages contain opaque information that is not 
>>> actually parsable by any other entity than a process that
>>> created the action. And I don't think the structure of these
>>> opaque fields should become part of uAPI in neither kernel nor
>>> OVS in userspace.
>>> 
>> 
>> I understand this can be cumbersome, specially given the opaque 
>> field is currently also used for some purely-internal OVS actions 
>> (e.g: CONTROLLER).
>> 
>> However, for features such as OVN-driven per-flow sampling, where 
>> OVN-generated identifiers are placed in obs_domain_id and 
>> obs_point_id, it would be _really_ useful if this opaque value 
>> could be somehow decoded by external programs.
>> 
>> Two ideas come to mind to try to alleviate the potential 
>> maintainability issues: - As I suggested, using a new action maybe 
>> makes things easier. By splitting the current "user_action_cookie" 
>> in two, one for internal actions and one for "observability" 
>> actions, we could expose the latter in the OVS userspace API 
>> without having to expose the former. - Exposing functions in OVS 
>> that decode the opaque value. Third party applications could link 
>> against, say, libopenvswitch.so and use it to extract 
>> obs_{domain,point}_ids.
> 
> Linking with OVS libraries is practically the same as just exposing 
> the internal structure, because once the external application is 
> running it either must have the same library version as the process 
> that installs the action, or it may not be able to parse the 
> message.
> 
> Any form of exposing to an external application will freeze the 
> opaque arguments and effectively make them a form of uAPI.
> 
> The separate action with a defined uAPI solves this problem by just 
> creating a new uAPI, but I'm not sure why it is needed.
> 
>> 
>> What do you think?
>> 
>>> The userspace() action already has a OVS_USERSPACE_ATTR_PID 
>>> argument. And it is not actually used when 
>>> OVS_DP_F_DISPATCH_UPCALL_PER_CPU is enabled.  All known users of 
>>> OVS_DP_F_DISPATCH_UPCALL_PER_CPU are setting the 
>>> OVS_USERSPACE_ATTR_PID to UINT32_MAX, which is not a pid that 
>>> kernel could generate.
>>> 
>>> So, with a minimal and pretty much backward compatible change in
>>>  output_userspace() function, we can honor
>>> OVS_USERSPACE_ATTR_PID if it's not U32_MAX.  This way userspace
>>> process can open a separate socket and configure sampling to
>>> redirect all packets there while normal MISS upcalls would still
>>> arrive to per-cpu sockets.  This should cover the performance
>>> concern.
>>> 
>> 
>> Do you mean creating a new thread to process samples or using 
>> handlers? The latter would still have performance impact and the 
>> former would likely fail to process all samples in a timely manner 
>> if there are many.
>> 
>> Besides, the current userspace tc-offloading series uses netlink 
>> broadcast with psample, can't we do the same for non-offloaded 
>> actions? It enable building external observability applications 
>> without overloading OVS.
> 
> Creating a separate thread solves the performance issue.  But you can
> also write a separate application that would communicate its PID to
> the running OVS daemon.  Let's say the same application that 
> configures sampling in the OVS database can also write a PID there.
> 
> The thing is that existence of external application immediately 
> breaks opacity of the arguments and forces us to define uAPI. 
> However, if there is an explicit communication between that 
> application and OVS userpsace daemon, then we can establish a 
> contract (structure of opaque values) between these two userspace 
> applications without defining that contract in the kernel uAPI.  But 
> if we're going with multicast, that anyone can subscribe to, then we 
> have to define that contract in the kernel uAPI.
> 
> Also, in order for this observability to work with userspace datapath
> we'll have to implement userspace-to-userspace netlink multicast
> (does that even exist?).  Running the sample collection within OVS as
> a thread will be much less painful.
> 
> One other thing worth mentioning is that the PID approach I suggested
> is just a minor tweak of what is already supported in the kernel.  It
> doesn't prohibit introduction of a new action or a multicast group in
> the future.  While premature uAPI definition may end up with another
> action that nobody uses.  It can be added later if end up being
> actually necessary.

Thinking more about this problem, it seems to make some sense to have
a way to ask OVS for sampling that multiple observers can subscribe to.
Unicast socket will not allow such functionality.  However, I still don't
think creation of a new multicast group for that purpose is justified.
Kernel already has a generic sampling mechanism (psample) with a multicast
group created specifically for a very similar purpose.  So, instead of
re-inventing it, we can add a small modification to the OVS'es sampling
action allowing it to sample to psample instead of OVS'es own unicast
sockets.  This can be achieved by adding a new OVS_SAMPLE_ATTR that
would tell to direct packets to psample instead of executing actions.
Or adding a new OVS_USERSPACE_ATTR that would do the same thing but from
a userspace() action instead, i.e. direct packets to psample instead of
OVS'es own sockets copying OVS_PACKET_ATTR_USERDATA into the
PSAMPLE_ATTR_SAMPLE_GROUP.  Might be cleaner this way, not sure.

Form a perspective of an OVS userspace daemon, this functionality can
be clearly exposed as a separate sampling mechanism alongside IPFIX,
sFlow and NetFlow.

I see you eluded to this approach in the original cover letter above.
So, I'd vote for it instead.  Hopefully the psample API can be extended
to be more flexible and allow larger userdata to be passed in.  Maybe have
SAMPLE_SUBGROUP in addition to the existing PSAMPLE_ATTR_SAMPLE_GROUP ?
OTOH, it is not really IPFIX, it's a different interface, so it might have
different requirements.  In any case OVS may check that userdata fits
into psample arguments and reject flows that are not compatible.

> 
> Best regards, Ilya Maximets.
> 
>> 
>> 
>>> For the case without per-cpu dispatch, the feature comes for free
>>> if userspace application wants to use it.  However, there is no
>>> currently supported version of OVS that doesn't use per-cpu 
>>> dispatch when available.
>>>> What do you think? Best regards, Ilya Maximets.
>>> 
>> 
> 

Re: [RFC PATCH 0/4] net: openvswitch: Add sample multicasting.

[Adrian Moreno]

On 3/8/24 15:24, Ilya Maximets wrote:
> On 3/7/24 22:29, Ilya Maximets wrote:
>> On 3/7/24 21:59, Adrian Moreno wrote:
>>>
>>>
>>> On 3/7/24 17:54, Ilya Maximets wrote:
>>>> On 3/7/24 16:18, Adrian Moreno wrote:
>>>>> ** 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> ** Proposal ** In this RFC, I'd like to request feedback on an
>>>>> attempt to fix this situation by adding a flag to the userspace
>>>>> action to indicate the upcall should be sent to a netlink
>>>>> multicast group instead of unicasted to ovs-vswitchd.
>>>>>
>>>>> This would allow for other processes to read samples directly,
>>>>> freeing the netlink sockets and handler threads to process
>>>>> packet upcalls.
>>>>>
>>>>> ** Notes on tc-offloading ** I am aware of the efforts being
>>>>> made to offload the sample action with the help of psample. I
>>>>> did consider using psample to multicast the samples. However, I
>>>>> found a limitation that I'd like to discuss: I would like to
>>>>> support OVN-driven per-flow (IPFIX) sampling because it allows
>>>>> OVN to insert two 32-bit values (obs_domain_id and
>>>>> ovs_point_id) that can be used to enrich the sample with "high
>>>>> level" controller metadata (see debug_drop_domain_id NBDB
>>>>> option in ovn-nb(5)).
>>>>>
>>>>> The existing fields in psample_metadata are not enough to
>>>>> carry this information. Would it be possible to extend this
>>>>> struct to make room for some extra "application-specific"
>>>>> metadata?
>>>>>
>>>>> ** Alternatives ** An alternative approach that I'm
>>>>> considering (apart from using psample as explained above) is to
>>>>> use a brand-new action. This lead to a cleaner separation of
>>>>> concerns with existing userspace action (used for slow paths
>>>>> and OFP_CONTROLLER actions) and cleaner statistics. Also,
>>>>> ovs-vswitchd could more easily make the layout of this new
>>>>> userdata part of the public API, allowing third party sample
>>>>> collectors to decode it.
>>>>>
>>>>> I am currently exploring this alternative but wanted to send
>>>>> the RFC to get some early feedback, guidance or ideas.
>>>>
>>>>
>>>> Hi, Adrian.  Thanks for the patches!
>>>>
>>>
>>> Thanks for the quick feedback. Also adding Dumitru who I missed to
>>> include in the original CC list.
>>>
>>>> Though I'm not sure if broadcasting is generally the best
>>>> approach. These messages contain opaque information that is not
>>>> actually parsable by any other entity than a process that
>>>> created the action. And I don't think the structure of these
>>>> opaque fields should become part of uAPI in neither kernel nor
>>>> OVS in userspace.
>>>>
>>>
>>> I understand this can be cumbersome, specially given the opaque
>>> field is currently also used for some purely-internal OVS actions
>>> (e.g: CONTROLLER).
>>>
>>> However, for features such as OVN-driven per-flow sampling, where
>>> OVN-generated identifiers are placed in obs_domain_id and
>>> obs_point_id, it would be _really_ useful if this opaque value
>>> could be somehow decoded by external programs.
>>>
>>> Two ideas come to mind to try to alleviate the potential
>>> maintainability issues: - As I suggested, using a new action maybe
>>> makes things easier. By splitting the current "user_action_cookie"
>>> in two, one for internal actions and one for "observability"
>>> actions, we could expose the latter in the OVS userspace API
>>> without having to expose the former. - Exposing functions in OVS
>>> that decode the opaque value. Third party applications could link
>>> against, say, libopenvswitch.so and use it to extract
>>> obs_{domain,point}_ids.
>>
>> Linking with OVS libraries is practically the same as just exposing
>> the internal structure, because once the external application is
>> running it either must have the same library version as the process
>> that installs the action, or it may not be able to parse the
>> message.
>>
>> Any form of exposing to an external application will freeze the
>> opaque arguments and effectively make them a form of uAPI.
>>
>> The separate action with a defined uAPI solves this problem by just
>> creating a new uAPI, but I'm not sure why it is needed.
>>
>>>
>>> What do you think?
>>>
>>>> The userspace() action already has a OVS_USERSPACE_ATTR_PID
>>>> argument. And it is not actually used when
>>>> OVS_DP_F_DISPATCH_UPCALL_PER_CPU is enabled.  All known users of
>>>> OVS_DP_F_DISPATCH_UPCALL_PER_CPU are setting the
>>>> OVS_USERSPACE_ATTR_PID to UINT32_MAX, which is not a pid that
>>>> kernel could generate.
>>>>
>>>> So, with a minimal and pretty much backward compatible change in
>>>>   output_userspace() function, we can honor
>>>> OVS_USERSPACE_ATTR_PID if it's not U32_MAX.  This way userspace
>>>> process can open a separate socket and configure sampling to
>>>> redirect all packets there while normal MISS upcalls would still
>>>> arrive to per-cpu sockets.  This should cover the performance
>>>> concern.
>>>>
>>>
>>> Do you mean creating a new thread to process samples or using
>>> handlers? The latter would still have performance impact and the
>>> former would likely fail to process all samples in a timely manner
>>> if there are many.
>>>
>>> Besides, the current userspace tc-offloading series uses netlink
>>> broadcast with psample, can't we do the same for non-offloaded
>>> actions? It enable building external observability applications
>>> without overloading OVS.
>>
>> Creating a separate thread solves the performance issue.  But you can
>> also write a separate application that would communicate its PID to
>> the running OVS daemon.  Let's say the same application that
>> configures sampling in the OVS database can also write a PID there.
>>
>> The thing is that existence of external application immediately
>> breaks opacity of the arguments and forces us to define uAPI.
>> However, if there is an explicit communication between that
>> application and OVS userpsace daemon, then we can establish a
>> contract (structure of opaque values) between these two userspace
>> applications without defining that contract in the kernel uAPI.  But
>> if we're going with multicast, that anyone can subscribe to, then we
>> have to define that contract in the kernel uAPI.
>>
>> Also, in order for this observability to work with userspace datapath
>> we'll have to implement userspace-to-userspace netlink multicast
>> (does that even exist?).  Running the sample collection within OVS as
>> a thread will be much less painful.
>>
>> One other thing worth mentioning is that the PID approach I suggested
>> is just a minor tweak of what is already supported in the kernel.  It
>> doesn't prohibit introduction of a new action or a multicast group in
>> the future.  While premature uAPI definition may end up with another
>> action that nobody uses.  It can be added later if end up being
>> actually necessary.
> 
> Thinking more about this problem, it seems to make some sense to have
> a way to ask OVS for sampling that multiple observers can subscribe to.
> Unicast socket will not allow such functionality.  However, I still don't
> think creation of a new multicast group for that purpose is justified.
> Kernel already has a generic sampling mechanism (psample) with a multicast
> group created specifically for a very similar purpose.  So, instead of
> re-inventing it, we can add a small modification to the OVS'es sampling
> action allowing it to sample to psample instead of OVS'es own unicast
> sockets.  This can be achieved by adding a new OVS_SAMPLE_ATTR that
> would tell to direct packets to psample instead of executing actions.
> Or adding a new OVS_USERSPACE_ATTR that would do the same thing but from
> a userspace() action instead, i.e. direct packets to psample instead of
> OVS'es own sockets copying OVS_PACKET_ATTR_USERDATA into the
> PSAMPLE_ATTR_SAMPLE_GROUP.  Might be cleaner this way, not sure.
> 
> Form a perspective of an OVS userspace daemon, this functionality can
> be clearly exposed as a separate sampling mechanism alongside IPFIX,
> sFlow and NetFlow.
> 
> I see you eluded to this approach in the original cover letter above.
> So, I'd vote for it instead.  Hopefully the psample API can be extended
> to be more flexible and allow larger userdata to be passed in.  Maybe have
> SAMPLE_SUBGROUP in addition to the existing PSAMPLE_ATTR_SAMPLE_GROUP ?
> OTOH, it is not really IPFIX, it's a different interface, so it might have
> different requirements.  In any case OVS may check that userdata fits
> into psample arguments and reject flows that are not compatible.
> 
Thanks for your feedback Ilya.

I'll send an RFC_v2 with the proposed alternative.

>>
>> Best regards, Ilya Maximets.
>>
>>>
>>>
>>>> For the case without per-cpu dispatch, the feature comes for free
>>>> if userspace application wants to use it.  However, there is no
>>>> currently supported version of OVS that doesn't use per-cpu
>>>> dispatch when available.
>>>>> What do you think? Best regards, Ilya Maximets.
>>>>
>>>
>>
> 

-- 
Adrián Moreno