control_output: use dynfields

Replace the GR_MBUF_PRIV_DATA_TYPE based control_output_mbuf_data
structure with DPDK dynamic fields. Register two dynfields at
initialization: one for the control queue callback pointer and one for
a generic uintptr_t private value.

Using GR_MBUF_PRIV_DATA_TYPE for control_output_mbuf_data was
problematic because it would overwrite mbuf private data set by previous
nodes in the graph. Callers that needed to pass extra context to their
callback had to copy data into the opaque cb_data buffer, which was
error-prone and wasteful.

With dynfields, the callback and priv value are stored at dedicated
offsets that do not conflict with other node data. Callers can pass
a pointer or value directly via the priv argument without any copying.
For ICMP and ICMPv6, the timestamp is passed via priv and stored in the
queue item structure on the control plane side.

Provide control_output_set_cb() as the single entry point for setting
the callback and private data on an mbuf destined for control plane
processing.

Signed-off-by: Robin Jarry <rjarry@redhat.com>

Author: rjarry

devtools/complexity.json

@@ -15,10 +15,10 @@
         "uops": 49
       }
     ],
-    "ipc": 1.92,
-    "self_cycles": 33.8,
-    "total_cycles": 52.3,
-    "uops": 135
+    "ipc": 1.87,
+    "self_cycles": 29.5,
+    "total_cycles": 48.0,
+    "uops": 118
   },
   "arp_input_request_process": {
     "callees": [
@@ -31,9 +31,9 @@
       }
     ],
     "ipc": 1.72,
-    "self_cycles": 28.2,
-    "total_cycles": 46.7,
-    "uops": 113
+    "self_cycles": 28,
+    "total_cycles": 46.5,
+    "uops": 112
   },
   "arp_output_reply_process": {
     "callees": [
@@ -117,16 +117,16 @@
         "uops": 3
       }
     ],
-    "ipc": 1.54,
-    "self_cycles": 26,
-    "total_cycles": 145.6,
-    "uops": 104
+    "ipc": 1.65,
+    "self_cycles": 27,
+    "total_cycles": 146.6,
+    "uops": 108
   },
   "dhcp_input_process": {
-    "ipc": 1.91,
-    "self_cycles": 35.5,
-    "total_cycles": 35.5,
-    "uops": 142
+    "ipc": 1.96,
+    "self_cycles": 36.2,
+    "total_cycles": 36.2,
+    "uops": 145
   },
   "dnat44_dynamic_process": {
     "callees": [
@@ -219,10 +219,10 @@
         "uops": 42
       }
     ],
