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 E4D3F1B610 for ; Wed, 27 Jun 2018 05:35:31 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jun 2018 20:35:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,277,1526367600"; d="scan'208";a="70306349" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga002.jf.intel.com with ESMTP; 26 Jun 2018 20:35:30 -0700 Received: from fmsmsx125.amr.corp.intel.com (10.18.125.40) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Jun 2018 20:35:30 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX125.amr.corp.intel.com (10.18.125.40) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 26 Jun 2018 20:35:29 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.51]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.116]) with mapi id 14.03.0319.002; Wed, 27 Jun 2018 11:35:27 +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: AQHUDUPiO2Tmi4unBkmm43HTlqDjCaRygFFggAAIlQD//7MsgIABGLpg Date: Wed, 27 Jun 2018 03:35:27 +0000 Message-ID: <039ED4275CED7440929022BC67E706115323EFE5@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> <039ED4275CED7440929022BC67E706115323E812@SHSMSX103.ccr.corp.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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzAxYTY4MjctNzBhYi00ZTc1LWI2ODItMjJiNDg3MmJjMDg3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiZHVJUUJmbWE0ZTE4bDc4dW5NQ3BXVUpmaXVJZmRKejRFXC9BRmtXYmJybXg5UTI3eDBqeEh0dWVNY3pSZm1vYUIifQ== 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: Wed, 27 Jun 2018 03:35:33 -0000 > -----Original Message----- > From: Matan Azrad [mailto:matan@mellanox.com] > Sent: Wednesday, June 27, 2018 12:55 AM > 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 Zhang >=20 > From: Zhang, Qi Z > > Sent: Tuesday, June 26, 2018 4:30 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 > > > > > > > > > -----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 > > > > > > 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 > > > > > > > > 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 to > > > reset. > > > > > > OK, will remove this. > > > > > > > > > > > > + 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? > > > > > > 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 true for all drivers. >=20 > rte_eth_dev_release_port() like detach port should be called only by the = port > owner, by default is the application, so it should not be called by the P= MD, > maybe need to fix PMDs which does it. Agree, release_port is better only be handled by ethdev layer But Just did a "git grep", seems rte_eth_dev_release_port is used by most P= MDs :) I don't know much about the background, not sure if there is some other rea= son we need this in PMD. There could be separate task to cleanup this. >=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. > > > > > > 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 > This solution is suffering from much more complexity, The port id should = be > the same in all the processes, so you can pass it by the mp channel and n= ot use > rte_eth_dev_allocated. >=20 > I see it like this: >=20 > The initiator process asks from all the other processes to detach a port = by port > id, all the other processes using rte_eth_dev_release_port_local and send= ing > ack back to the initiator, Then, the initiator using the non-local releas= e to > release the port. yes, that will be a solution, but if we are able to figure out which releas= e_port API should be called in rte_eth_dev_detach scenario, we do don't need more IPC here. I will add some condition check in rte_eth_dev_detach to make sure correct = release port API be invoked so there will not be the case that rte_eth_dev_= release_port will not be invoked after rte_eth_dev_release_port_private, An= d previous concerned change can be removed. Thanks Qi >=20 > Am I missing something? >=20 >=20 > > > > > > Regards > > > Qi > > > > > > > > > > > > + 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 proces= s. > > > > > + * > > > > > + * @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 th= e user > > > > > * application to reconfigure it. It is for internal use only. > > > > > * > > > > > -- > > > > > 2.13.6