From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id ED3987F0C; Mon, 5 Mar 2018 16:06:39 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Mar 2018 07:06:38 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,427,1515484800"; d="scan'208";a="180012317" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.62]) ([10.237.221.62]) by orsmga004.jf.intel.com with ESMTP; 05 Mar 2018 07:06:36 -0800 To: Matan Azrad , Thomas Monjalon , Gaetan Rivet , Jingjing Wu Cc: "dev@dpdk.org" , Neil Horman , Bruce Richardson , Konstantin Ananyev , "stable@dpdk.org" 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> From: Ferruh Yigit Message-ID: Date: Mon, 5 Mar 2018 15:06:35 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/7] ethdev: fix port data reset timing 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: , X-List-Received-Date: Mon, 05 Mar 2018 15:06:40 -0000 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? >