[v4,03/24] ethdev: add function to release port in local process
Checks
Commit Message
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@intel.com>
---
lib/librte_ethdev/rte_ethdev.c | 24 +++++++++++++++++++++---
lib/librte_ethdev/rte_ethdev_driver.h | 13 +++++++++++++
2 files changed, 34 insertions(+), 3 deletions(-)
Comments
On 26/06/2018 08:08, Qi Zhang wrote:
[..]
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>
> lib/librte_ethdev/rte_ethdev.c | 24 +++++++++++++++++++++---
> lib/librte_ethdev/rte_ethdev_driver.h | 13 +++++++++++++
> 2 files changed, 34 insertions(+), 3 deletions(-)
Acked-by: Remy Horton <remy.horton@intel.com>
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@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.
> + 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?
> + 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
Hi Matan:
> -----Original Message-----
> From: Matan Azrad [mailto:matan@mellanox.com]
> Sent: Tuesday, June 26, 2018 7:50 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@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@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
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, June 26, 2018 9:29 PM
> To: 'Matan Azrad' <matan@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@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@mellanox.com]
> > Sent: Tuesday, June 26, 2018 7:50 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> > Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Shelton, Benjamin H
> > <benjamin.h.shelton@intel.com>; Vangati, Narender
> > <narender.vangati@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@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
I can add more comment
>
> 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
Hi Zhang
From: Zhang, Qi Z
> Sent: Tuesday, June 26, 2018 4:30 PM
> To: Matan Azrad <matan@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@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@mellanox.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> > Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Shelton, Benjamin H
> > <benjamin.h.shelton@intel.com>; Vangati, Narender
> > <narender.vangati@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@mellanox.com]
> > > Sent: Tuesday, June 26, 2018 7:50 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Yigit,
> > > Ferruh <ferruh.yigit@intel.com>; Shelton, Benjamin H
> > > <benjamin.h.shelton@intel.com>; Vangati, Narender
> > > <narender.vangati@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@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
> -----Original Message-----
> From: Matan Azrad [mailto:matan@mellanox.com]
> Sent: Wednesday, June 27, 2018 12:55 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@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@mellanox.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> > Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Shelton, Benjamin H
> > <benjamin.h.shelton@intel.com>; Vangati, Narender
> > <narender.vangati@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@mellanox.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; Burakov, Anatoly <anatoly.burakov@intel.com>
> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Yigit,
> > > Ferruh <ferruh.yigit@intel.com>; Shelton, Benjamin H
> > > <benjamin.h.shelton@intel.com>; Vangati, Narender
> > > <narender.vangati@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@mellanox.com]
> > > > Sent: Tuesday, June 26, 2018 7:50 PM
> > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>; Burakov, Anatoly
> > > > <anatoly.burakov@intel.com>
> > > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > > dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> > > > Yigit, Ferruh <ferruh.yigit@intel.com>; Shelton, Benjamin H
> > > > <benjamin.h.shelton@intel.com>; Vangati, Narender
> > > > <narender.vangati@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@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
@@ -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);
+
+ 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) {
+ 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);
@@ -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.
*