Re: [dpdk-dev] [PATCH v4 06/29] graph: populate fastpath memory for graph reel

[Andrzej Ostruszka <amo@semihalf.com>]

On 4/5/20 10:55 AM, jerinj@marvell.com wrote:
> From: Jerin Jacob <jerinj@marvell.com>
[...]
> diff --git a/lib/librte_graph/graph_populate.c b/lib/librte_graph/graph_populate.c
> new file mode 100644
> index 000000000..093512efa
> --- /dev/null
> +++ b/lib/librte_graph/graph_populate.c
> @@ -0,0 +1,234 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell International Ltd.
> + */
> +
> +#include <fnmatch.h>
> +#include <stdbool.h>
> +
> +#include <rte_common.h>
> +#include <rte_errno.h>
> +#include <rte_malloc.h>
> +#include <rte_memzone.h>
> +
> +#include "graph_private.h"
> +
> +static size_t
> +graph_fp_mem_calc_size(struct graph *graph)
> +{
> +	struct graph_node *graph_node;
> +	rte_node_t val;
> +	size_t sz;
> +
> +	/* Graph header */
> +	sz = sizeof(struct rte_graph);
> +	/* Source nodes list */
> +	sz += sizeof(rte_graph_off_t) * graph->src_node_count;
> +	/* Circular buffer for pending streams of size number of nodes */
> +	val = rte_align32pow2(graph->node_count * sizeof(rte_graph_off_t));
> +	sz = RTE_ALIGN(sz, val);
> +	graph->cir_start = sz;
> +	graph->cir_mask = rte_align32pow2(graph->node_count) - 1;
> +	sz += val;

Aren't here source nodes counted twice?  I'm trying now to wrap my head
around how this all is structured and laid out in memory (thus the
slowdown in review) so I am most probably missing something here.

> +	/* Fence */
> +	sz += sizeof(RTE_GRAPH_FENCE);
> +	sz = RTE_ALIGN(sz, RTE_CACHE_LINE_SIZE);
> +	graph->nodes_start = sz;
> +	/* For 0..N node objects with fence */
> +	STAILQ_FOREACH(graph_node, &graph->node_list, next) {
> +		sz = RTE_ALIGN(sz, RTE_CACHE_LINE_SIZE);
> +		sz += sizeof(struct rte_node);
> +		/* Pointer to next nodes(edges) */
> +		sz += sizeof(struct rte_node *) * graph_node->node->nb_edges;
> +	}
> +
> +	graph->mem_sz = sz;
> +	return sz;
> +}
> +
> +static void
> +graph_header_popluate(struct graph *_graph)
> +{
> +	struct rte_graph *graph = _graph->graph;
> +
> +	graph->tail = 0;
> +	graph->head = (int32_t)-_graph->src_node_count;
> +	graph->cir_mask = _graph->cir_mask;
> +	graph->nb_nodes = _graph->node_count;
> +	graph->cir_start = RTE_PTR_ADD(graph, _graph->cir_start);
> +	graph->nodes_start = _graph->nodes_start;
> +	graph->socket = _graph->socket;
> +	graph->id = _graph->id;
> +	memcpy(graph->name, _graph->name, RTE_GRAPH_NAMESIZE);

As I've mentioned above I'm learning the structure of the lib/memory so
quick question here.  My understanding is that rte_graph is a "view of
the 'struct graph' sufficient for worker" so does it need both id &
name?  Both of them seems to be used in error or dump/debug paths.  It
probably doesn't matter (e.g. for performance) - just asking because
'id' seems to be used only in one place (where name could replace it
probably).

> +	graph->fence = RTE_GRAPH_FENCE;
> +}
> +
> +static void
> +graph_nodes_populate(struct graph *_graph)
> +{
> +	rte_graph_off_t off = _graph->nodes_start;
> +	struct rte_graph *graph = _graph->graph;
> +	struct graph_node *graph_node;
> +	rte_edge_t count, nb_edges;
> +	const char *parent;
> +	rte_node_t pid;
> +
> +	STAILQ_FOREACH(graph_node, &_graph->node_list, next) {
> +		struct rte_node *node = RTE_PTR_ADD(graph, off);
> +		memset(node, 0, sizeof(*node));
> +		node->fence = RTE_GRAPH_FENCE;
> +		node->off = off;
> +		node->process = graph_node->node->process;
> +		memcpy(node->name, graph_node->node->name, RTE_GRAPH_NAMESIZE);
> +		pid = graph_node->node->parent_id;
> +		if (pid != RTE_NODE_ID_INVALID) { /* Cloned node */
> +			parent = rte_node_id_to_name(pid);
> +			memcpy(node->parent, parent, RTE_GRAPH_NAMESIZE);
> +		}
> +		node->id = graph_node->node->id;
> +		node->parent_id = pid;
> +		nb_edges = graph_node->node->nb_edges;
> +		node->nb_edges = nb_edges;
> +		off += sizeof(struct rte_node);
> +		/* Copy the name in first pass to replace with rte_node* later*/
> +		for (count = 0; count < nb_edges; count++)
> +			node->nodes[count] = (struct rte_node *)&graph_node
> +						     ->adjacency_list[count]
> +						     ->node->name[0];

I'm not sure I understand what is going here.  Please see below ...

> +
> +		off += sizeof(struct rte_node *) * nb_edges;
> +		off = RTE_ALIGN(off, RTE_CACHE_LINE_SIZE);
> +		node->next = off;
> +		__rte_node_stream_alloc(graph, node);
> +	}
> +}
[...]
> +static int
> +graph_node_nexts_populate(struct graph *_graph)
> +{
> +	rte_node_t count, val;
> +	rte_graph_off_t off;
> +	struct rte_node *node;
> +	const struct rte_graph *graph = _graph->graph;
> +	const char *name;
> +
> +	rte_graph_foreach_node(count, off, graph, node) {
> +		for (val = 0; val < node->nb_edges; val++) {
> +			name = (const char *)node->nodes[val];
> +			node->nodes[val] = graph_node_name_to_ptr(graph, name);

... Is it so that during node the first loop above some node might refer
(by name) to other node that is not yet "registered" so instead of
storing rte_node pointer you stored actually pointer to name which you
now update to proper rte_node?

> +			if (node->nodes[val] == NULL)
> +				SET_ERR_JMP(EINVAL, fail, "%s not found", name);
> +		}
> +	}
> +
> +	return 0;
> +fail:
> +	return -rte_errno;
> +}
[...]

With regards
Andrzej Ostruszka