[dpdk-dev] [PATCH] net/virtio-user: fix multi-process issue

[dpdk-dev] [PATCH] net/virtio-user: fix multi-process issue

[Ami Sabo]

Secondary process doesn't properly attach to the rte_eth_device
initialized by the primary process.

Accessing device from secondary process (e.g. via rte_eth_rx_burst),
causes process to crash. because rte_eth_dev_data is not properly set.

The issue was flood by
'commit 7f95f78a8aea ("ethdev: clear data when allocating device")'
which now clears rte_eth_dev_data entry.

So, most of the rte_eth_dev_data fields are not initialized.
For pci devices these fields are initialized  by rte_eth_dev_pci_probe
->eth_dev_attach_secondary().
However, for virtio-user virtio_user_pmd_probe() is called instead of
rte_eth_dev_pci_probe().
To fix it:  Allow non-pci drivers call to dev_attach_secondary() and
call it (for secondary process) from virtio_user_pmd_probe.

Signed-off-by: Ami Sabo <amis@radware.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 29 +++++++++++++++++------------
 lib/librte_ether/rte_ethdev.c           |  6 +++---
 lib/librte_ether/rte_ethdev.h           | 11 +++++++++++
 lib/librte_ether/rte_ether_version.map  |  1 +
 4 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index e544acc..d388b92 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -418,18 +418,23 @@ virtio_user_pmd_probe(const char *name, const char *params)
 		goto end;
 	}
 
-	eth_dev = virtio_user_eth_dev_alloc(name);
-	if (!eth_dev) {
-		PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
-		goto end;
-	}
-
-	hw = eth_dev->data->dev_private;
-	if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
-				 queue_size, mac_addr) < 0) {
-		PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
-		virtio_user_eth_dev_free(eth_dev);
-		goto end;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev = virtio_user_eth_dev_alloc(name);
+		if (!eth_dev) {
+			PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
+			goto end;
+		}
+		hw = eth_dev->data->dev_private;
+		if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
+			queue_size, mac_addr) < 0) {
+			PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
+			virtio_user_eth_dev_free(eth_dev);
+			goto end;
+		}
+	} else {
+		eth_dev = rte_eth_dev_attach_secondary(name);
+		if (!eth_dev)
+			goto end;
 	}
 
 	/* previously called by rte_eal_pci_probe() for physical dev */
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 61f44e2..ea4f76c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -239,8 +239,8 @@ rte_eth_dev_allocate(const char *name)
  * makes sure that the same device would have the same port id both
  * in the primary and secondary process.
  */
