From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 59BDFA0577; Mon, 6 Apr 2020 19:57:13 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5261D2BE9; Mon, 6 Apr 2020 19:57:12 +0200 (CEST) Received: from mail-lf1-f67.google.com (mail-lf1-f67.google.com [209.85.167.67]) by dpdk.org (Postfix) with ESMTP id 364AFF12 for ; Mon, 6 Apr 2020 19:57:11 +0200 (CEST) Received: by mail-lf1-f67.google.com with SMTP id x23so241073lfq.1 for ; Mon, 06 Apr 2020 10:57:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Qt6vVOR6RMBi9fBpsqdf4TT7fnxGgBOYZ/Mz8OvKY0I=; b=q/o+ttYTqSOQVSDJmznoCJYuZ303kKPSnuLfOz1jDh/VojWgoemIIHSqHFFcExWvC/ hdA4zaeclPo6/Nx7lJ+rt/E4+wv9ftoyajC2T6M1HFAQ8HTPc+xLQkpALwNYeKFJQwTr EPlAjISYlXDvmgEF5N2SmJt7LeZyYYc3VRV7INSHOlhlgvc9oRM66yAAMQl+Mx4mAji7 R3zroeidQX1kiYadtQcIO2bCyyAtV+m2NhH01xcXX8HD+MTeIO+hx1wxA+MFJ15rIo5Z vGncP7hZMYk0iip6XQOP8I4gMVToZYn8HRIlG1G2omKBTh9IcYj1nzKZ3mWhQL9sliDL qmYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Qt6vVOR6RMBi9fBpsqdf4TT7fnxGgBOYZ/Mz8OvKY0I=; b=esZxLepO0/1RmvvrL62d7PA6Xvn7ibyGbBrcma0qkTzuql6qLZ6/W86QwvA3iUgitP 7Xb/6XiMMO430LwoCI9i9g43I08WGPm2jJa6l+OhAWE+4WSL7E5eqEILvpa9CNYGGM2I XGEBO61gt4A1xjN0mQmU6ITWBsKTLA+Sd9LZHyrzbSSsjAusmmnX8mzoZvDFwethDG07 bqr/xHTrFjyzdm0PECKh7smyDEpQwiQ115wI8rSlSNWc8EC1mtVDa9h5u26WOvCfs+Mz vqXZgghH9bjDPIjqRBaoRrW+rawnatBUzdVSidrp4pEGMhFKhuc856l+H9PsiaWv7yWU IBuw== X-Gm-Message-State: AGi0PuYSbkasf73DUiYtc8LQZ31if9zQs9AR1Nx2E/3Vn7rxy9IR/uib Nk783GVf+xtKyEaIJby9wAxPDQ== X-Google-Smtp-Source: APiQypIQrstGTgGX15bGVOElXlDtBYPPEc106Egb/9SYA0dD0XS/WlJMOsfnnhf4ZywRFie9M39fWg== X-Received: by 2002:a19:c64b:: with SMTP id w72mr13870933lff.82.1586195829688; Mon, 06 Apr 2020 10:57:09 -0700 (PDT) Received: from [192.168.8.100] (user-5-173-33-152.play-internet.pl. [5.173.33.152]) by smtp.gmail.com with ESMTPSA id c4sm5202617lfk.11.2020.04.06.10.57.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Apr 2020 10:57:09 -0700 (PDT) To: jerinj@marvell.com, Kiran Kumar K Cc: dev@dpdk.org, thomas@monjalon.net, david.marchand@redhat.com, mdr@ashroe.eu, mattias.ronnblom@ericsson.com, pbhagavatula@marvell.com, ndabilpuram@marvell.com, xiao.w.wang@intel.com References: <20200331192945.2466880-1-jerinj@marvell.com> <20200405085613.1336841-1-jerinj@marvell.com> <20200405085613.1336841-4-jerinj@marvell.com> From: Andrzej Ostruszka Message-ID: Date: Mon, 6 Apr 2020 19:57:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200405085613.1336841-4-jerinj@marvell.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 03/29] graph: implement node operations X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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); > + > + /* 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. > + > + 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? > + > + 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