From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 0C6351B5B8 for ; Tue, 26 Jun 2018 15:30:51 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jun 2018 06:30:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,274,1526367600"; d="scan'208";a="211272087" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga004.jf.intel.com with ESMTP; 26 Jun 2018 06:30:10 -0700 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Jun 2018 06:30:09 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Jun 2018 06:30:08 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.51]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.82]) with mapi id 14.03.0319.002; Tue, 26 Jun 2018 21:30:06 +0800 From: "Zhang, Qi Z" To: Matan Azrad , Thomas Monjalon , "Burakov, Anatoly" CC: "Ananyev, Konstantin" , "dev@dpdk.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "Shelton, Benjamin H" , "Vangati, Narender" Thread-Topic: [dpdk-dev] [PATCH v4 03/24] ethdev: add function to release port in local process Thread-Index: AQHUDUPiO2Tmi4unBkmm43HTlqDjCaRygFFggAAIlQA= Date: Tue, 26 Jun 2018 13:30:05 +0000 Message-ID: <039ED4275CED7440929022BC67E706115323E812@SHSMSX103.ccr.corp.intel.com> References: <20180607123849.14439-1-qi.z.zhang@intel.com> <20180626070832.3055-1-qi.z.zhang@intel.com> <20180626070832.3055-4-qi.z.zhang@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDJmOWU2MGQtNjY0YS00MDM5LTg4OGQtODVjMWJhZDE5M2U5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiV2VrVVRFUjNCVm54RWc5QWFQVHp4Yk1VeTBiQ2ZoOWM5RFR0V0JzampFOUthXC9aOXBPTWh4VkplbmVSbHRSVVgifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4 03/24] ethdev: add function to release port in local process 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: Tue, 26 Jun 2018 13:30:52 -0000 > -----Original Message----- > From: Zhang, Qi Z > Sent: Tuesday, June 26, 2018 9:29 PM > To: 'Matan Azrad' ; Thomas Monjalon > ; Burakov, Anatoly > Cc: Ananyev, Konstantin ; dev@dpdk.org; > Richardson, Bruce ; Yigit, Ferruh > ; Shelton, Benjamin H > ; Vangati, Narender > > Subject: RE: [dpdk-dev] [PATCH v4 03/24] ethdev: add function to release = port > in local process >=20 > Hi Matan: >=20 > > -----Original Message----- > > From: Matan Azrad [mailto:matan@mellanox.com] > > Sent: Tuesday, June 26, 2018 7:50 PM > > To: Zhang, Qi Z ; Thomas Monjalon > > ; Burakov, Anatoly > > Cc: Ananyev, Konstantin ; dev@dpdk.org; > > Richardson, Bruce ; Yigit, Ferruh > > ; Shelton, Benjamin H > > ; Vangati, Narender > > > > Subject: RE: [dpdk-dev] [PATCH v4 03/24] ethdev: add function to > > release port in local process > > > > Hi Qi > > > > Please see comments\questions.. > > > > From: Qi Zhang > > > Add driver API rte_eth_release_port_private to support the > > > requirement that an ethdev only be released on secondary process, so > > > only local state be set to unused , share data will not be reset so > > > primary process can > > still use it. > > > > > > Signed-off-by: Qi Zhang > > > --- > > > > > > lib/librte_ethdev/rte_ethdev.c | 24 +++++++++++++++++++++--- > > > lib/librte_ethdev/rte_ethdev_driver.h | 13 +++++++++++++ > > > 2 files changed, 34 insertions(+), 3 deletions(-) > > > > > > diff --git a/lib/librte_ethdev/rte_ethdev.c > > > b/lib/librte_ethdev/rte_ethdev.c index a9977df97..205b2ee33 100644 > > > --- a/lib/librte_ethdev/rte_ethdev.c > > > +++ b/lib/librte_ethdev/rte_ethdev.c > > > @@ -359,6 +359,23 @@ rte_eth_dev_attach_secondary(const char > > *name) } > > > > > > int > > > +rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev) { > > > + if (eth_dev =3D=3D NULL) > > > + return -EINVAL; > > > + > > > + _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY, > > > NULL); > > > + > > > + rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock); > > > > I don't think you need the lock here because there is not shared data t= o > reset. >=20 > OK, will remove this. >=20 > > > > > + eth_dev->state =3D RTE_ETH_DEV_UNUSED; > > > + > > > + rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock); > > > + > > > + return 0; > > > +} > > > + > > > +int > > > rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) { > > > if (eth_dev =3D=3D NULL) > > > @@ -370,9 +387,10 @@ rte_eth_dev_release_port(struct rte_eth_dev > > > *eth_dev) > > > > > > rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock); > > > > > > - eth_dev->state =3D RTE_ETH_DEV_UNUSED; > > > - > > > - memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data)); > > > + if (eth_dev->state !=3D RTE_ETH_DEV_UNUSED) { > > > > Can you explain why not to release the shared data in case of locally > > unused port? >=20 > Now, we have rte_eth_dev_release_port be called in rte_eth_dev_detach, it= s > redundant for some driver, because they already will release port in > dev->remove, but I'm not sure if it is true for all drivers. >=20 > On secondary process, when detach a device, it is possible that > rte_eth_dev_release_port be called after rte_eth_dev_release_port_local ,= so > it will reset share data which is not expected, since the primary process= will fail > on detach it because rte_eth_dev_allocated will return NULL. >=20 > There could be other way, but current implementation help to reuse exist > code., I can't add more comment on this change in v5 I can add more comment >=20 > Regards > Qi >=20 > > > > > + eth_dev->state =3D RTE_ETH_DEV_UNUSED; > > > + memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data)); > > > + } > > > > > > rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock); > > > > > > diff --git a/lib/librte_ethdev/rte_ethdev_driver.h > > > b/lib/librte_ethdev/rte_ethdev_driver.h > > > index c9c825e3f..49c27223d 100644 > > > --- a/lib/librte_ethdev/rte_ethdev_driver.h > > > +++ b/lib/librte_ethdev/rte_ethdev_driver.h > > > @@ -70,6 +70,19 @@ int rte_eth_dev_release_port(struct rte_eth_dev > > > *eth_dev); > > > > > > /** > > > * @internal > > > + * Release the specified ethdev port in local process, only set to > > > +ethdev > > > + * state to unused, but not reset share data since it assume other > > > +process > > > + * is still using it, typically it is called by secondary process. > > > + * > > > + * @param eth_dev > > > + * The *eth_dev* pointer is the address of the *rte_eth_dev* structu= re. > > > + * @return > > > + * - 0 on success, negative on error > > > + */ > > > +int rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev); > > > + > > > +/** > > > + * @internal > > > * Release device queues and clear its configuration to force the us= er > > > * application to reconfigure it. It is for internal use only. > > > * > > > -- > > > 2.13.6