-static struct rte_eth_dev *
-eth_dev_attach_secondary(const char *name)
+struct rte_eth_dev *
+rte_eth_dev_attach_secondary(const char *name)
 {
 	uint8_t i;
 	struct rte_eth_dev *eth_dev;
@@ -302,7 +302,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 		if (eth_dev->data->dev_private == NULL)
 			rte_panic("Cannot allocate memzone for private port data\n");
 	} else {
-		eth_dev = eth_dev_attach_secondary(ethdev_name);
+		eth_dev = rte_eth_dev_attach_secondary(ethdev_name);
 		if (eth_dev == NULL) {
 			/*
 			 * if we failed to attach a device, it means the
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index c17bbda..3281205 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1762,6 +1762,17 @@ struct rte_eth_dev *rte_eth_dev_allocate(const char *name);
 
 /**
  * @internal
+ * Attach to the ethdev already initialized by the primary
+ * process.
+ *
+ * @param	name	Ethernet device's name.
+  @return
+ *   - Slot in the rte_dev_devices array for attached device;
+ */
+struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
+
+/**
+ * @internal
  * Release the specified ethdev port.
  *
  * @param eth_dev
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index c6c9d0d..f8bf2ee 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -152,5 +152,6 @@ DPDK_17.02 {
 	rte_flow_flush;
 	rte_flow_query;
 	rte_flow_validate;
+	rte_eth_dev_attach_secondary;
 
 } DPDK_16.11;
-- 
2.7.4

Re: [dpdk-dev] [PATCH] net/virtio-user: fix multi-process issue

[Yuanhan Liu]

On Thu, Feb 23, 2017 at 01:58:16PM +0200, Ami Sabo wrote:
> Secondary process doesn't properly attach to the rte_eth_device
> initialized by the primary process.
> 
> Accessing device from secondary process (e.g. via rte_eth_rx_burst),
> causes process to crash. because rte_eth_dev_data is not properly set.
> 
> The issue was flood by
> 'commit 7f95f78a8aea ("ethdev: clear data when allocating device")'
> which now clears rte_eth_dev_data entry.
> 
> So, most of the rte_eth_dev_data fields are not initialized.
> For pci devices these fields are initialized  by rte_eth_dev_pci_probe
> ->eth_dev_attach_secondary().
> However, for virtio-user virtio_user_pmd_probe() is called instead of
> rte_eth_dev_pci_probe().
> To fix it:  Allow non-pci drivers call to dev_attach_secondary() and
> call it (for secondary process) from virtio_user_pmd_probe.
> 
> Signed-off-by: Ami Sabo <amis@radware.com>

Firstly, two minor comments:

- A fix path needs a fixline (check dpdk.org/dev for HOWTO)

- It fixes a bug in former releases, thus it need be picked into a stable
  release. Then you need add following just before you Signed-off-by:

     Cc: stable@dpdk.org

>  /**
>   * @internal
> + * Attach to the ethdev already initialized by the primary
> + * process.
> + *
> + * @param	name	Ethernet device's name.
> +  @return
> + *   - Slot in the rte_dev_devices array for attached device;

Yes, that's what it returns on success. You also need to add the case
when it fails.

> + */
> +struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
> +
> +/**
> + * @internal
>   * Release the specified ethdev port.
>   *
>   * @param eth_dev
> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
> index c6c9d0d..f8bf2ee 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -152,5 +152,6 @@ DPDK_17.02 {
>  	rte_flow_flush;
>  	rte_flow_query;
>  	rte_flow_validate;
> +	rte_eth_dev_attach_secondary;
>  
>  } DPDK_16.11;

17.02 is released, you should add a new table for 17.05 and add it there.


Besides, I would suggest you to split this patch into two:

- one for exporting rte_eth_dev_attach_secondary
- another for fixing the bug

	--yliu

[dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.

[Ami Sabo]

The patchset fixes secondary process crash issue when it tries
to access virtio-user pmd (e.g. via rte_eth_rx_burst).

The crash happens because in virtio_user probing,
eth_dev_attach_secondary is not being called, as it does from
rte_eth_dev_pci_probe. Therefore, the device is not properly
initialized.

The patchset contains 2 patches:
1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
allowed to call it.
2. Fix the actual bug by calling the function during virtio_user probe.

Ami Sabo (2):
  lib/librte_ether: export secondary attach function
  net/virtio-user: fix multi-process issue

 drivers/net/virtio/virtio_user_ethdev.c | 26 ++++++++++++++++----------
 lib/librte_ether/rte_ethdev.c           |  6 +++---
 lib/librte_ether/rte_ethdev.h           | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map  |  7 +++++++
 4 files changed, 39 insertions(+), 13 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function

[Ami Sabo]

Today eth_dev_attach_secondary is defined as static and can only be
called by pci drivers. However, the functionality is also required for
non-pci drivers - so the patch export the function.

Signed-off-by: Ami Sabo <amis@radware.com>
---
 lib/librte_ether/rte_ethdev.c          |  6 +++---
 lib/librte_ether/rte_ethdev.h          | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index eb0a94a..86ee5bb 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -239,8 +239,8 @@ rte_eth_dev_allocate(const char *name)
  * makes sure that the same device would have the same port id both
  * in the primary and secondary process.
  */
-static struct rte_eth_dev *
-eth_dev_attach_secondary(const char *name)
+struct rte_eth_dev *
+rte_eth_dev_attach_secondary(const char *name)
 {
 	uint8_t i;
 	struct rte_eth_dev *eth_dev;
@@ -302,7 +302,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 		if (eth_dev->data->dev_private == NULL)
 			rte_panic("Cannot allocate memzone for private port data\n");
 	} else {
-		eth_dev = eth_dev_attach_secondary(ethdev_name);
+		eth_dev = rte_eth_dev_attach_secondary(ethdev_name);
 		if (eth_dev == NULL) {
 			/*
 			 * if we failed to attach a device, it means the
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 97f3e2d..9d5848b 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1762,6 +1762,19 @@ struct rte_eth_dev *rte_eth_dev_allocate(const char *name);
 
 /**
  * @internal
+ * Attach to the ethdev already initialized by the primary
+ * process.
+ *
+ * @param       name    Ethernet device's name.
+  @return
+ *   - Success: Slot in the rte_dev_devices array for attached
+ *        device.
+ *   - Error: Null pointer.
+ */
+struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
+
+/**
+ * @internal
  * Release the specified ethdev port.
  *
  * @param eth_dev
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index c6c9d0d..d34c57a 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -154,3 +154,10 @@ DPDK_17.02 {
 	rte_flow_validate;
 
 } DPDK_16.11;
+
+DPDK_17.05 {
+        global:
+
+        rte_eth_dev_attach_secondary;
+
+} DPDK_17.02;
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function

[Yuanhan Liu]

Cc Thomas, the librte_ether maintainer.

On Sun, Feb 26, 2017 at 11:55:25AM +0200, Ami Sabo wrote:
>  /**
>   * @internal
> + * Attach to the ethdev already initialized by the primary
> + * process.
> + *
> + * @param       name    Ethernet device's name.
> +  @return

mailformed comment: missing *.

> + *   - Success: Slot in the rte_dev_devices array for attached
> + *        device.
> + *   - Error: Null pointer.
> + */
> +struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
> +
> +/**
> + * @internal
>   * Release the specified ethdev port.
>   *
>   * @param eth_dev
> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
> index c6c9d0d..d34c57a 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -154,3 +154,10 @@ DPDK_17.02 {
>  	rte_flow_validate;
>  
>  } DPDK_16.11;
> +
> +DPDK_17.05 {
> +        global:
> +
> +        rte_eth_dev_attach_secondary;

I saw whitespace chars. Like code, it should be TABs. Other than those
minor nits, it looks good to me.

Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu

[dpdk-dev] [PATCH v2 0/3] Fix virtio-user multi-process crash.

[Ami Sabo]

The patchset fixes secondary process crash issue when it tries
to access virtio-user pmd (e.g. via rte_eth_rx_burst).

The crash happens because the secondary process calls, at
virtio_user_pmd_probe() to virtio_user_eth_dev_alloc()->
rte_eth_dev_allocate() instead of eth_dev_attach_secondary(), as it's
done from rte_eth_dev_pci_probe. Therefore, the device is not properly
initialized + the device data maybe overridden by the secondary
process.

The patchset contains 3 patches:
1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
allowed to call it.
2. Fix the actual bug by calling the function during virtio_user probe.
3. Code style fixes following Yuanhan Lio's comments.


Ami Sabo (3):
  lib/librte_ether: export secondary attach function
  net/virtio-user: fix multi-process issue
  lib/librte_ether: fix code style issues

 drivers/net/virtio/virtio_user_ethdev.c | 26 ++++++++++++++++----------
 lib/librte_ether/rte_ethdev.c           |  6 +++---
 lib/librte_ether/rte_ethdev.h           | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map  |  7 +++++++
 4 files changed, 39 insertions(+), 13 deletions(-)

--
v2:
* Fix code style issues following Yuanhan Liu's review.
-- 
2.7.4

[dpdk-dev] [PATCH v2 1/3] lib/librte_ether: export secondary attach function

[Ami Sabo]

Today eth_dev_attach_secondary is defined as static and can only be
called by pci drivers. However, the functionality is also required for
non-pci drivers - so the patch export the function.

Signed-off-by: Ami Sabo <amis@radware.com>
---
 lib/librte_ether/rte_ethdev.c          |  6 +++---
 lib/librte_ether/rte_ethdev.h          | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index eb0a94a..86ee5bb 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -239,8 +239,8 @@ rte_eth_dev_allocate(const char *name)
  * makes sure that the same device would have the same port id both
  * in the primary and secondary process.
  */
-static struct rte_eth_dev *
-eth_dev_attach_secondary(const char *name)
+struct rte_eth_dev *
+rte_eth_dev_attach_secondary(const char *name)
 {
 	uint8_t i;
 	struct rte_eth_dev *eth_dev;
@@ -302,7 +302,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 		if (eth_dev->data->dev_private == NULL)
 			rte_panic("Cannot allocate memzone for private port data\n");
 	} else {
-		eth_dev = eth_dev_attach_secondary(ethdev_name);
+		eth_dev = rte_eth_dev_attach_secondary(ethdev_name);
 		if (eth_dev == NULL) {
 			/*
 			 * if we failed to attach a device, it means the
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 97f3e2d..9d5848b 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1762,6 +1762,19 @@ struct rte_eth_dev *rte_eth_dev_allocate(const char *name);
 
 /**
  * @internal
+ * Attach to the ethdev already initialized by the primary
+ * process.
+ *
+ * @param       name    Ethernet device's name.
+  @return
+ *   - Success: Slot in the rte_dev_devices array for attached
+ *        device.
+ *   - Error: Null pointer.
+ */
+struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
+
+/**
+ * @internal
  * Release the specified ethdev port.
  *
  * @param eth_dev
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index c6c9d0d..d34c57a 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -154,3 +154,10 @@ DPDK_17.02 {
 	rte_flow_validate;
 
 } DPDK_16.11;
+
+DPDK_17.05 {
+        global:
+
+        rte_eth_dev_attach_secondary;
+
+} DPDK_17.02;
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/3] net/virtio-user: fix multi-process issue

[Ami Sabo]

Secondary process doesn't properly attach to the rte_eth_device
initialized by the primary process.

ccessing device from secondary process (e.g. via rte_eth_rx_burst),
causes process to crash. because rte_eth_dev_data is not properly set.

The issue was flood by
'commit 7f95f78a8aea ("ethdev: clear data when allocating device")'
which now clears rte_eth_dev_data entry.
For pci devices the struct is initialized by rte_eth_dev_pci_probe
->eth_dev_attach_secondary().
However, for virtio-user virtio_user_pmd_probe() is called instead of
rte_eth_dev_pci_probe().

The fix is to call rte_eth_dev_attach_secondary(), for secondary
process, from virtio_user_pmd_probe.

Fixes: 7f95f78a8aea ("ethdev: clear data when allocating device")

Cc: stable@dpdk.org

Signed-off-by: Ami Sabo <amis@radware.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 0b226ac..6033908 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -418,18 +418,24 @@ virtio_user_pmd_probe(const char *name, const char *params)
 		goto end;
 	}
 
-	eth_dev = virtio_user_eth_dev_alloc(name);
-	if (!eth_dev) {
-		PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
-		goto end;
-	}
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev = virtio_user_eth_dev_alloc(name);
+		if (!eth_dev) {
+			PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
+			goto end;
+		}
 
-	hw = eth_dev->data->dev_private;
-	if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
+		hw = eth_dev->data->dev_private;
+		if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
 				 queue_size, mac_addr) < 0) {
-		PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
-		virtio_user_eth_dev_free(eth_dev);
-		goto end;
+			PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
+			virtio_user_eth_dev_free(eth_dev);
+			goto end;
+		}
+	} else {
+		eth_dev = rte_eth_dev_attach_secondary(name);
+		if (!eth_dev)
+			goto end;
 	}
 
 	/* previously called by rte_eal_pci_probe() for physical dev */
-- 
2.7.4

[dpdk-dev] [PATCH v2 3/3] lib/librte_ether: fix code style issues

[Ami Sabo]

Fixed code style issues following Yuanhan Liu's review.

Signed-off-by: Ami Sabo <amis@radware.com>
---
 lib/librte_ether/rte_ethdev.h          | 2 +-
 lib/librte_ether/rte_ether_version.map | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9d5848b..6c9af46 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1766,7 +1766,7 @@ struct rte_eth_dev *rte_eth_dev_allocate(const char *name);
  * process.
  *
  * @param       name    Ethernet device's name.
-  @return
+ * @return
  *   - Success: Slot in the rte_dev_devices array for attached
  *        device.
  *   - Error: Null pointer.
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index d34c57a..28fb7d9 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -156,8 +156,8 @@ DPDK_17.02 {
 } DPDK_16.11;
 
 DPDK_17.05 {
-        global:
+	global:
 
-        rte_eth_dev_attach_secondary;
+	rte_eth_dev_attach_secondary;
 
 } DPDK_17.02;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 3/3] lib/librte_ether: fix code style issues

[Yuanhan Liu]

On Thu, Mar 02, 2017 at 09:51:24AM +0200, Ami Sabo wrote:
> Fixed code style issues following Yuanhan Liu's review.

Patch 1 is not merged yet. You should squash this fix into patch 1,
instead of sending another patch.

Meanwhile, when you do so, you should add version log, and put it
at .....>

> 
> Signed-off-by: Ami Sabo <amis@radware.com>
> ---

... here (right below the ---). It could be something like:

- v2: fix coding style issues (Yuanhan Liu)


If you are still unsure, just find some examples from the mailing list.

	--yliu

[dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.

[Ami Sabo]

The patchset fixes secondary process crash issue when it tries
to access virtio-user pmd (e.g. via rte_eth_rx_burst).

The crash happens because in virtio_user probing,
eth_dev_attach_secondary is not being called, as it does from
rte_eth_dev_pci_probe. Therefore, the device is not properly
initialized.

The patchset contains 2 patches:
1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
allowed to call it.
2. Fix the actual bug by calling the function during virtio_user probe.

Ami Sabo (2):
  lib/librte_ether: export secondary attach function
  net/virtio-user: fix multi-process issue

 drivers/net/virtio/virtio_user_ethdev.c | 26 ++++++++++++++++----------
 lib/librte_ether/rte_ethdev.c           |  6 +++---
 lib/librte_ether/rte_ethdev.h           | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map  |  6 ++++++
 4 files changed, 38 insertions(+), 13 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function

[Ami Sabo]

Today eth_dev_attach_secondary is defined as static and can only be
called by pci drivers. However, the functionality is also required for
non-pci drivers - so the patch export the function.

Signed-off-by: Ami Sabo <amis@radware.com>
---
v2: Fix coding style issues (Yuanhan Liu)
---
 lib/librte_ether/rte_ethdev.c          |  6 +++---
 lib/librte_ether/rte_ethdev.h          | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map |  6 ++++++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index eb0a94a..86ee5bb 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -239,8 +239,8 @@ rte_eth_dev_allocate(const char *name)
  * makes sure that the same device would have the same port id both
  * in the primary and secondary process.
  */
-static struct rte_eth_dev *
-eth_dev_attach_secondary(const char *name)
+struct rte_eth_dev *
+rte_eth_dev_attach_secondary(const char *name)
 {
 	uint8_t i;
 	struct rte_eth_dev *eth_dev;
@@ -302,7 +302,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 		if (eth_dev->data->dev_private == NULL)
 			rte_panic("Cannot allocate memzone for private port data\n");
 	} else {
-		eth_dev = eth_dev_attach_secondary(ethdev_name);
+		eth_dev = rte_eth_dev_attach_secondary(ethdev_name);
 		if (eth_dev == NULL) {
 			/*
 			 * if we failed to attach a device, it means the
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 97f3e2d..6c9af46 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1762,6 +1762,19 @@ struct rte_eth_dev *rte_eth_dev_allocate(const char *name);
 
 /**
  * @internal
+ * Attach to the ethdev already initialized by the primary
+ * process.
+ *
+ * @param       name    Ethernet device's name.
+ * @return
+ *   - Success: Slot in the rte_dev_devices array for attached
+ *        device.
+ *   - Error: Null pointer.
+ */
+struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
+
+/**
+ * @internal
  * Release the specified ethdev port.
  *
  * @param eth_dev
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index c6c9d0d..85b0402 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -154,3 +154,9 @@ DPDK_17.02 {
 	rte_flow_validate;
 
 } DPDK_16.11;
+
+DPDK_17.05 {
+	global:
+
+	rte_eth_dev_attach_secondary;
+} DPDK_17.02;
-- 
2.7.4

[dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue

[Ami Sabo]

Secondary process doesn't properly attach to the rte_eth_device
initialized by the primary process.

ccessing device from secondary process (e.g. via rte_eth_rx_burst),
causes process to crash. because rte_eth_dev_data is not properly set.

The issue was flood by
'commit 7f95f78a8aea ("ethdev: clear data when allocating device")'
which now clears rte_eth_dev_data entry.
For pci devices the struct is initialized by rte_eth_dev_pci_probe
->eth_dev_attach_secondary().
However, for virtio-user virtio_user_pmd_probe() is called instead of
rte_eth_dev_pci_probe().

The fix is to call rte_eth_dev_attach_secondary(), for secondary
process, from virtio_user_pmd_probe.

Fixes: 7f95f78a8aea ("ethdev: clear data when allocating device")

Cc: stable@dpdk.org

Signed-off-by: Ami Sabo <amis@radware.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 0b226ac..6033908 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -418,18 +418,24 @@ virtio_user_pmd_probe(const char *name, const char *params)
 		goto end;
 	}
 
-	eth_dev = virtio_user_eth_dev_alloc(name);
-	if (!eth_dev) {
-		PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
-		goto end;
-	}
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev = virtio_user_eth_dev_alloc(name);
+		if (!eth_dev) {
+			PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
+			goto end;
+		}
 
-	hw = eth_dev->data->dev_private;
-	if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
+		hw = eth_dev->data->dev_private;
+		if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
 				 queue_size, mac_addr) < 0) {
-		PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
-		virtio_user_eth_dev_free(eth_dev);
-		goto end;
+			PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
+			virtio_user_eth_dev_free(eth_dev);
+			goto end;
+		}
+	} else {
+		eth_dev = rte_eth_dev_attach_secondary(name);
+		if (!eth_dev)
+			goto end;
 	}
 
 	/* previously called by rte_eal_pci_probe() for physical dev */
-- 
2.7.4

Re: [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.

[Thomas Monjalon]

2017-03-02 11:00, Ami Sabo:
> The patchset fixes secondary process crash issue when it tries
> to access virtio-user pmd (e.g. via rte_eth_rx_burst).
> 
> The crash happens because in virtio_user probing,
> eth_dev_attach_secondary is not being called, as it does from
> rte_eth_dev_pci_probe. Therefore, the device is not properly
> initialized.
> 
> The patchset contains 2 patches:
> 1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
> allowed to call it.
> 2. Fix the actual bug by calling the function during virtio_user probe.

I do not understand why nobody complains for other virtual devices.
We should have the same issue with pcap, tap, ring, af_packet, etc.
Probably that other drivers are broken in secondary processes.
Or should we make a fix to handle every secondary vdev in
rte_eth_dev_allocate() ?

Re: [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.

[Thomas Monjalon]

Ping

2017-03-08 12:40, Thomas Monjalon:
> 2017-03-02 11:00, Ami Sabo:
> > The patchset fixes secondary process crash issue when it tries
> > to access virtio-user pmd (e.g. via rte_eth_rx_burst).
> > 
> > The crash happens because in virtio_user probing,
> > eth_dev_attach_secondary is not being called, as it does from
> > rte_eth_dev_pci_probe. Therefore, the device is not properly
> > initialized.
> > 
> > The patchset contains 2 patches:
> > 1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
> > allowed to call it.
> > 2. Fix the actual bug by calling the function during virtio_user probe.
> 
> I do not understand why nobody complains for other virtual devices.
> We should have the same issue with pcap, tap, ring, af_packet, etc.
> Probably that other drivers are broken in secondary processes.
> Or should we make a fix to handle every secondary vdev in
> rte_eth_dev_allocate() ?

Any opinion?

Re: [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.

[Tan, Jianfeng]

Hi Thomas,


On 4/7/2017 4:14 AM, Thomas Monjalon wrote:
> Ping
>
> 2017-03-08 12:40, Thomas Monjalon:
>> 2017-03-02 11:00, Ami Sabo:
>>> The patchset fixes secondary process crash issue when it tries
>>> to access virtio-user pmd (e.g. via rte_eth_rx_burst).
>>>
>>> The crash happens because in virtio_user probing,
>>> eth_dev_attach_secondary is not being called, as it does from
>>> rte_eth_dev_pci_probe. Therefore, the device is not properly
>>> initialized.
>>>
>>> The patchset contains 2 patches:
>>> 1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
>>> allowed to call it.
>>> 2. Fix the actual bug by calling the function during virtio_user probe.
>> I do not understand why nobody complains for other virtual devices.
>> We should have the same issue with pcap, tap, ring, af_packet, etc.

Yes, none of vdev except ring supports primary/secondary mode.

>> Probably that other drivers are broken in secondary processes.
>> Or should we make a fix to handle every secondary vdev in
>> rte_eth_dev_allocate() ?

Agreed. We can change the rte_eth_dev_allocate() like this:
if (primary)
     allocate();
else
     lookup(name);

Besides, we need each vdev to handle its private. For example, pcap 
should share the selectable unix fd with secondary process; virtio-user 
should share callfds and kickfds; tap should share the fd pointing to 
/dev/tun.

Thanks,
Jianfeng

Re: [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.

[Thomas Monjalon]

2017-03-02 11:00, Ami Sabo:
> The patchset fixes secondary process crash issue when it tries
> to access virtio-user pmd (e.g. via rte_eth_rx_burst).
> 
> The crash happens because in virtio_user probing,
> eth_dev_attach_secondary is not being called, as it does from
> rte_eth_dev_pci_probe. Therefore, the device is not properly
> initialized.
> 
> The patchset contains 2 patches:
> 1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
> allowed to call it.
> 2. Fix the actual bug by calling the function during virtio_user probe.
> 
> Ami Sabo (2):
>   lib/librte_ether: export secondary attach function
>   net/virtio-user: fix multi-process issue

Applied, thanks

[dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue

[Ami Sabo]

Secondary process doesn't properly attach to the rte_eth_device
initialized by the primary process.

ccessing device from secondary process (e.g. via rte_eth_rx_burst),
causes process to crash. because rte_eth_dev_data is not properly set.

The issue was flood by
'commit 7f95f78a8aea ("ethdev: clear data when allocating device")'
which now clears rte_eth_dev_data entry.
For pci devices the struct is initialized by rte_eth_dev_pci_probe
->eth_dev_attach_secondary().
However, for virtio-user virtio_user_pmd_probe() is called instead of
rte_eth_dev_pci_probe().

The fix is to call rte_eth_dev_attach_secondary(), for secondary
process, from virtio_user_pmd_probe.

Fixes: 7f95f78a8aea ("ethdev: clear data when allocating device")

Cc: stable@dpdk.org

Signed-off-by: Ami Sabo <amis@radware.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 0b226ac..6033908 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -418,18 +418,24 @@ virtio_user_pmd_probe(const char *name, const char *params)
 		goto end;
 	}
 
-	eth_dev = virtio_user_eth_dev_alloc(name);
-	if (!eth_dev) {
-		PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
-		goto end;
-	}
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev = virtio_user_eth_dev_alloc(name);
+		if (!eth_dev) {
+			PMD_INIT_LOG(ERR, "virtio_user fails to alloc device");
+			goto end;
+		}
 
-	hw = eth_dev->data->dev_private;
-	if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
+		hw = eth_dev->data->dev_private;
+		if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
 				 queue_size, mac_addr) < 0) {
-		PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
-		virtio_user_eth_dev_free(eth_dev);
-		goto end;
+			PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
+			virtio_user_eth_dev_free(eth_dev);
+			goto end;
+		}
+	} else {
+		eth_dev = rte_eth_dev_attach_secondary(name);
+		if (!eth_dev)
+			goto end;
 	}
 
 	/* previously called by rte_eal_pci_probe() for physical dev */
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue

[Yuanhan Liu]

On Sun, Feb 26, 2017 at 11:55:26AM +0200, Ami Sabo wrote:
> Secondary process doesn't properly attach to the rte_eth_device
> initialized by the primary process.
> 
> ccessing device from secondary process (e.g. via rte_eth_rx_burst),
> causes process to crash. because rte_eth_dev_data is not properly set.
> 
> The issue was flood by
> 'commit 7f95f78a8aea ("ethdev: clear data when allocating device")'
> which now clears rte_eth_dev_data entry.
> For pci devices the struct is initialized by rte_eth_dev_pci_probe
> ->eth_dev_attach_secondary().
> However, for virtio-user virtio_user_pmd_probe() is called instead of
> rte_eth_dev_pci_probe().
> 
> The fix is to call rte_eth_dev_attach_secondary(), for secondary
> process, from virtio_user_pmd_probe.
> 
> Fixes: 7f95f78a8aea ("ethdev: clear data when allocating device")

Are you sure that's the real culprit? As I'm aware of, virtio-user
is not built with multiple process support in the beginning. That
said, it's likely that the first commit introduces virtio-user is
the "culprit" commit.

Besides that, the code looks good to me. If Thomas is fine with
your first patch, I could merge them to my tree.

	--yliu

Re: [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue

[Ami Sabo]

Hi,
You are right, the commit I mentioned didn't cause the issue - it just flooded it.
The real issue is that rte_eth_dev_allocate should be called only from the primary process.
Tomas's commit flood the issue by resseting rte_eth_dev_data, so now, when the virtio-user secondary process comes up and calls rte_eth_dev_allocate
It clears the ethdev->data struct (so fields like rx_queues, mac_addrs, etc will be 0, plus this may cause race condition between the primary and secondary processes...) 

	--ami

-----Original Message-----
From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] 
Sent: Tuesday, February 28, 2017 8:40 AM
To: Ami Sabo
Cc: dev@dpdk.org; stable@dpdk.org
Subject: Re: [PATCH 2/2] net/virtio-user: fix multi-process issue

On Sun, Feb 26, 2017 at 11:55:26AM +0200, Ami Sabo wrote:
> Secondary process doesn't properly attach to the rte_eth_device 
> initialized by the primary process.
> 
> ccessing device from secondary process (e.g. via rte_eth_rx_burst), 
> causes process to crash. because rte_eth_dev_data is not properly set.
> 
> The issue was flood by
> 'commit 7f95f78a8aea ("ethdev: clear data when allocating device")'
> which now clears rte_eth_dev_data entry.
> For pci devices the struct is initialized by rte_eth_dev_pci_probe
> ->eth_dev_attach_secondary().
> However, for virtio-user virtio_user_pmd_probe() is called instead of 
> rte_eth_dev_pci_probe().
> 
> The fix is to call rte_eth_dev_attach_secondary(), for secondary 
> process, from virtio_user_pmd_probe.
> 
> Fixes: 7f95f78a8aea ("ethdev: clear data when allocating device")

Are you sure that's the real culprit? As I'm aware of, virtio-user is not built with multiple process support in the beginning. That said, it's likely that the first commit introduces virtio-user is the "culprit" commit.

Besides that, the code looks good to me. If Thomas is fine with your first patch, I could merge them to my tree.

	--yliu