DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/virtio-user: fix multi-process issue
@ 2017-02-23 11:58 Ami Sabo
  2017-02-24  8:22 ` Yuanhan Liu
  2017-02-26  9:55 ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
  0 siblings, 2 replies; 20+ messages in thread
From: Ami Sabo @ 2017-02-23 11:58 UTC (permalink / raw)
  To: yuanhan.liu; +Cc: dev, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio-user: fix multi-process issue
  2017-02-23 11:58 [dpdk-dev] [PATCH] net/virtio-user: fix multi-process issue Ami Sabo
@ 2017-02-24  8:22 ` Yuanhan Liu
  2017-02-26  9:55 ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
  1 sibling, 0 replies; 20+ messages in thread
From: Yuanhan Liu @ 2017-02-24  8:22 UTC (permalink / raw)
  To: Ami Sabo; +Cc: dev, Thomas Monjalon

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.
  2017-02-23 11:58 [dpdk-dev] [PATCH] net/virtio-user: fix multi-process issue Ami Sabo
  2017-02-24  8:22 ` Yuanhan Liu
@ 2017-02-26  9:55 ` Ami Sabo
  2017-02-26  9:55   ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
  2017-02-26  9:55   ` [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue Ami Sabo
  1 sibling, 2 replies; 20+ messages in thread
From: Ami Sabo @ 2017-02-26  9:55 UTC (permalink / raw)
  To: yuanhan.liu; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function
  2017-02-26  9:55 ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
@ 2017-02-26  9:55   ` Ami Sabo
  2017-02-28  6:37     ` Yuanhan Liu
                       ` (2 more replies)
  2017-02-26  9:55   ` [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue Ami Sabo
  1 sibling, 3 replies; 20+ messages in thread
From: Ami Sabo @ 2017-02-26  9:55 UTC (permalink / raw)
  To: yuanhan.liu; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue
  2017-02-26  9:55 ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
  2017-02-26  9:55   ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
@ 2017-02-26  9:55   ` Ami Sabo
  2017-02-28  6:40     ` Yuanhan Liu
  1 sibling, 1 reply; 20+ messages in thread
From: Ami Sabo @ 2017-02-26  9:55 UTC (permalink / raw)
  To: yuanhan.liu; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function
  2017-02-26  9:55   ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
@ 2017-02-28  6:37     ` Yuanhan Liu
  2017-03-02  7:51     ` [dpdk-dev] [PATCH v2 0/3] Fix virtio-user multi-process crash Ami Sabo
  2017-03-02  9:00     ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
  2 siblings, 0 replies; 20+ messages in thread
From: Yuanhan Liu @ 2017-02-28  6:37 UTC (permalink / raw)
  To: Ami Sabo; +Cc: dev, stable, Thomas Monjalon

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue
  2017-02-26  9:55   ` [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue Ami Sabo
@ 2017-02-28  6:40     ` Yuanhan Liu
  2017-02-28  7:50       ` Ami Sabo
  0 siblings, 1 reply; 20+ messages in thread
From: Yuanhan Liu @ 2017-02-28  6:40 UTC (permalink / raw)
  To: Ami Sabo; +Cc: dev, stable

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue
  2017-02-28  6:40     ` Yuanhan Liu
@ 2017-02-28  7:50       ` Ami Sabo
  0 siblings, 0 replies; 20+ messages in thread
From: Ami Sabo @ 2017-02-28  7:50 UTC (permalink / raw)
  To: Yuanhan Liu; +Cc: dev, stable

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v2 0/3] Fix virtio-user multi-process crash.
  2017-02-26  9:55   ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
  2017-02-28  6:37     ` Yuanhan Liu
@ 2017-03-02  7:51     ` Ami Sabo
  2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 1/3] lib/librte_ether: export secondary attach function Ami Sabo
                         ` (2 more replies)
  2017-03-02  9:00     ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
  2 siblings, 3 replies; 20+ messages in thread
From: Ami Sabo @ 2017-03-02  7:51 UTC (permalink / raw)
  To: yuanhan.liu, thomas.monjalon; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v2 1/3] lib/librte_ether: export secondary attach function
  2017-03-02  7:51     ` [dpdk-dev] [PATCH v2 0/3] Fix virtio-user multi-process crash Ami Sabo
@ 2017-03-02  7:51       ` Ami Sabo
  2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 2/3] net/virtio-user: fix multi-process issue Ami Sabo
  2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 3/3] lib/librte_ether: fix code style issues Ami Sabo
  2 siblings, 0 replies; 20+ messages in thread
From: Ami Sabo @ 2017-03-02  7:51 UTC (permalink / raw)
  To: yuanhan.liu, thomas.monjalon; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v2 2/3] net/virtio-user: fix multi-process issue
  2017-03-02  7:51     ` [dpdk-dev] [PATCH v2 0/3] Fix virtio-user multi-process crash Ami Sabo
  2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 1/3] lib/librte_ether: export secondary attach function Ami Sabo
@ 2017-03-02  7:51       ` Ami Sabo
  2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 3/3] lib/librte_ether: fix code style issues Ami Sabo
  2 siblings, 0 replies; 20+ messages in thread
From: Ami Sabo @ 2017-03-02  7:51 UTC (permalink / raw)
  To: yuanhan.liu, thomas.monjalon; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v2 3/3] lib/librte_ether: fix code style issues
  2017-03-02  7:51     ` [dpdk-dev] [PATCH v2 0/3] Fix virtio-user multi-process crash Ami Sabo
  2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 1/3] lib/librte_ether: export secondary attach function Ami Sabo
  2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 2/3] net/virtio-user: fix multi-process issue Ami Sabo
