Re: [dpdk-dev] [PATCH v4 07/29] graph: implement create and destroy APIs

[Andrzej Ostruszka <amo@semihalf.com>]

On 4/5/20 10:55 AM, jerinj@marvell.com wrote:
> From: Jerin Jacob <jerinj@marvell.com>
> 
> Adding graph specific API implementations like graph create
> and graph destroy. This detect loops in the graph,
> check for isolated nodes and operation to verify the validity of
> graph.
> 
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
>  lib/librte_graph/graph.c               | 320 +++++++++++++++++++++++++
>  lib/librte_graph/rte_graph_version.map |   2 +
>  2 files changed, 322 insertions(+)
> 
> diff --git a/lib/librte_graph/graph.c b/lib/librte_graph/graph.c
> index e1930b7d2..dc373231e 100644
> --- a/lib/librte_graph/graph.c
> +++ b/lib/librte_graph/graph.c
[...]
> +static int
> +graph_node_add(struct graph *graph, struct node *node)
> +{
> +	struct graph_node *graph_node;
> +	size_t sz;
> +
> +	/* Skip the duplicate nodes */
> +	STAILQ_FOREACH(graph_node, &graph->node_list, next)
> +		if (strncmp(node->name, graph_node->node->name,
> +			    RTE_NODE_NAMESIZE) == 0)

Is it not a "deficiency" of a program to attempt to add node twice?
If it is, then maybe a warning here?

[...]
> +rte_graph_t
> +rte_graph_create(const char *name, struct rte_graph_param *prm)
> +{
> +	struct graph *graph;
> +	const char *pattern;
> +	uint16_t i;
> +
> +	graph_spinlock_lock();
> +
> +	/* Check arguments sanity */
> +	if (prm == NULL)
> +		SET_ERR_JMP(EINVAL, fail, "Param should not be NULL");
> +
> +	if (name == NULL)
> +		SET_ERR_JMP(EINVAL, fail, "Graph name should not be NULL");
> +
> +	/* Check for existence of duplicate graph */
> +	STAILQ_FOREACH(graph, &graph_list, next)
> +		if (strncmp(name, graph->name, RTE_GRAPH_NAMESIZE) == 0)
> +			SET_ERR_JMP(EEXIST, fail, "Found duplicate graph %s",
> +				    name);
> +
> +	/* Create graph object */
> +	graph = calloc(1, sizeof(*graph));
> +	if (graph == NULL)
> +		SET_ERR_JMP(ENOMEM, fail, "Failed to calloc graph object");
> +
> +	/* Initialize the graph object */
> +	STAILQ_INIT(&graph->node_list);
> +	if (rte_strscpy(graph->name, name, RTE_GRAPH_NAMESIZE) < 0)
> +		SET_ERR_JMP(E2BIG, free, "Too big name=%s", name);
> +
> +	/* Expand node pattern and add the nodes to the graph */
> +	for (i = 0; i < prm->nb_node_patterns; i++) {
> +		pattern = prm->node_patterns[i];
> +		if (expand_pattern_to_node(graph, pattern))
> +			goto graph_cleanup;
> +	}
> +
> +	/* Go over all the nodes edges and add them to the graph */
> +	if (graph_node_edges_add(graph))
> +		goto graph_cleanup;
> +
> +	/* Update adjacency list of all nodes in the graph */
> +	if (graph_adjacency_list_update(graph))
> +		goto graph_cleanup;
> +
> +	/* Make sure at least a source node present in the graph */
> +	if (!graph_src_nodes_count(graph))
> +		goto graph_cleanup;
> +
> +	/* Make sure no node is pointing to source node */
> +	if (graph_node_has_edge_to_src_node(graph))
> +		goto graph_cleanup;
> +
> +	/* Don't allow node has loop to self */
> +	if (graph_node_has_loop_edge(graph))
> +		goto graph_cleanup;
> +
> +	/* Do BFS from src nodes on the graph to find isolated nodes */
> +	if (graph_has_isolated_node(graph))
> +		goto graph_cleanup;
> +
> +	/* Initialize graph object */
> +	graph->socket = prm->socket_id;
> +	graph->src_node_count = graph_src_nodes_count(graph);

Maybe reuse value of previous call (above)?

> +	graph->node_count = graph_nodes_count(graph);
> +	graph->id = graph_id;
> +
> +	/* Allocate the Graph fast path memory and populate the data */
> +	if (graph_fp_mem_create(graph))
> +		goto graph_cleanup;
> +
> +	/* Call init() of the all the nodes in the graph */
> +	if (graph_node_init(graph))
> +		goto graph_mem_destroy;
> +
> +	/* All good, Lets add the graph to the list */
> +	graph_id++;
> +	STAILQ_INSERT_TAIL(&graph_list, graph, next);
> +
> +	graph_spinlock_unlock();
> +	return graph->id;
> +
> +graph_mem_destroy:
> +	graph_fp_mem_destroy(graph);
> +graph_cleanup:
> +	graph_cleanup(graph);
> +free:
> +	free(graph);
> +fail:
> +	graph_spinlock_unlock();
> +	return RTE_GRAPH_ID_INVALID;
> +}
> +

With regards
Andrzej Ostruszka