From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [dpdk-stable] [PATCH v3 1/7] ethdev: fix port data reset timing Date: Mon, 5 Mar 2018 15:06:35 +0000 Message-ID: References: <1515318351-4756-1-git-send-email-matan@mellanox.com> <1516293317-30748-1-git-send-email-matan@mellanox.com> <1516293317-30748-2-git-send-email-matan@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" , Neil Horman , Bruce Richardson , Konstantin Ananyev , "stable@dpdk.org" To: Matan Azrad , Thomas Monjalon , Gaetan Rivet , Jingjing Wu Return-path: In-Reply-To: Content-Language: en-US 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 3/5/2018 2:52 PM, Matan Azrad wrote: > HI > > From: Ferruh Yigit, Sent: Monday, March 5, 2018 1:24 PM >> On 1/18/2018 4:35 PM, Matan Azrad wrote: >>> rte_eth_dev_data structure is allocated per ethdev port and can be >>> used to get a data of the port internally. >>> >>> rte_eth_dev_attach_secondary tries to find the port identifier using >>> rte_eth_dev_data name field comparison and may get an identifier of >>> invalid port in case of this port was released by the primary process >>> because the port release API doesn't reset the port data. >>> >>> So, it will be better to reset the port data in release time instead >>> of allocation time. >>> >>> Move the port data reset to the port release API. >>> >>> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple >>> process model") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Matan Azrad >>> --- >>> lib/librte_ether/rte_ethdev.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/librte_ether/rte_ethdev.c >>> b/lib/librte_ether/rte_ethdev.c index 7044159..156231c 100644 >>> --- a/lib/librte_ether/rte_ethdev.c >>> +++ b/lib/librte_ether/rte_ethdev.c >>> @@ -204,7 +204,6 @@ struct rte_eth_dev * >>> return NULL; >>> } >>> >>> - memset(&rte_eth_dev_data[port_id], 0, sizeof(struct >> rte_eth_dev_data)); >>> eth_dev = eth_dev_get(port_id); >>> snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), >> "%s", name); >>> eth_dev->data->port_id = port_id; >>> @@ -252,6 +251,7 @@ struct rte_eth_dev * >>> if (eth_dev == NULL) >>> return -EINVAL; >>> >>> + memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data)); >> >> Hi Matan, >> >> What most of the vdev release path does is: >> >> eth_dev = rte_eth_dev_allocated(...) >> rte_free(eth_dev->data->dev_private); >> rte_free(eth_dev->data); >> rte_eth_dev_release_port(eth_dev); >> >> Since eth_dev->data freed, memset() it in rte_eth_dev_release_port() will >> be problem. >> >> We don't run remove path that is why we didn't hit the issue but this seems >> problem for all virtual PMDs. > > Yes, it is a problem and should be fixed: > For vdevs which use private rte_eth_dev_data the remove order can be: > private_data = eth_dev->data; > rte_free(eth_dev->data->dev_private); > rte_eth_dev_release_port(eth_dev); /* The last operation working on ethdev structure. */ > rte_free(private_data); Do we need to save "private_data"? > > >> Also rte_eth_dev_pci_release() looks problematic now. > > Yes, again, the last operation working on ethdev structure should be rte_eth_dev_release_port(). > > So need to fix all vdevs and the rte_eth_dev_pci_release() function. > > Any comments? >