* [PATCH v2] net/virtio-user: fix multi-process issue [not found] <1487762487-21698-amis@radware.com> @ 2017-02-22 15:40 ` Ami Sabo 2017-02-23 2:54 ` Yuanhan Liu 2017-02-23 8:52 ` [PATCH] Signed-off-by: Ami Sabo <amis@radware.com> Ami Sabo 0 siblings, 2 replies; 4+ messages in thread From: Ami Sabo @ 2017-02-22 15:40 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. --- v2: * Fix coding style issues. Signed-off-by: Ami Sabo <amis@radware.com> --- drivers/net/virtio/virtio_user_ethdev.c | 38 ++++++++++++++++----------------- lib/librte_ether/rte_ethdev.h | 4 ++-- lib/librte_ether/rte_ether_version.map | 2 +- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index 5291294..9b3c266 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -418,25 +418,25 @@ virtio_user_pmd_probe(const char *name, const char *params) 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; - } - } + 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 */ if (eth_virtio_dev_init(eth_dev) < 0) { diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index b30829f..3281205 100644 --- a/lib/librte_ether/rte_ethdev.h +++ b/lib/librte_ether/rte_ethdev.h @@ -1762,8 +1762,8 @@ struct rte_eth_dev *rte_eth_dev_allocate(const char *name); /** * @internal - * Attach to the ethdev already initialized by the primary - * process. + * Attach to the ethdev already initialized by the primary + * process. * * @param name Ethernet device's name. @return diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map index ed4917c..f8bf2ee 100644 --- a/lib/librte_ether/rte_ether_version.map +++ b/lib/librte_ether/rte_ether_version.map @@ -152,6 +152,6 @@ DPDK_17.02 { rte_flow_flush; rte_flow_query; rte_flow_validate; - rte_eth_dev_attach_secondary; + rte_eth_dev_attach_secondary; } DPDK_16.11; -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] net/virtio-user: fix multi-process issue 2017-02-22 15:40 ` [PATCH v2] net/virtio-user: fix multi-process issue Ami Sabo @ 2017-02-23 2:54 ` Yuanhan Liu 2017-02-23 9:02 ` Ami Sabo 2017-02-23 8:52 ` [PATCH] Signed-off-by: Ami Sabo <amis@radware.com> Ami Sabo 1 sibling, 1 reply; 4+ messages in thread From: Yuanhan Liu @ 2017-02-23 2:54 UTC (permalink / raw) To: Ami Sabo; +Cc: dev On Wed, Feb 22, 2017 at 05:40:28PM +0200, Ami Sabo wrote: > diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c > index 5291294..9b3c266 100644 > --- a/drivers/net/virtio/virtio_user_ethdev.c > +++ b/drivers/net/virtio/virtio_user_ethdev.c > @@ -418,25 +418,25 @@ virtio_user_pmd_probe(const char *name, const char *params) > 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; > - } > - } Were you making the patch based on some internal code base? The latest virtio code doesn't have the above code. > + 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); We also don't have rte_eth_dev_attach_secondary exported, neither. --yliu ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] net/virtio-user: fix multi-process issue 2017-02-23 2:54 ` Yuanhan Liu @ 2017-02-23 9:02 ` Ami Sabo 0 siblings, 0 replies; 4+ messages in thread From: Ami Sabo @ 2017-02-23 9:02 UTC (permalink / raw) To: Yuanhan Liu; +Cc: dev@dpdk.org Mistakenly sent only partial diff. Resent the all patch (from the original code base) as a reply to the first message in the thread. -- Ami -----Original Message----- From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] Sent: Thursday, February 23, 2017 4:55 AM To: Ami Sabo Cc: dev@dpdk.org Subject: Re: [PATCH v2] net/virtio-user: fix multi-process issue On Wed, Feb 22, 2017 at 05:40:28PM +0200, Ami Sabo wrote: > diff --git a/drivers/net/virtio/virtio_user_ethdev.c > b/drivers/net/virtio/virtio_user_ethdev.c > index 5291294..9b3c266 100644 > --- a/drivers/net/virtio/virtio_user_ethdev.c > +++ b/drivers/net/virtio/virtio_user_ethdev.c > @@ -418,25 +418,25 @@ virtio_user_pmd_probe(const char *name, const char *params) > 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; > - } > - } Were you making the patch based on some internal code base? The latest virtio code doesn't have the above code. > + 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); We also don't have rte_eth_dev_attach_secondary exported, neither. --yliu ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Signed-off-by: Ami Sabo <amis@radware.com> 2017-02-22 15:40 ` [PATCH v2] net/virtio-user: fix multi-process issue Ami Sabo 2017-02-23 2:54 ` Yuanhan Liu @ 2017-02-23 8:52 ` Ami Sabo 1 sibling, 0 replies; 4+ messages in thread From: Ami Sabo @ 2017-02-23 8:52 UTC (permalink / raw) To: yuanhan.liu; +Cc: dev, Ami Sabo net/virtio-user: fix multi-process issue 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 related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-23 9:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1487762487-21698-amis@radware.com>
2017-02-22 15:40 ` [PATCH v2] net/virtio-user: fix multi-process issue Ami Sabo
2017-02-23 2:54 ` Yuanhan Liu
2017-02-23 9:02 ` Ami Sabo
2017-02-23 8:52 ` [PATCH] Signed-off-by: Ami Sabo <amis@radware.com> 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).