-    "ipc": 2.64,
-    "self_cycles": 55.2,
-    "total_cycles": 73.5,
-    "uops": 221
+    "ipc": 2.62,
+    "self_cycles": 54.5,
+    "total_cycles": 72.8,
+    "uops": 218
   },
   "icmp6_local_send_process": {
     "callees": [
@@ -262,10 +262,10 @@
     "uops": 402
   },
   "icmp_input_process": {
-    "ipc": 2.97,
-    "self_cycles": 59.8,
-    "total_cycles": 59.8,
-    "uops": 239
+    "ipc": 3.02,
+    "self_cycles": 60.8,
+    "total_cycles": 60.8,
+    "uops": 243
   },
   "icmp_local_send_process": {
     "ipc": 2.4,
@@ -310,10 +310,10 @@
     "uops": 112
   },
   "ip6_hold_process": {
-    "ipc": 1.28,
-    "self_cycles": 21.8,
-    "total_cycles": 21.8,
-    "uops": 87
+    "ipc": 2.85,
+    "self_cycles": 63.2,
+    "total_cycles": 63.2,
+    "uops": 253
   },
   "ip6_input_local_process": {
     "callees": [
@@ -353,10 +353,10 @@
     "uops": 209
   },
   "ip6_loadbalance_process": {
-    "ipc": 1.78,
-    "self_cycles": 28.5,
-    "total_cycles": 28.5,
-    "uops": 114
+    "ipc": 1.7,
+    "self_cycles": 27.5,
+    "total_cycles": 27.5,
+    "uops": 110
   },
   "ip6_output_process": {
     "callees": [
@@ -424,10 +424,10 @@
     "uops": 697
   },
   "ip_hold_process": {
-    "ipc": 1.28,
-    "self_cycles": 21.8,
-    "total_cycles": 21.8,
-    "uops": 87
+    "ipc": 2.91,
+    "self_cycles": 65.8,
+    "total_cycles": 65.8,
+    "uops": 263
   },
   "ip_input_local_process": {
     "ipc": 1.87,
@@ -483,7 +483,7 @@
         "ipc": 1.67,
         "name": "snat44_static_process",
         "self_cycles": 30,
-        "total_cycles": 44.1,
+        "total_cycles": 44.8,
         "uops": 120
       },
       {
@@ -496,7 +496,7 @@
     ],
     "ipc": 2.53,
     "self_cycles": 48,
-    "total_cycles": 866.1,
+    "total_cycles": 866.8,
     "uops": 192
   },
   "ipip_input_process": {
@@ -552,10 +552,10 @@
     "uops": 127
   },
   "l2_redirect_process": {
-    "ipc": 1.28,
-    "self_cycles": 21.8,
-    "total_cycles": 21.8,
-    "uops": 87
+    "ipc": 2.85,
+    "self_cycles": 63.2,
+    "total_cycles": 63.2,
+    "uops": 253
   },
   "l4_input_local_process": {
     "ipc": 1.57,
@@ -579,10 +579,10 @@
     "uops": 307
   },
   "lacp_input_process": {
-    "ipc": 2.07,
-    "self_cycles": 37.5,
-    "total_cycles": 37.5,
-    "uops": 150
+    "ipc": 2.05,
+    "self_cycles": 37.8,
+    "total_cycles": 37.8,
+    "uops": 151
   },
   "lacp_output_process": {
     "ipc": 2.32,
@@ -591,10 +591,10 @@
     "uops": 159
   },
   "loop_xvrf_process": {
-    "ipc": 1.82,
-    "self_cycles": 30,
-    "total_cycles": 30,
-    "uops": 120
+    "ipc": 1.74,
+    "self_cycles": 29,
+    "total_cycles": 29,
+    "uops": 116
   },
   "loopback_input_process": {
     "ipc": 1.64,
@@ -603,10 +603,10 @@
     "uops": 108
   },
   "loopback_output_process": {
-    "ipc": 1.19,
-    "self_cycles": 21.2,
-    "total_cycles": 21.2,
-    "uops": 85
+    "ipc": 2.85,
+    "self_cycles": 63.2,
+    "total_cycles": 63.2,
+    "uops": 253
   },
   "ndp_na_input_process": {
     "callees": [
@@ -625,10 +625,10 @@
         "uops": 49
       }
     ],
-    "ipc": 2.4,
-    "self_cycles": 40.8,
-    "total_cycles": 126.3,
-    "uops": 163
+    "ipc": 2.42,
+    "self_cycles": 38.8,
+    "total_cycles": 124.3,
+    "uops": 155
   },
   "ndp_na_output_process": {
     "ipc": 3.08,
@@ -653,10 +653,10 @@
         "uops": 49
       }
     ],
