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 B02D21B5DC for ; Tue, 26 Jun 2018 15:28:52 +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:28:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,274,1526367600"; d="scan'208";a="211271540" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga004.jf.intel.com with ESMTP; 26 Jun 2018 06:28:36 -0700 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) 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:28:35 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Jun 2018 06:28:35 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.51]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.70]) with mapi id 14.03.0319.002; Tue, 26 Jun 2018 21:28:33 +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: AQHUDUPiO2Tmi4unBkmm43HTlqDjCaRygFFg Date: Tue, 26 Jun 2018 13:28:32 +0000 Message-ID: <039ED4275CED7440929022BC67E706115323E7FB@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> In-Reply-To: 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:28:53 -0000 Hi Matan: > -----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 >=20 > Hi Qi >=20 > Please see comments\questions.. >=20 > 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 proces= s 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); >=20 > I don't think you need the lock here because there is not shared data to = reset. 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) { >=20 > Can you explain why not to release the shared data in case of locally unu= sed > port? Now, we have rte_eth_dev_release_port be called in rte_eth_dev_detach, its = redundant for some driver, because they already will release port in dev->remove, but I'm not sure if it is tr= ue for all drivers. On secondary process, when detach a device, it is possible that=20 rte_eth_dev_release_port be called after rte_eth_dev_release_port_local , s= o 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. There could be other way, but current implementation help to reuse exist co= de., I can't add more comment on this change in v5 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* structure= . > > + * @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 user > > * application to reconfigure it. It is for internal use only. > > * > > -- > > 2.13.6