Re: [dpdk-dev] [EXT] Re: [PATCH v4 03/29] graph: implement node operations

[Kiran Kumar Kokkilagadda <kirankumark@marvell.com>]

> -----Original Message-----
> From: Andrzej Ostruszka <amo@semihalf.com>
> Sent: Monday, April 6, 2020 11:27 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com;
> mdr@ashroe.eu; mattias.ronnblom@ericsson.com; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; xiao.w.wang@intel.com
> Subject: [EXT] Re: [dpdk-dev] [PATCH v4 03/29] graph: implement node
> operations
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 4/5/20 10:55 AM, jerinj@marvell.com wrote:
> [...]
> > diff --git a/lib/librte_graph/node.c b/lib/librte_graph/node.c index
> > 336cd1c94..d04a0fce0 100644
> > --- a/lib/librte_graph/node.c
> > +++ b/lib/librte_graph/node.c
> [...]
> > +static rte_edge_t
> > +edge_update(struct node *node, struct node *prev, rte_edge_t from,
> > +	    const char **next_nodes, rte_edge_t nb_edges) {
> > +	rte_edge_t i, max_edges, count = 0;
> > +	struct node *new_node;
> > +	bool need_realloc;
> > +	size_t sz;
> > +
> > +	if (from == RTE_EDGE_ID_INVALID)
> > +		from = node->nb_edges;
> > +
> > +	/* Don't create hole in next_nodes[] list */
> > +	if (from > node->nb_edges) {
> > +		rte_errno = ENOMEM;
> > +		goto fail;
> > +	}
> > +
> > +	/* Remove me from list */
> > +	STAILQ_REMOVE(&node_list, node, node, next);

This is where we will remove the node first unconditionally. Later we update the new node.

> > +
> > +	/* Allocate the storage space for new node if required */
> > +	max_edges = from + nb_edges;
> > +	need_realloc = max_edges > node->nb_edges;
> > +	if (need_realloc) {
> > +		sz = sizeof(struct node) + (max_edges * RTE_NODE_NAMESIZE);
> > +		new_node = realloc(node, sz);
> > +		if (new_node == NULL) {
> > +			rte_errno = ENOMEM;
> > +			goto restore;
> > +		} else {
> > +			node = new_node;
> > +		}
> > +	}
> > +
> > +	/* Update the new nodes name */
> > +	for (i = from; i < max_edges; i++, count++) {
> > +		if (rte_strscpy(node->next_nodes[i], next_nodes[count],
> > +				RTE_NODE_NAMESIZE) < 0) {
> > +			rte_errno = E2BIG;
> > +			goto restore;
> > +		}
> > +	}
> > +restore:
> > +	/* Update the linked list to point new node address in prev node */
> > +	if (prev)
> > +		STAILQ_INSERT_AFTER(&node_list, prev, node, next);
> > +	else
> > +		STAILQ_INSERT_HEAD(&node_list, node, next);
> 
> AFAIU node_list keeps the list of nodes - so I guess you wanted here "replace"
> the old node pointer with the new one.  I have not yet seen the usage of this
> function but it seems to me like you unconditionally insert the updated node -
> possibly having node pointer present doubly or with stale pointer.  I might be
> missing something here.
> 
See above.

> > +
> > +	if (need_realloc)
> > +		node->nb_edges += max_edges;
> 
> It looks to me like this should be simple '='.
> 
> > +
> > +fail:
> > +	return count;
> > +}
> [...]
> > +rte_edge_t
> > +rte_node_edge_update(rte_node_t id, rte_edge_t from, const char
> **next_nodes,
> > +		     uint16_t nb_edges)
> > +{
> > +	rte_edge_t rc = RTE_EDGE_ID_INVALID;
> > +	struct node *n, *prev;
> > +
> > +	NODE_ID_CHECK(id);
> > +	graph_spinlock_lock();
> > +
> > +	prev = NULL;
> > +	STAILQ_FOREACH(n, &node_list, next) {
> > +		if (n->id == id) {
> > +			rc = edge_update(n, prev, from, next_nodes,
> nb_edges);
> > +			break;
> > +		}
> > +		prev = n;
> > +	}
> 
> OK so in this context my comment above seems to be valid.  When we find the id
> we have: prev -> n, we call update() and in there we insert new_node after prev
> so we end up with: prev -> n' -> n where n' might be new address for n or just n
> when no realloc was performed.
> 
> Do I miss anything?
> 

See above.

> > +
> > +	graph_spinlock_unlock();
> > +fail:
> > +	return rc;
> > +}
> > +
> > +static rte_node_t
> > +node_copy_edges(struct node *node, char *next_nodes[]) {
> > +	rte_edge_t i;
> > +
> > +	for (i = 0; i < node->nb_edges; i++)
> > +		next_nodes[i] = node->next_nodes[i];
> > +
> > +	return i;
> > +}
> > +
> > +rte_node_t
> > +rte_node_edge_get(rte_node_t id, char *next_nodes[]) {
> > +	rte_node_t rc = RTE_NODE_ID_INVALID;
> > +	struct node *node;
> > +
> > +	NODE_ID_CHECK(id);
> > +	graph_spinlock_lock();
> > +
> > +	STAILQ_FOREACH(node, &node_list, next) {
> > +		if (node->id == id) {
> > +			if (next_nodes == NULL)
> > +				rc = sizeof(char *) * node->nb_edges;
> > +			else
> > +				rc = node_copy_edges(node, next_nodes);
> 
> Do we want to ready for next_nodes not large enough?
> 
> > +			break;
> > +		}
> > +	}
> > +
> > +	graph_spinlock_unlock();
> > +fail:
> > +	return rc;
> > +}
> 
> [...]
> 
> With regards
> Andrzej Ostruszka