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.zh...@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:
