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

Matan Azrad matan at mellanox.com
Tue Jun 26 18:54:33 CEST 2018


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.

> >
> > 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.
 
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



More information about the dev mailing list