From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH] ethdev: fix wrong memset Date: Mon, 23 Jan 2017 13:09:24 +0000 Message-ID: <5a383807-dc74-5a17-d09c-1fa6c88d2333@intel.com> References: <4d897cf9-f1f4-d924-10cd-63e95b12b411@intel.com> <20170122024529.GZ10293@yliu-dev.sh.intel.com> <3451afa6-12fb-dc65-f379-873facc0301c@intel.com> <20170123103417.GB10293@yliu-dev.sh.intel.com> <53a23156-dcb9-b41f-c27c-5bd13d5874f6@intel.com> <20170123112445.GE10293@yliu-dev.sh.intel.com> <90752e37-444b-e2bf-6d4b-1bf2eda38deb@intel.com> <20170123114050.GF10293@yliu-dev.sh.intel.com> <20170123115610.GG10293@yliu-dev.sh.intel.com> <2601191342CEEE43887BDE71AB9772583F10A80C@irsmsx105.ger.corp.intel.com> <20170123125256.GH10293@yliu-dev.sh.intel.com> <2601191342CEEE43887BDE71AB9772583F10A841@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" , Thomas Monjalon , "Horton, Remy" To: "Ananyev, Konstantin" , Yuanhan Liu Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id ACDA02C38 for ; Mon, 23 Jan 2017 14:09:27 +0100 (CET) In-Reply-To: <2601191342CEEE43887BDE71AB9772583F10A841@irsmsx105.ger.corp.intel.com> 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 1/23/2017 1:06 PM, Ananyev, Konstantin wrote: > > >> -----Original Message----- >> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] >> Sent: Monday, January 23, 2017 12:53 PM >> To: Ananyev, Konstantin >> Cc: Yigit, Ferruh ; dev@dpdk.org; Thomas Monjalon ; Horton, Remy >> >> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix wrong memset >> >> On Mon, Jan 23, 2017 at 12:44:11PM +0000, Ananyev, Konstantin wrote: >>>> On Mon, Jan 23, 2017 at 07:40:50PM +0800, Yuanhan Liu wrote: >>>>> On Mon, Jan 23, 2017 at 11:32:23AM +0000, Ferruh Yigit wrote: >>>>>> On 1/23/2017 11:24 AM, Yuanhan Liu wrote: >>>>>>> On Mon, Jan 23, 2017 at 11:05:25AM +0000, Ferruh Yigit wrote: >>>>>>>>>>>>>> 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 4790faf..61f44e2 100644 >>>>>>>>>>>>>> --- a/lib/librte_ether/rte_ethdev.c >>>>>>>>>>>>>> +++ b/lib/librte_ether/rte_ethdev.c >>>>>>>>>>>>>> @@ -225,7 +225,7 @@ struct rte_eth_dev * >>>>>>>>>>>>>> return NULL; >>>>>>>>>>>>>> } >>>>>>>>>>>>>> >>>>>>>>>>>>>> - memset(&rte_eth_devices[port_id], 0, sizeof(*eth_dev->data)); >>>>>>>>>>>>>> + memset(&rte_eth_dev_data[port_id], 0, sizeof(struct rte_eth_dev_data)); >>>>>>>>>>>>> >>>>>>>>>>>>> Not directly related to the this issue, but, after fix, this may have >>>>>>>>>>>>> issues with secondary process. >>>>>>>>>>>>> >>>>>>>>>>>>> There were patches sent to fix this. >>>>>>>>>>>> >>>>>>>>>>>> I mean this one: >>>>>>>>>>>> http://dpdk.org/ml/archives/dev/2017-January/054422.html >>>>>>>>>>> >>>>>>>>>>> d948f596fee2 ("ethdev: fix port data mismatched in multiple process >>>>>>>>>>> model") should have fixed it. >>>>>>>>>> >>>>>>>>>> Think about case, where secondary process uses a virtual PMD, which does >>>>>>>>>> a rte_eth_dev_allocate() call, shouldn't this corrupt primary process >>>>>>>>>> device data? >>>>>>>>> >>>>>>>>> Yes, it may. However, I doubt that's the typical usage. >>>>>>>> >>>>>>>> But this is a use case, and broken now, >>>>>>> >>>>>>> I thought it was broken since the beginning? >>>>>> >>>>>> No, memset(&rte_eth_dev_data[port_id], ...) breaks it. >>>>> >>>>> Oh, you were talking about that particular case Remy's patch meant to >>>>> fix. >>>>> >>>>>>>> and fix is known. >>>>>>> >>>>>>> And there is already a fix? >>>>>> >>>>>> http://dpdk.org/ml/archives/dev/2017-January/054422.html >>>>> >>>>> Yes, it should fix that issue. >>>> >>>> Well, few more thoughts: it may fix the crash issue Remy saw, but it >>>> looks like more a workaround to me. Basically, if primary and secondary >>>> shares a same port id, they should point to same device. Otherwise, >>>> primary process may use eth_dev->data for a device A, while the >>>> secondary process may use it for another device, as you said, it >>>> could be a vdev. >>>> >>>> In such case, there is no way we could continue safely. That said, >>>> the given patch avoids the total reset of eth_dev->data, while it >>>> continues reset the eth_dev->data->name, which is wrong. >>>> >>>> So it's not a proper fix. >>>> >>>> Again, I think it's more about the usage. If primary starts with >>>> a nic device A, while the secondary starts with a nic device B, >>>> there is no way they could work well (unless they use different >>>> port id). >>> >>> Why not? >>> I think this is possible. >> >> Yes, it's possible: find another port id if that one is already taken >> by primary process (or even by secondary process: think that primary >> process might attatch a port later). >> >>> They just need to be initialized properly, >>> so each rte_eth_devices[port_id]->data, etc. point to the right place. >> >> My understanding is, as far as they use different port_id, it might >> be fine. Just not sure it's enough or not. > > As I understand, the main problem is that rte_eth_devices[] is local, > while rte_eth_dev_data points to the shared memory array. > And rte_eth_dev_allocate() assumes that if rte_eth_devices[x] is free, > then rte_eth_dev_data[port_id] is also free. > Which is wrong in case when primary/secondary processes have different devices attached. > Another problem is that inside rte_ethdev.c we manipulate rte_eth_dev_data[] > contents without grabbing any lock. > I think it was an attempt to fix that issue in 16.07 timeframe or so, > but I don't remember what happened with that patch. Same here, I remember this already discussed and even some patches sent, by Reshma if I remember correctly, but I don't remember latest status. > Konstantin > > > >