[v15,1/7] ethdev: add function to release port in secondary process
Checks
Commit Message
Add driver API rte_eth_release_port_secondary to support the
case when an ethdev need to be detached on a secondary process.
Local state is set to unused and shared data will not be reset
so the primary process can still use it.
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
lib/librte_ethdev/rte_ethdev.c | 17 +++++++++++++++--
lib/librte_ethdev/rte_ethdev_driver.h | 16 +++++++++++++++-
lib/librte_ethdev/rte_ethdev_pci.h | 10 ++++++++--
lib/librte_ethdev/rte_ethdev_version.map | 7 +++++++
4 files changed, 45 insertions(+), 5 deletions(-)
Comments
On 16.08.2018 06:04, Qi Zhang wrote:
> Add driver API rte_eth_release_port_secondary to support the
> case when an ethdev need to be detached on a secondary process.
> Local state is set to unused and shared data will not be reset
> so the primary process can still use it.
There are few questions below, but in general I'm really puzzled
after looking at variety of release, destroy etc functions and how
call chains look like.
IMHO, introduction of the function is really wrong direction.
It duplicates part of rte_eth_dev_release_port() functionality,
it will complicate maintenance since it will be required to remember
to find and update both. Also it looks like it already has bugs
(missing init of shared data, missing lock).
I would prefer to update rte_eth_dev_release_port() to make it
secondary process aware.
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> lib/librte_ethdev/rte_ethdev.c | 17 +++++++++++++++--
> lib/librte_ethdev/rte_ethdev_driver.h | 16 +++++++++++++++-
> lib/librte_ethdev/rte_ethdev_pci.h | 10 ++++++++--
> lib/librte_ethdev/rte_ethdev_version.map | 7 +++++++
> 4 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 4c3202505..1a1cc1125 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -359,6 +359,18 @@ rte_eth_dev_attach_secondary(const char *name)
> }
>
> int
> +rte_eth_dev_release_port_secondary(struct rte_eth_dev *eth_dev)
> +{
> + if (eth_dev == NULL)
> + return -EINVAL;
> +
> + _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY, NULL);
> + eth_dev->state = RTE_ETH_DEV_UNUSED;
rte_eth_dev_release_port() does it under ownership lock.
Why is lock not required here?
> +
> + return 0;
> +}
> +
> +int
> rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
> {
> if (eth_dev == NULL)
> @@ -3532,9 +3544,10 @@ rte_eth_dev_destroy(struct rte_eth_dev *ethdev,
> return ret;
> }
>
> - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> - rte_free(ethdev->data->dev_private);
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + return rte_eth_dev_release_port_secondary(ethdev);
>
> + rte_free(ethdev->data->dev_private);
> ethdev->data->dev_private = NULL;
>
> return rte_eth_dev_release_port(ethdev);
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index c6d9bc1a3..8fe82d2ab 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -61,7 +61,7 @@ struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
> * Release the specified ethdev port.
> *
> * @param eth_dev
> - * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.
> + * Device to be detached.
> * @return
> * - 0 on success, negative on error
> */
> @@ -69,6 +69,20 @@ int rte_eth_dev_release_port(struct rte_eth_dev *eth_dev);
>
> /**
> * @internal
> + * Release the specified ethdev port in the local process.
> + * Only set ethdev state to unused, but not reset shared data since
> + * it assume other processes is still using it. typically it is
> + * called by a secondary process.
> + *
> + * @param eth_dev
> + * Device to be detached.
> + * @return
> + * - 0 on success, negative on error
> + */
> +int rte_eth_dev_release_port_secondary(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.
> *
> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> index f652596f4..70d2d2503 100644
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -135,9 +135,15 @@ rte_eth_dev_pci_allocate(struct rte_pci_device *dev, size_t private_data_size)
> static inline void
> rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev)
> {
> - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> - rte_free(eth_dev->data->dev_private);
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> + eth_dev->device = NULL;
> + eth_dev->intr_handle = NULL;
Why are above two assignments done here in the PCI device release,
but not included in rte_eth_dev_release_port_secondary()?
> + rte_eth_dev_release_port_secondary(eth_dev);
> + return;
> + }
>
> + /* primary process */
> + rte_free(eth_dev->data->dev_private);
> eth_dev->data->dev_private = NULL;
>
> /*
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 38f117f01..acc407f86 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -220,6 +220,13 @@ DPDK_18.08 {
>
> } DPDK_18.05;
>
> +DPDK_18.11 {
> + global:
> +
> + rte_eth_dev_release_port_secondary;
> +
> +} DPDK_18.08;
Shouldn't it be experimental?
> +
> EXPERIMENTAL {
> global:
>
> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Monday, August 20, 2018 4:53 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net;
> gaetan.rivet@6wind.com; 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: [PATCH v15 1/7] ethdev: add function to release port in
> secondary process
>
> On 16.08.2018 06:04, Qi Zhang wrote:
> > Add driver API rte_eth_release_port_secondary to support the case when
> > an ethdev need to be detached on a secondary process.
> > Local state is set to unused and shared data will not be reset so the
> > primary process can still use it.
>
> There are few questions below, but in general I'm really puzzled after looking
> at variety of release, destroy etc functions and how call chains look like.
>
> IMHO, introduction of the function is really wrong direction.
> It duplicates part of rte_eth_dev_release_port() functionality, it will
> complicate maintenance since it will be required to remember to find and
> update both. Also it looks like it already has bugs (missing init of shared data,
> missing lock).
>
> I would prefer to update rte_eth_dev_release_port() to make it secondary
> process aware.
Well, this is on purpose to pair with rte_eth_dev_attach_secondary which is also a dedicate function for
secondary process. So we have
in driver->probe: use rte_eth_dev_attach_secondary to attach an already registered port by primary to secondary
In driver->remove: use rte_eth_dev_release_port_secondary to detach the port from secondary and the input parameter is exactly the return value of previous.)
>
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> > lib/librte_ethdev/rte_ethdev.c | 17 +++++++++++++++--
> > lib/librte_ethdev/rte_ethdev_driver.h | 16 +++++++++++++++-
> > lib/librte_ethdev/rte_ethdev_pci.h | 10 ++++++++--
> > lib/librte_ethdev/rte_ethdev_version.map | 7 +++++++
> > 4 files changed, 45 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c
> > b/lib/librte_ethdev/rte_ethdev.c index 4c3202505..1a1cc1125 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -359,6 +359,18 @@ rte_eth_dev_attach_secondary(const char *name)
> > }
> >
> > int
> > +rte_eth_dev_release_port_secondary(struct rte_eth_dev *eth_dev) {
> > + if (eth_dev == NULL)
> > + return -EINVAL;
> > +
> > + _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY,
> NULL);
> > + eth_dev->state = RTE_ETH_DEV_UNUSED;
>
> rte_eth_dev_release_port() does it under ownership lock.
Yes, because on primary process, it is possible that another thread is going to attach a new port which will also access the dev state and shared data.
> Why is lock not required here?
It's not necessary, since for secondary process, we only attach a port already be registered on primary, there will be no concurrent issue.
>
> > +
> > + return 0;
> > +}
> > +
> > +int
> > rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
> > {
> > if (eth_dev == NULL)
> > @@ -3532,9 +3544,10 @@ rte_eth_dev_destroy(struct rte_eth_dev
> *ethdev,
> > return ret;
> > }
> >
> > - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > - rte_free(ethdev->data->dev_private);
> > + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > + return rte_eth_dev_release_port_secondary(ethdev);
> >
> > + rte_free(ethdev->data->dev_private);
> > ethdev->data->dev_private = NULL;
> >
> > return rte_eth_dev_release_port(ethdev); diff --git
> > a/lib/librte_ethdev/rte_ethdev_driver.h
> > b/lib/librte_ethdev/rte_ethdev_driver.h
> > index c6d9bc1a3..8fe82d2ab 100644
> > --- a/lib/librte_ethdev/rte_ethdev_driver.h
> > +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> > @@ -61,7 +61,7 @@ struct rte_eth_dev
> *rte_eth_dev_attach_secondary(const char *name);
> > * Release the specified ethdev port.
> > *
> > * @param eth_dev
> > - * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.
> > + * Device to be detached.
> > * @return
> > * - 0 on success, negative on error
> > */
> > @@ -69,6 +69,20 @@ int rte_eth_dev_release_port(struct rte_eth_dev
> > *eth_dev);
> >
> > /**
> > * @internal
> > + * Release the specified ethdev port in the local process.
> > + * Only set ethdev state to unused, but not reset shared data since
> > + * it assume other processes is still using it. typically it is
> > + * called by a secondary process.
> > + *
> > + * @param eth_dev
> > + * Device to be detached.
> > + * @return
> > + * - 0 on success, negative on error
> > + */
> > +int rte_eth_dev_release_port_secondary(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.
> > *
> > diff --git a/lib/librte_ethdev/rte_ethdev_pci.h
> > b/lib/librte_ethdev/rte_ethdev_pci.h
> > index f652596f4..70d2d2503 100644
> > --- a/lib/librte_ethdev/rte_ethdev_pci.h
> > +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> > @@ -135,9 +135,15 @@ rte_eth_dev_pci_allocate(struct rte_pci_device
> *dev, size_t private_data_size)
> > static inline void
> > rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev)
> > {
> > - if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > - rte_free(eth_dev->data->dev_private);
> > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > + eth_dev->device = NULL;
> > + eth_dev->intr_handle = NULL;
>
> Why are above two assignments done here in the PCI device release, but not
> included in rte_eth_dev_release_port_secondary()?
Since they are not in rte_eth_dev_release_port, so I keep them out of rte_eth_dev_release_port_secondary simply.
Probably there could be some consolidation, but should not for rte_eth_dev_release_port_secondary only from my view.
>
> > + rte_eth_dev_release_port_secondary(eth_dev);
> > + return;
> > + }
> >
> > + /* primary process */
> > + rte_free(eth_dev->data->dev_private);
> > eth_dev->data->dev_private = NULL;
> >
> > /*
> > diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> > b/lib/librte_ethdev/rte_ethdev_version.map
> > index 38f117f01..acc407f86 100644
> > --- a/lib/librte_ethdev/rte_ethdev_version.map
> > +++ b/lib/librte_ethdev/rte_ethdev_version.map
> > @@ -220,6 +220,13 @@ DPDK_18.08 {
> >
> > } DPDK_18.05;
> >
> > +DPDK_18.11 {
> > + global:
> > +
> > + rte_eth_dev_release_port_secondary;
> > +
> > +} DPDK_18.08;
>
> Shouldn't it be experimental?
This API is a help function for PMD only which is defined in rte_ethdev_driver.h, it is not for external use.
Regards
Qi
@@ -359,6 +359,18 @@ rte_eth_dev_attach_secondary(const char *name)
}
int
+rte_eth_dev_release_port_secondary(struct rte_eth_dev *eth_dev)
+{
+ if (eth_dev == NULL)
+ return -EINVAL;
+
+ _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY, NULL);
+ eth_dev->state = RTE_ETH_DEV_UNUSED;
+
+ return 0;
+}
+
+int
rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
{
if (eth_dev == NULL)
@@ -3532,9 +3544,10 @@ rte_eth_dev_destroy(struct rte_eth_dev *ethdev,
return ret;
}
- if (rte_eal_process_type() == RTE_PROC_PRIMARY)
- rte_free(ethdev->data->dev_private);
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_secondary(ethdev);
+ rte_free(ethdev->data->dev_private);
ethdev->data->dev_private = NULL;
return rte_eth_dev_release_port(ethdev);
@@ -61,7 +61,7 @@ struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
* Release the specified ethdev port.
*
* @param eth_dev
- * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.
+ * Device to be detached.
* @return
* - 0 on success, negative on error
*/
@@ -69,6 +69,20 @@ int rte_eth_dev_release_port(struct rte_eth_dev *eth_dev);
/**
* @internal
+ * Release the specified ethdev port in the local process.
+ * Only set ethdev state to unused, but not reset shared data since
+ * it assume other processes is still using it. typically it is
+ * called by a secondary process.
+ *
+ * @param eth_dev
+ * Device to be detached.
+ * @return
+ * - 0 on success, negative on error
+ */
+int rte_eth_dev_release_port_secondary(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.
*
@@ -135,9 +135,15 @@ rte_eth_dev_pci_allocate(struct rte_pci_device *dev, size_t private_data_size)
static inline void
rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev)
{
- if (rte_eal_process_type() == RTE_PROC_PRIMARY)
- rte_free(eth_dev->data->dev_private);
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+ eth_dev->device = NULL;
+ eth_dev->intr_handle = NULL;
+ rte_eth_dev_release_port_secondary(eth_dev);
+ return;
+ }
+ /* primary process */
+ rte_free(eth_dev->data->dev_private);
eth_dev->data->dev_private = NULL;
/*
@@ -220,6 +220,13 @@ DPDK_18.08 {
} DPDK_18.05;
+DPDK_18.11 {
+ global:
+
+ rte_eth_dev_release_port_secondary;
+
+} DPDK_18.08;
+
EXPERIMENTAL {
global: