dpdk: fix graph issues

Signed-off-by: Robin Jarry <[email protected]>
DPDK · Mar 25, 2024 · fdb0313 · fdb0313
1 parent 1cceefe
commit fdb0313
Show file tree

Hide file tree

Showing 4 changed files with 351 additions and 2 deletions.
diff --git a/subprojects/dpdk.wrap b/subprojects/dpdk.wrap
@@ -6,7 +6,9 @@ diff_files =
 	dpdk/build-whole-archive-for-static-libs.patch,
 	dpdk/build-pass-cflags-in-subproject.patch,
 	dpdk/graph-enhance-export-to-graphviz.patch,
-	dpdk/node-change-ctx-array-to-union.patch
+	dpdk/node-change-ctx-array-to-union.patch,
+	dpdk/graph-avoid-accessing-global-list-in-rte_graph_clust.patch,
+	dpdk/graph-avoid-id-collisions.patch
 
 [provide]
 dependency_names = libdpdk
diff --git a/subprojects/packagefiles/dpdk/graph-avoid-accessing-global-list-in-rte_graph_clust.patch b/subprojects/packagefiles/dpdk/graph-avoid-accessing-global-list-in-rte_graph_clust.patch
@@ -0,0 +1,188 @@
+From 1b52e39c61260745852dce3343921169e0437de3 Mon Sep 17 00:00:00 2001
+From: Robin Jarry <[email protected]>
+Date: Mon, 25 Mar 2024 11:50:19 +0100
+Subject: [PATCH 3/4] graph: avoid accessing global list in
+ rte_graph_cluster_stats_get
+
+In rte_graph_cluster_stats_get, the walk model of the first graph is
+checked to determine if multi-core dispatch specific counters should be
+updated or not. This global list is accessed without any locks.
+
+If the global list is modified by another thread while
+rte_graph_cluster_stats_get is called, it can result in undefined
+behaviour.
+
+Adding a lock would make it impossible to call
+rte_graph_cluster_stats_get in packet processing code paths. Avoid
+accessing the global list instead by storing a bool field in the private
+rte_graph_cluster_stats structure.
+
+Also update the default callback to avoid accessing the global list and
+store different callbacks according to the graph models.
+
+Signed-off-by: Robin Jarry <[email protected]>
+---
+ lib/graph/graph_stats.c | 60 ++++++++++++++++++++++++++---------------
+ 1 file changed, 39 insertions(+), 21 deletions(-)
+
+diff --git a/lib/graph/graph_stats.c b/lib/graph/graph_stats.c
+index 2fb808b21ec5..f8048e08be8a 100644
+--- a/lib/graph/graph_stats.c
++++ b/lib/graph/graph_stats.c
+@@ -34,6 +34,7 @@ struct __rte_cache_aligned rte_graph_cluster_stats {
+ 	uint32_t cluster_node_size; /* Size of struct cluster_node */
+ 	rte_node_t max_nodes;
+ 	int socket_id;
++	bool dispatch;
+ 	void *cookie;
+ 	size_t sz;
+
+@@ -74,17 +75,16 @@ print_banner_dispatch(FILE *f)
+ }
+
+ static inline void
+-print_banner(FILE *f)
++print_banner(FILE *f, bool dispatch)
+ {
+-	if (rte_graph_worker_model_get(STAILQ_FIRST(graph_list_head_get())->graph) ==
+-	    RTE_GRAPH_MODEL_MCORE_DISPATCH)
++	if (dispatch)
+ 		print_banner_dispatch(f);
+ 	else
+ 		print_banner_default(f);
+ }
+
+ static inline void
+-print_node(FILE *f, const struct rte_graph_cluster_node_stats *stat)
++print_node(FILE *f, const struct rte_graph_cluster_node_stats *stat, bool dispatch)
+ {
+ 	double objs_per_call, objs_per_sec, cycles_per_call, ts_per_hz;
+ 	const uint64_t prev_calls = stat->prev_calls;
+@@ -104,8 +104,7 @@ print_node(FILE *f, const struct rte_graph_cluster_node_stats *stat)
+ 	objs_per_sec = ts_per_hz ? (objs - prev_objs) / ts_per_hz : 0;
+ 	objs_per_sec /= 1000000;
+
+-	if (rte_graph_worker_model_get(STAILQ_FIRST(graph_list_head_get())->graph) ==
+-	    RTE_GRAPH_MODEL_MCORE_DISPATCH) {
++	if (dispatch) {
+ 		fprintf(f,
+ 			"|%-31s|%-15" PRIu64 "|%-15" PRIu64 "|%-15" PRIu64
+ 			"|%-15" PRIu64 "|%-15" PRIu64
+@@ -123,20 +122,17 @@ print_node(FILE *f, const struct rte_graph_cluster_node_stats *stat)
+ }
+
+ static int
+-graph_cluster_stats_cb(bool is_first, bool is_last, void *cookie,
++graph_cluster_stats_cb(bool dispatch, bool is_first, bool is_last, void *cookie,
+ 		       const struct rte_graph_cluster_node_stats *stat)
+ {
+ 	FILE *f = cookie;
+-	int model;
+-
+-	model = rte_graph_worker_model_get(STAILQ_FIRST(graph_list_head_get())->graph);
+
+ 	if (unlikely(is_first))
+-		print_banner(f);
++		print_banner(f, dispatch);
+ 	if (stat->objs)
+-		print_node(f, stat);
++		print_node(f, stat, dispatch);
+ 	if (unlikely(is_last)) {
+-		if (model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
++		if (dispatch)
+ 			boarder_model_dispatch();
+ 		else
+ 			boarder();
+@@ -145,6 +141,20 @@ graph_cluster_stats_cb(bool is_first, bool is_last, void *cookie,
+ 	return 0;
+ };
+
++static int
++graph_cluster_stats_cb_rtc(bool is_first, bool is_last, void *cookie,
++			   const struct rte_graph_cluster_node_stats *stat)
++{
++	return graph_cluster_stats_cb(false, is_first, is_last, cookie, stat);
++};
++
++static int
++graph_cluster_stats_cb_dispatch(bool is_first, bool is_last, void *cookie,
++				const struct rte_graph_cluster_node_stats *stat)
++{
++	return graph_cluster_stats_cb(true, is_first, is_last, cookie, stat);
++};
++
+ static struct rte_graph_cluster_stats *
+ stats_mem_init(struct cluster *cluster,
+ 	       const struct rte_graph_cluster_stats_param *prm)
+@@ -157,8 +167,16 @@ stats_mem_init(struct cluster *cluster,
+
+ 	/* Fix up callback */
+ 	fn = prm->fn;
+-	if (fn == NULL)
+-		fn = graph_cluster_stats_cb;
++	if (fn == NULL) {
++		for (int i = 0; i < cluster->nb_graphs; i++) {
++			const struct rte_graph *graph = cluster->graphs[i]->graph;
++			if (graph->model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
++				fn = graph_cluster_stats_cb_dispatch;
++			else
++				fn = graph_cluster_stats_cb_rtc;
++			break;
++		}
++	}
+
+ 	cluster_node_size = sizeof(struct cluster_node);
+ 	/* For a given cluster, max nodes will be the max number of graphs */
+@@ -350,6 +368,8 @@ rte_graph_cluster_stats_create(const struct rte_graph_cluster_stats_param *prm)
+ 			if (stats_mem_populate(&stats, graph_fp, graph_node))
+ 				goto realloc_fail;
+ 		}
++		if (graph->graph->model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
++			stats->dispatch = true;
+ 	}
+
+ 	/* Finally copy to hugepage memory to avoid pressure on rte_realloc */
+@@ -375,20 +395,18 @@ rte_graph_cluster_stats_destroy(struct rte_graph_cluster_stats *stat)
+ }
+
+ static inline void
+-cluster_node_arregate_stats(struct cluster_node *cluster)
++cluster_node_arregate_stats(struct cluster_node *cluster, bool dispatch)
+ {
+ 	uint64_t calls = 0, cycles = 0, objs = 0, realloc_count = 0;
+ 	struct rte_graph_cluster_node_stats *stat = &cluster->stat;
+ 	uint64_t sched_objs = 0, sched_fail = 0;
+ 	struct rte_node *node;
+ 	rte_node_t count;
+-	int model;
+
+-	model = rte_graph_worker_model_get(STAILQ_FIRST(graph_list_head_get())->graph);
+ 	for (count = 0; count < cluster->nb_nodes; count++) {
+ 		node = cluster->nodes[count];
+
+-		if (model == RTE_GRAPH_MODEL_MCORE_DISPATCH) {
++		if (dispatch) {
+ 			sched_objs += node->dispatch.total_sched_objs;
+ 			sched_fail += node->dispatch.total_sched_fail;
+ 		}
+@@ -403,7 +421,7 @@ cluster_node_arregate_stats(struct cluster_node *cluster)
+ 	stat->objs = objs;
+ 	stat->cycles = cycles;
+
+-	if (model == RTE_GRAPH_MODEL_MCORE_DISPATCH) {
++	if (dispatch) {
+ 		stat->dispatch.sched_objs = sched_objs;
+ 		stat->dispatch.sched_fail = sched_fail;
+ 	}
+@@ -433,7 +451,7 @@ rte_graph_cluster_stats_get(struct rte_graph_cluster_stats *stat, bool skip_cb)
+ 	cluster = stat->clusters;
+
+ 	for (count = 0; count < stat->max_nodes; count++) {
+-		cluster_node_arregate_stats(cluster);
++		cluster_node_arregate_stats(cluster, stat->dispatch);
+ 		if (!skip_cb)
+ 			rc = stat->fn(!count, (count == stat->max_nodes - 1),
+ 				      stat->cookie, &cluster->stat);
+-- 
+2.44.0
+
diff --git a/subprojects/packagefiles/dpdk/graph-avoid-id-collisions.patch b/subprojects/packagefiles/dpdk/graph-avoid-id-collisions.patch
@@ -0,0 +1,159 @@
+From 065501abf9657da6b57d4fbcbed6bb42376de06a Mon Sep 17 00:00:00 2001
+From: Robin Jarry <[email protected]>
+Date: Mon, 25 Mar 2024 14:29:06 +0100
+Subject: [PATCH 4/4] graph: avoid id collisions
+
+The graph id is determined based on a global variable that is
+incremented every time a graph is created, and decremented every time
+a graph is destroyed. This only works if graphs are destroyed in the
+reverse order in which they have been created.
+
+The following code produces duplicate graph ids:
+
+	id1 = rte_graph_create(...);
+	id2 = rte_graph_create(...);
+	rte_graph_destroy(id1);
+	id3 = rte_graph_create(...); // new graph with duplicate id=2
+
+Remove the global counter. Make sure that the graph list is always
+ordered by increasing graph ids. When creating a new graph, pick a free
+id which is not allocated.
+
+Signed-off-by: Robin Jarry <[email protected]>
+---
+ lib/graph/graph.c | 75 ++++++++++++++++++++++++++++++++++++++++-------
+ 1 file changed, 64 insertions(+), 11 deletions(-)
+
+diff --git a/lib/graph/graph.c b/lib/graph/graph.c
+index 147bc6c685c5..f40b47297633 100644
+--- a/lib/graph/graph.c
++++ b/lib/graph/graph.c
+@@ -19,11 +19,61 @@
+
+ static struct graph_head graph_list = STAILQ_HEAD_INITIALIZER(graph_list);
+ static rte_spinlock_t graph_lock = RTE_SPINLOCK_INITIALIZER;
+-static rte_graph_t graph_id;
+-
+-#define GRAPH_ID_CHECK(id) ID_CHECK(id, graph_id)
+
+ /* Private functions */
++static struct graph *
++graph_from_id(rte_graph_t id)
++{
++	struct graph *graph;
++	STAILQ_FOREACH(graph, &graph_list, next) {
++		if (graph->id == id)
++			return graph;
++	}
++	rte_errno = EINVAL;
++	return NULL;
++}
++
++#define GRAPH_ID_CHECK(id)                                                     \
++	do {                                                                   \
++		if (graph_from_id(id) == NULL)                                 \
++			goto fail;                                             \
++	} while (0)
++
++static rte_graph_t
++graph_next_free_id(void)
++{
++	struct graph *graph;
++	rte_graph_t id = 0;
++
++	STAILQ_FOREACH(graph, &graph_list, next) {
++		if (id < graph->id) {
++			break;
++		}
++		id = graph->id + 1;
++	}
++
++	return id;
++}
++
++static void
++graph_insert_ordered(struct graph *graph)
++{
++	struct graph *after, *g;
++
++	after = NULL;
++	STAILQ_FOREACH(g, &graph_list, next) {
++		if (g->id < graph->id) {
++			after = g;
++			break;
++		}
++	}
++	if (after == NULL) {
++		STAILQ_INSERT_TAIL(&graph_list, graph, next);
++	} else {
++		STAILQ_INSERT_AFTER(&graph_list, after, graph, next);
++	}
++}
++
+ struct graph_head *
+ graph_list_head_get(void)
+ {
+@@ -406,7 +456,7 @@ rte_graph_create(const char *name, struct rte_graph_param *prm)
+ 	graph->socket = prm->socket_id;
+ 	graph->src_node_count = src_node_count;
+ 	graph->node_count = graph_nodes_count(graph);
+-	graph->id = graph_id;
++	graph->id = graph_next_free_id();
+ 	graph->parent_id = RTE_GRAPH_ID_INVALID;
+ 	graph->lcore_id = RTE_MAX_LCORE;
+ 	graph->num_pkt_to_capture = prm->num_pkt_to_capture;
+@@ -422,8 +472,7 @@ rte_graph_create(const char *name, struct rte_graph_param *prm)
+ 		goto graph_mem_destroy;
+
+ 	/* All good, Lets add the graph to the list */
+-	graph_id++;
+-	STAILQ_INSERT_TAIL(&graph_list, graph, next);
++	graph_insert_ordered(graph);
+
+ 	graph_spinlock_unlock();
+ 	return graph->id;
+@@ -467,7 +516,6 @@ rte_graph_destroy(rte_graph_t id)
+ 			graph_cleanup(graph);
+ 			STAILQ_REMOVE(&graph_list, graph, graph, next);
+ 			free(graph);
+-			graph_id--;
+ 			goto done;
+ 		}
+ 		graph = tmp;
+@@ -520,7 +568,7 @@ graph_clone(struct graph *parent_graph, const char *name, struct rte_graph_param
+ 	graph->parent_id = parent_graph->id;
+ 	graph->lcore_id = parent_graph->lcore_id;
+ 	graph->socket = parent_graph->socket;
+-	graph->id = graph_id;
++	graph->id = graph_next_free_id();
+
+ 	/* Allocate the Graph fast path memory and populate the data */
+ 	if (graph_fp_mem_create(graph))
+@@ -539,8 +587,7 @@ graph_clone(struct graph *parent_graph, const char *name, struct rte_graph_param
+ 		goto graph_mem_destroy;
+
+ 	/* All good, Lets add the graph to the list */
+-	graph_id++;
+-	STAILQ_INSERT_TAIL(&graph_list, graph, next);
++	graph_insert_ordered(graph);
+
+ 	graph_spinlock_unlock();
+ 	return graph->id;
+@@ -776,7 +823,13 @@ rte_graph_list_dump(FILE *f)
+ rte_graph_t
+ rte_graph_max_count(void)
+ {
+-	return graph_id;
++	struct graph *graph;
++	rte_graph_t count = 0;
++
++	STAILQ_FOREACH(graph, &graph_list, next)
++		count++;
++
++	return count;
+ }
+
+ RTE_LOG_REGISTER_DEFAULT(rte_graph_logtype, INFO);
+-- 
+2.44.0
+
diff --git a/subprojects/packagefiles/dpdk/graph-enhance-export-to-graphviz.patch b/subprojects/packagefiles/dpdk/graph-enhance-export-to-graphviz.patch
@@ -1,7 +1,7 @@
 From affb18c5d24aba613a038cd032efeff583b5af85 Mon Sep 17 00:00:00 2001
 From: Robin Jarry <[email protected]>
 Date: Tue, 19 Mar 2024 17:46:17 +0100
-Subject: [PATCH] graph: enhance export to graphviz
+Subject: [PATCH 1/4] graph: enhance export to graphviz
 
 * Quote graph name to avoid parsing errors when it contains a dash.
 * Use fixed margin and smaller font for a more compact layout.