[v11,01/19] ethdev: add function to release port in local process
Checks
Commit Message
Add driver API rte_eth_release_port_private 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>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
Acked-by: Remy Horton <remy.horton@intel.com>
---
lib/librte_ethdev/rte_ethdev.c | 12 ++++++++++++
lib/librte_ethdev/rte_ethdev_driver.h | 16 +++++++++++++++-
lib/librte_ethdev/rte_ethdev_pci.h | 8 ++++++++
3 files changed, 35 insertions(+), 1 deletion(-)
Comments
On 11.07.2018 06:08, Qi Zhang wrote:
> Add driver API rte_eth_release_port_private 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>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Remy Horton <remy.horton@intel.com>
> ---
> lib/librte_ethdev/rte_ethdev.c | 12 ++++++++++++
> lib/librte_ethdev/rte_ethdev_driver.h | 16 +++++++++++++++-
> lib/librte_ethdev/rte_ethdev_pci.h | 8 ++++++++
> 3 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index a9977df97..52a97694c 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_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);
> + 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)
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index c9c825e3f..269586d88 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -62,7 +62,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
> */
> @@ -70,6 +70,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_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.
> *
> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> index 2cfd37274..a46d9e182 100644
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -197,6 +197,14 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev,
> if (!eth_dev)
> return -ENODEV;
>
> + /**
> + * PCI device can only be globally detached directly by a
> + * primary process. In secondary process, we only need to
> + * release port.
> + */
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + return rte_eth_dev_release_port_private(eth_dev);
I've realized that some uninit functions which will not be called anymore
in secondary processes have check for process type and handling of
secondary process case. It makes code inconsistent and should be fixed.
> +
> if (dev_uninit) {
> ret = dev_uninit(eth_dev);
> if (ret)
> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Wednesday, July 11, 2018 5:27 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; 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 v11 01/19] ethdev: add function to release port
> in local process
>
> On 11.07.2018 06:08, Qi Zhang wrote:
> > Add driver API rte_eth_release_port_private 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>
> > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > Acked-by: Remy Horton <remy.horton@intel.com>
> > ---
<...>
> > + /**
> > + * PCI device can only be globally detached directly by a
> > + * primary process. In secondary process, we only need to
> > + * release port.
> > + */
> > + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > + return rte_eth_dev_release_port_private(eth_dev);
>
> I've realized that some uninit functions which will not be called anymore in
> secondary processes have check for process type and handling of secondary
> process case. It makes code inconsistent and should be fixed.
Good point, I did a scan and check all the places that rte_eth_dev_pci_generic_remove be involved.
I found only sfc driver (sfc_eth_dev_unit) will call some cleanup on secondary process as below.
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
sfc_eth_dev_secondary_clear_ops(dev);
return 0;
}
But in sfc_eth_dev_secondary_clear_ops
static void
sfc_eth_dev_secondary_clear_ops(struct rte_eth_dev *dev)
{
dev->dev_ops = NULL;
dev->tx_pkt_burst = NULL;
dev->rx_pkt_burst = NULL;
}
So my understand is current change is not a problem for all exist drivers.
Please let me know if I missed something
Thanks
Qi
>
> > +
> > if (dev_uninit) {
> > ret = dev_uninit(eth_dev);
> > if (ret)
On 11.07.2018 15:30, Zhang, Qi Z wrote:
>
>> -----Original Message-----
>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>> Sent: Wednesday, July 11, 2018 5:27 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; 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 v11 01/19] ethdev: add function to release port
>> in local process
>>
>> On 11.07.2018 06:08, Qi Zhang wrote:
>>> Add driver API rte_eth_release_port_private 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>
>>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Acked-by: Remy Horton <remy.horton@intel.com>
>>> ---
> <...>
>>> + /**
>>> + * PCI device can only be globally detached directly by a
>>> + * primary process. In secondary process, we only need to
>>> + * release port.
>>> + */
>>> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>> + return rte_eth_dev_release_port_private(eth_dev);
>> I've realized that some uninit functions which will not be called anymore in
>> secondary processes have check for process type and handling of secondary
>> process case. It makes code inconsistent and should be fixed.
> Good point, I did a scan and check all the places that rte_eth_dev_pci_generic_remove be involved.
> I found only sfc driver (sfc_eth_dev_unit) will call some cleanup on secondary process as below.
The patch makes impossible dev_uninit to be executed for secondary process
for all cases if rte_eth_dev_pci_generic_remove() is used. However, many
drivers
still check for process type. Yes, sfc does cleanup, but some drivers
return -EPERM,
some return 0. In fact it does not matter. It leaves dead code which is
really confusing.
>
> if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> sfc_eth_dev_secondary_clear_ops(dev);
> return 0;
> }
>
> But in sfc_eth_dev_secondary_clear_ops
>
> static void
> sfc_eth_dev_secondary_clear_ops(struct rte_eth_dev *dev)
> {
> dev->dev_ops = NULL;
> dev->tx_pkt_burst = NULL;
> dev->rx_pkt_burst = NULL;
> }
>
> So my understand is current change is not a problem for all exist drivers.
>
> Please let me know if I missed something
>
> Thanks
> Qi
>
>>> +
>>> if (dev_uninit) {
>>> ret = dev_uninit(eth_dev);
>>> if (ret)
> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Thursday, July 12, 2018 12:05 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; 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 v11 01/19] ethdev: add function to release port
> in local process
>
> On 11.07.2018 15:30, Zhang, Qi Z wrote:
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> >> Sent: Wednesday, July 11, 2018 5:27 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; 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 v11 01/19] ethdev: add function to
> >> release port in local process
> >>
> >> On 11.07.2018 06:08, Qi Zhang wrote:
> >>> Add driver API rte_eth_release_port_private 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>
> >>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>> Acked-by: Remy Horton <remy.horton@intel.com>
> >>> ---
> > <...>
> >>> + /**
> >>> + * PCI device can only be globally detached directly by a
> >>> + * primary process. In secondary process, we only need to
> >>> + * release port.
> >>> + */
> >>> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >>> + return rte_eth_dev_release_port_private(eth_dev);
> >> I've realized that some uninit functions which will not be called
> >> anymore in secondary processes have check for process type and
> >> handling of secondary process case. It makes code inconsistent and should
> be fixed.
> > Good point, I did a scan and check all the places that
> rte_eth_dev_pci_generic_remove be involved.
> > I found only sfc driver (sfc_eth_dev_unit) will call some cleanup on
> secondary process as below.
>
> The patch makes impossible dev_uninit to be executed for secondary process
> for all cases if rte_eth_dev_pci_generic_remove() is used. However, many
> drivers still check for process type. Yes, sfc does cleanup, but some drivers
> return -EPERM, some return 0. In fact it does not matter. It leaves dead code
> which is really confusing.
OK, l can do a cleanup in a separate patchset if this one will be merged.
>
> >
> > if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > sfc_eth_dev_secondary_clear_ops(dev);
> > return 0;
> > }
> >
> > But in sfc_eth_dev_secondary_clear_ops
> >
> > static void
> > sfc_eth_dev_secondary_clear_ops(struct rte_eth_dev *dev) {
> > dev->dev_ops = NULL;
> > dev->tx_pkt_burst = NULL;
> > dev->rx_pkt_burst = NULL;
> > }
> >
> > So my understand is current change is not a problem for all exist drivers.
> >
> > Please let me know if I missed something
> >
> > Thanks
> > Qi
> >
> >>> +
> >>> if (dev_uninit) {
> >>> ret = dev_uninit(eth_dev);
> >>> if (ret)
On 12.07.2018 03:23, Zhang, Qi Z wrote:
>
>> -----Original Message-----
>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>> Sent: Thursday, July 12, 2018 12:05 AM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; 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 v11 01/19] ethdev: add function to release port
>> in local process
>>
>> On 11.07.2018 15:30, Zhang, Qi Z wrote:
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
>>>> Sent: Wednesday, July 11, 2018 5:27 PM
>>>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; 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 v11 01/19] ethdev: add function to
>>>> release port in local process
>>>>
>>>> On 11.07.2018 06:08, Qi Zhang wrote:
>>>>> Add driver API rte_eth_release_port_private 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>
>>>>> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>> Acked-by: Remy Horton <remy.horton@intel.com>
>>>>> ---
>>> <...>
>>>>> + /**
>>>>> + * PCI device can only be globally detached directly by a
>>>>> + * primary process. In secondary process, we only need to
>>>>> + * release port.
>>>>> + */
>>>>> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>> + return rte_eth_dev_release_port_private(eth_dev);
>>>> I've realized that some uninit functions which will not be called
>>>> anymore in secondary processes have check for process type and
>>>> handling of secondary process case. It makes code inconsistent and should
>> be fixed.
>>> Good point, I did a scan and check all the places that
>> rte_eth_dev_pci_generic_remove be involved.
>>> I found only sfc driver (sfc_eth_dev_unit) will call some cleanup on
>> secondary process as below.
>>
>> The patch makes impossible dev_uninit to be executed for secondary process
>> for all cases if rte_eth_dev_pci_generic_remove() is used. However, many
>> drivers still check for process type. Yes, sfc does cleanup, but some drivers
>> return -EPERM, some return 0. In fact it does not matter. It leaves dead code
>> which is really confusing.
> OK, l can do a cleanup in a separate patchset if this one will be merged.
For now, I'd like to revoke my Reviewed-by. I'll review once again when
complete solution is suggested.
@@ -359,6 +359,18 @@ 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);
+ 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)
@@ -62,7 +62,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
*/
@@ -70,6 +70,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_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.
*
@@ -197,6 +197,14 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev,
if (!eth_dev)
return -ENODEV;
+ /**
+ * PCI device can only be globally detached directly by a
+ * primary process. In secondary process, we only need to
+ * release port.
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return rte_eth_dev_release_port_private(eth_dev);
+
if (dev_uninit) {
ret = dev_uninit(eth_dev);
if (ret)