@ 2017-03-02  7:51       ` Ami Sabo
  2017-03-02  8:12         ` Yuanhan Liu
  2 siblings, 1 reply; 20+ messages in thread
From: Ami Sabo @ 2017-03-02  7:51 UTC (permalink / raw)
  To: yuanhan.liu, thomas.monjalon; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v2 3/3] lib/librte_ether: fix code style issues
  2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 3/3] lib/librte_ether: fix code style issues Ami Sabo
@ 2017-03-02  8:12         ` Yuanhan Liu
  0 siblings, 0 replies; 20+ messages in thread
From: Yuanhan Liu @ 2017-03-02  8:12 UTC (permalink / raw)
  To: Ami Sabo; +Cc: thomas.monjalon, dev, stable

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.
  2017-02-26  9:55   ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
  2017-02-28  6:37     ` Yuanhan Liu
  2017-03-02  7:51     ` [dpdk-dev] [PATCH v2 0/3] Fix virtio-user multi-process crash Ami Sabo
@ 2017-03-02  9:00     ` Ami Sabo
  2017-03-02  9:00       ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
                         ` (3 more replies)
  2 siblings, 4 replies; 20+ messages in thread
From: Ami Sabo @ 2017-03-02  9:00 UTC (permalink / raw)
  To: yuanhan.liu, thomas.monjalon; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function
  2017-03-02  9:00     ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
@ 2017-03-02  9:00       ` Ami Sabo
  2017-03-02  9:00       ` [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue Ami Sabo
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 20+ messages in thread
From: Ami Sabo @ 2017-03-02  9:00 UTC (permalink / raw)
  To: yuanhan.liu, thomas.monjalon; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue
  2017-03-02  9:00     ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
  2017-03-02  9:00       ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
@ 2017-03-02  9:00       ` Ami Sabo
  2017-03-08 11:40       ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Thomas Monjalon
  2017-04-14 12:13       ` Thomas Monjalon
  3 siblings, 0 replies; 20+ messages in thread
From: Ami Sabo @ 2017-03-02  9:00 UTC (permalink / raw)
  To: yuanhan.liu, thomas.monjalon; +Cc: dev, stable, 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.
  2017-03-02  9:00     ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
  2017-03-02  9:00       ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
  2017-03-02  9:00       ` [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue Ami Sabo
@ 2017-03-08 11:40       ` Thomas Monjalon
  2017-04-06 20:14         ` Thomas Monjalon
  2017-04-14 12:13       ` Thomas Monjalon
  3 siblings, 1 reply; 20+ messages in thread
From: Thomas Monjalon @ 2017-03-08 11:40 UTC (permalink / raw)
  To: Ami Sabo, yuanhan.liu; +Cc: dev

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() ?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.
  2017-03-08 11:40       ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Thomas Monjalon
@ 2017-04-06 20:14         ` Thomas Monjalon
  2017-04-07  6:33           ` Tan, Jianfeng
  0 siblings, 1 reply; 20+ messages in thread
From: Thomas Monjalon @ 2017-04-06 20:14 UTC (permalink / raw)
  To: Ami Sabo, yuanhan.liu; +Cc: dev

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?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.
  2017-04-06 20:14         ` Thomas Monjalon
@ 2017-04-07  6:33           ` Tan, Jianfeng
  0 siblings, 0 replies; 20+ messages in thread
From: Tan, Jianfeng @ 2017-04-07  6:33 UTC (permalink / raw)
  To: Thomas Monjalon, Ami Sabo, yuanhan.liu; +Cc: dev

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash.
  2017-03-02  9:00     ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
                         ` (2 preceding siblings ...)
  2017-03-08 11:40       ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Thomas Monjalon
@ 2017-04-14 12:13       ` Thomas Monjalon
  3 siblings, 0 replies; 20+ messages in thread
From: Thomas Monjalon @ 2017-04-14 12:13 UTC (permalink / raw)
  To: Ami Sabo; +Cc: yuanhan.liu, dev, stable

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2017-04-14 12:13 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-23 11:58 [dpdk-dev] [PATCH] net/virtio-user: fix multi-process issue Ami Sabo
2017-02-24  8:22 ` Yuanhan Liu
2017-02-26  9:55 ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
2017-02-26  9:55   ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
2017-02-28  6:37     ` Yuanhan Liu
2017-03-02  7:51     ` [dpdk-dev] [PATCH v2 0/3] Fix virtio-user multi-process crash Ami Sabo
2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 1/3] lib/librte_ether: export secondary attach function Ami Sabo
2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 2/3] net/virtio-user: fix multi-process issue Ami Sabo
2017-03-02  7:51       ` [dpdk-dev] [PATCH v2 3/3] lib/librte_ether: fix code style issues Ami Sabo
2017-03-02  8:12         ` Yuanhan Liu
2017-03-02  9:00     ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Ami Sabo
2017-03-02  9:00       ` [dpdk-dev] [PATCH 1/2] lib/librte_ether: export secondary attach function Ami Sabo
2017-03-02  9:00       ` [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue Ami Sabo
2017-03-08 11:40       ` [dpdk-dev] [PATCH 0/2] Fix virtio-user multi-process crash Thomas Monjalon
2017-04-06 20:14         ` Thomas Monjalon
2017-04-07  6:33           ` Tan, Jianfeng
2017-04-14 12:13       ` Thomas Monjalon
2017-02-26  9:55   ` [dpdk-dev] [PATCH 2/2] net/virtio-user: fix multi-process issue Ami Sabo
2017-02-28  6:40     ` Yuanhan Liu
2017-02-28  7:50       ` Ami Sabo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).