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

[Zhang, Qi Z]

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.

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

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