-    "ipc": 2.87,
-    "self_cycles": 64.2,
-    "total_cycles": 149.7,
-    "uops": 257
+    "ipc": 2.93,
+    "self_cycles": 62.2,
+    "total_cycles": 147.7,
+    "uops": 249
   },
   "ndp_ns_output_process": {
     "callees": [
@@ -681,10 +681,10 @@
     "uops": 195
   },
   "ndp_rs_input_process": {
-    "ipc": 1.48,
-    "self_cycles": 25.5,
-    "total_cycles": 25.5,
-    "uops": 102
+    "ipc": 1.5,
+    "self_cycles": 25.2,
+    "total_cycles": 25.2,
+    "uops": 101
   },
   "ospf_redirect_process": {
     "callees": [
@@ -702,10 +702,10 @@
     "uops": 543
   },
   "port_output_process": {
-    "ipc": 1.72,
-    "self_cycles": 28.5,
-    "total_cycles": 28.5,
-    "uops": 114
+    "ipc": 1.57,
+    "self_cycles": 26.5,
+    "total_cycles": 26.5,
+    "uops": 106
   },
   "rx_process": {
     "callees": [

modules/dhcp/datapath/dhcp_input.c

@@ -30,14 +30,12 @@ enum {
 
 static uint16_t
 dhcp_input_process(struct rte_graph *graph, struct rte_node *node, void **objs, uint16_t nb_objs) {
-	struct control_output_mbuf_data *d;
 	struct rte_mbuf *mbuf;
 
 	for (uint16_t i = 0; i < nb_objs; i++) {
 		mbuf = objs[i];
 
-		d = control_output_mbuf_data(mbuf);
-		d->callback = dhcp_input_cb;
+		control_output_set_cb(mbuf, dhcp_input_cb, 0);
 
 		if (gr_mbuf_is_traced(mbuf)) {
 			struct dhcp_input_trace_data *t;

modules/infra/control/lacp.c

@@ -26,7 +26,6 @@
 static struct event *lacp_timer;
 
 void lacp_input_cb(void *obj, uintptr_t, const struct control_queue_drain *drain) {
-	struct control_output_mbuf_data *ctrl_data = control_output_mbuf_data(obj);
 	const struct iface_info_port *port;
 	const struct iface *port_iface;
 	struct iface_info_bond *bond;
@@ -35,7 +34,8 @@ void lacp_input_cb(void *obj, uintptr_t, const struct control_queue_drain *drain
 	struct bond_member *member;
 	struct iface *bond_iface;
 
-	memcpy(&port_iface, ctrl_data->cb_data, sizeof(struct iface *));
+	port_iface = mbuf_data(mbuf)->iface;
+
 	// Check if packet references deleted interface.
 	if (drain && drain->event == GR_EVENT_IFACE_REMOVE && port_iface == drain->obj)
 		goto out;

modules/infra/datapath/control_output.c

@@ -11,24 +11,33 @@
 
 #define ERROR 0
 
+int cq_callback_offset;
+int cq_priv_offset;
+
 static uint16_t control_output_process(
 	struct rte_graph *graph,
 	struct rte_node *node,
 	void **objs,
 	uint16_t n_objs
 ) {
+	control_queue_cb_t callback;
+	struct rte_mbuf *m;
 	unsigned sent = 0;
+	uintptr_t priv;
 
 	for (unsigned i = 0; i < n_objs; i++) {
-		const struct control_output_mbuf_data *d = control_output_mbuf_data(objs[i]);
-		if (control_queue_push(d->callback, objs[i], 0) < 0)
-			rte_node_enqueue_x1(graph, node, ERROR, objs[i]);
-		else {
+		m = objs[i];
+		callback = *RTE_MBUF_DYNFIELD(m, cq_callback_offset, control_queue_cb_t *);
+		priv = *RTE_MBUF_DYNFIELD(m, cq_priv_offset, uintptr_t *);
+
+		if (control_queue_push(callback, m, priv) < 0) {
+			rte_node_enqueue_x1(graph, node, ERROR, m);
+		} else {
 			sent++;
-			if (gr_mbuf_is_traced(objs[i])) {
+			if (gr_mbuf_is_traced(m)) {
 				// FIXME racy: we are operating on mbufs already enqueued in ring
-				gr_mbuf_trace_add(objs[i], node, 0);
-				gr_mbuf_trace_finish(objs[i]);
+				gr_mbuf_trace_add(m, node, 0);
+				gr_mbuf_trace_finish(m);
 			}
 		}
 	}
@@ -38,6 +47,25 @@ static uint16_t control_output_process(
 	return n_objs;
 }
 
+static void control_output_register(void) {
+	const struct rte_mbuf_dynfield cb_params = {
+		.name = "cq_callback",
+		.size = sizeof(control_queue_cb_t),
+		.align = alignof(control_queue_cb_t),
+	};
+	const struct rte_mbuf_dynfield priv_params = {
+		.name = "cq_priv",
+		.size = sizeof(uintptr_t),
+		.align = alignof(uintptr_t),
+	};
+	cq_callback_offset = rte_mbuf_dynfield_register(&cb_params);
+	if (cq_callback_offset < 0)
+		ABORT("rte_mbuf_dynfield_register(cq_callback): %s", rte_strerror(rte_errno));
+	cq_priv_offset = rte_mbuf_dynfield_register(&priv_params);
+	if (cq_priv_offset < 0)
+		ABORT("rte_mbuf_dynfield_register(cq_priv): %s", rte_strerror(rte_errno));
+}
+
 static struct rte_node_register control_output_node = {
 	.name = "control_output",
 	.process = control_output_process,
@@ -50,6 +78,7 @@ static struct rte_node_register control_output_node = {
 static struct gr_node_info info = {
 	.node = &control_output_node,
 	.type = GR_NODE_T_CONTROL,
+	.register_callback = control_output_register,
 };
 
 GR_NODE_REGISTER(info);

modules/infra/datapath/gr_control_output.h

@@ -5,10 +5,14 @@
 #pragma once
 
 #include <gr_control_queue.h>
-#include <gr_mbuf.h>
 
-GR_MBUF_PRIV_DATA_TYPE(control_output_mbuf_data, {
-	control_queue_cb_t callback;
-	clock_t timestamp;
-	uint8_t cb_data[GR_MBUF_PRIV_MAX_SIZE - 6 * sizeof(size_t)];
-});
+#include <rte_mbuf.h>
+
+extern int cq_callback_offset;
+extern int cq_priv_offset;
+
+static inline void
+control_output_set_cb(struct rte_mbuf *m, control_queue_cb_t cb, uintptr_t priv) {
+	*RTE_MBUF_DYNFIELD(m, cq_callback_offset, control_queue_cb_t *) = cb;
+	*RTE_MBUF_DYNFIELD(m, cq_priv_offset, uintptr_t *) = priv;
+}