> -----Original Message----- > From: Andrzej Ostruszka <a...@semihalf.com> > Sent: Monday, April 6, 2020 11:27 PM > To: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Kiran Kumar Kokkilagadda > <kirankum...@marvell.com> > Cc: dev@dpdk.org; tho...@monjalon.net; david.march...@redhat.com; > m...@ashroe.eu; mattias.ronnb...@ericsson.com; Pavan Nikhilesh > Bhagavatula <pbhagavat...@marvell.com>; Nithin Kumar Dabilpuram > <ndabilpu...@marvell.com>; xiao.w.w...@intel.com > Subject: [EXT] Re: [dpdk-dev] [PATCH v4 03/29] graph: implement node > operations > > External Email > > ---------------------------------------------------------------------- > On 4/5/20 10:55 AM, jer...@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