[dpdk-dev] [PATCH v4 03/24] ethdev: add function to release port in local process

[Zhang, Qi Z]

> -----Original Message-----
> From: Matan Azrad [mailto:matan at mellanox.com]
> Sent: Wednesday, June 27, 2018 12:55 AM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>; Thomas Monjalon
> <thomas at monjalon.net>; Burakov, Anatoly <anatoly.burakov at intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org;
> Richardson, Bruce <bruce.richardson at intel.com>; Yigit, Ferruh
> <ferruh.yigit at intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton at intel.com>; Vangati, Narender
> <narender.vangati at intel.com>
> Subject: RE: [dpdk-dev] [PATCH v4 03/24] ethdev: add function to release port
> in local process
> 
> Hi Zhang
> 
>  From: Zhang, Qi Z
> > Sent: Tuesday, June 26, 2018 4:30 PM
> > To: Matan Azrad <matan at mellanox.com>; Thomas Monjalon
> > <thomas at monjalon.net>; Burakov, Anatoly <anatoly.burakov at intel.com>
> > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org;
> > Richardson, Bruce <bruce.richardson at intel.com>; Yigit, Ferruh
> > <ferruh.yigit at intel.com>; Shelton, Benjamin H
> > <benjamin.h.shelton at intel.com>; Vangati, Narender
> > <narender.vangati at intel.com>
> > 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' <matan at mellanox.com>; Thomas Monjalon
> > > <thomas at monjalon.net>; Burakov, Anatoly <anatoly.burakov at intel.com>
> > > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>;
> > > dev at dpdk.org; Richardson, Bruce <bruce.richardson at intel.com>; Yigit,
> > > Ferruh <ferruh.yigit at intel.com>; Shelton, Benjamin H
> > > <benjamin.h.shelton at intel.com>; Vangati, Narender
> > > <narender.vangati at intel.com>
> > > 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 at mellanox.com]
> > > > Sent: Tuesday, June 26, 2018 7:50 PM
> > > > To: Zhang, Qi Z <qi.z.zhang at intel.com>; Thomas Monjalon
> > > > <thomas at monjalon.net>; Burakov, Anatoly
> > > > <anatoly.burakov at intel.com>
> > > > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>;
> > > > dev at dpdk.org; Richardson, Bruce <bruce.richardson at intel.com>;
> > > > Yigit, Ferruh <ferruh.yigit at intel.com>; Shelton, Benjamin H
> > > > <benjamin.h.shelton at intel.com>; Vangati, Narender
> > > > <narender.vangati at intel.com>
> > > > 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 <qi.z.zhang at intel.com>
> > > > > ---
> > > > >
> > > > >  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 == 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 = 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 == 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 = RTE_ETH_DEV_UNUSED;
> > > > > -
> > > > > -	memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
> > > > > +	if (eth_dev->state != 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.
> 
> 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 PMD,
> 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 PMDs :)
I don't know much about the background, not sure if there is some other reason
we need this in PMD.

There could be separate task to cleanup this.

> 
> > >
> > > 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
> 
> 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 not use
> rte_eth_dev_allocated.
> 
> I see it like this:
> 
> 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 sending
> ack back to the initiator, Then, the initiator using the non-local release to
> release the port.

yes, that will be a solution, but if we are able to figure out which release_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, And previous concerned change can be removed.

Thanks
Qi

> 
> Am I missing something?
> 
> 
> > >
> > > Regards
> > > Qi
> > >
> > > >
> > > > > +		eth_dev->state = 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