[dpdk-dev,v3] net/vhost: fix segfault when creating vdev dynamically
Checks
Commit Message
When creating vdev dynamically, vhost pmd driver starts directly without
checking TX/RX queues are ready or not, and thus causes segmentation fault
when vhost library accesses queues. This patch adds a flag to check whether
queues are setup or not, and adds queues setup into dev_start function to
allow user to start them after setting up.
Fixes: aed0b12930b3 ("net/vhost: fix socket file deleted on stop")
Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
---
Changes in v3:
- Update commit log, refine duplicated code
Changes in v2:
- Check queues status in new_device, create queue in dev_start if not setup yet
drivers/net/vhost/rte_eth_vhost.c | 69 ++++++++++++++++++++++++++-------------
1 file changed, 46 insertions(+), 23 deletions(-)
Comments
Hi Maxime, Junjie,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Junjie Chen
> Sent: Friday, March 30, 2018 2:59 PM
> To: Tan, Jianfeng <jianfeng.tan@intel.com>; maxime.coquelin@redhat.com;
> mtetsuyah@gmail.com
> Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>;
> Chen@dpdk.org
> Subject: [dpdk-dev] [PATCH v3] net/vhost: fix segfault when creating vdev
> dynamically
>
> When creating vdev dynamically, vhost pmd driver starts directly without
> checking TX/RX queues are ready or not, and thus causes segmentation fault
> when vhost library accesses queues. This patch adds a flag to check whether
> queues are setup or not, and adds queues setup into dev_start function to
> allow user to start them after setting up.
>
> Fixes: aed0b12930b3 ("net/vhost: fix socket file deleted on stop")
> Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
> ---
Thanks for Junjie's patch!
I also came across the similar issue when developing virtio-user server mode.
From user's perspective, the patch can fix the issue in my user case instead of
the patch http://www.dpdk.org/dev/patchwork/patch/36340/
Tested-by: Zhiyong Yang <zhiyong.yang@intel.com>
Thanks
Zhiyong
Hi Junjie,
On 03/30/2018 08:58 AM, Junjie Chen wrote:
> When creating vdev dynamically, vhost pmd driver starts directly without
> checking TX/RX queues are ready or not, and thus causes segmentation fault
> when vhost library accesses queues. This patch adds a flag to check whether
> queues are setup or not, and adds queues setup into dev_start function to
> allow user to start them after setting up.
>
> Fixes: aed0b12930b3 ("net/vhost: fix socket file deleted on stop")
> Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
> ---
> Changes in v3:
> - Update commit log, refine duplicated code
> Changes in v2:
> - Check queues status in new_device, create queue in dev_start if not setup yet
>
> drivers/net/vhost/rte_eth_vhost.c | 69 ++++++++++++++++++++++++++-------------
> 1 file changed, 46 insertions(+), 23 deletions(-)
Nice patch!
Thanks for having handled the changes that quickly.
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Maxime
On 03/30/2018 09:32 AM, Yang, Zhiyong wrote:
> Hi Maxime, Junjie,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Junjie Chen
>> Sent: Friday, March 30, 2018 2:59 PM
>> To: Tan, Jianfeng <jianfeng.tan@intel.com>; maxime.coquelin@redhat.com;
>> mtetsuyah@gmail.com
>> Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>;
>> Chen@dpdk.org
>> Subject: [dpdk-dev] [PATCH v3] net/vhost: fix segfault when creating vdev
>> dynamically
>>
>> When creating vdev dynamically, vhost pmd driver starts directly without
>> checking TX/RX queues are ready or not, and thus causes segmentation fault
>> when vhost library accesses queues. This patch adds a flag to check whether
>> queues are setup or not, and adds queues setup into dev_start function to
>> allow user to start them after setting up.
>>
>> Fixes: aed0b12930b3 ("net/vhost: fix socket file deleted on stop")
>> Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
>> ---
>
> Thanks for Junjie's patch!
>
> I also came across the similar issue when developing virtio-user server mode.
> From user's perspective, the patch can fix the issue in my user case instead of
> the patch http://www.dpdk.org/dev/patchwork/patch/36340/
>
> Tested-by: Zhiyong Yang <zhiyong.yang@intel.com>
Great it solves your issue, and thanks for having tested it.
Maxime
> Thanks
> Zhiyong
>
On 03/30/2018 08:58 AM, Junjie Chen wrote:
> When creating vdev dynamically, vhost pmd driver starts directly without
> checking TX/RX queues are ready or not, and thus causes segmentation fault
> when vhost library accesses queues. This patch adds a flag to check whether
> queues are setup or not, and adds queues setup into dev_start function to
> allow user to start them after setting up.
>
> Fixes: aed0b12930b3 ("net/vhost: fix socket file deleted on stop")
> Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
> ---
> Changes in v3:
> - Update commit log, refine duplicated code
> Changes in v2:
> - Check queues status in new_device, create queue in dev_start if not setup yet
>
> drivers/net/vhost/rte_eth_vhost.c | 69 ++++++++++++++++++++++++++-------------
> 1 file changed, 46 insertions(+), 23 deletions(-)
Applied to dpdk-next-virtio/master.
Thanks!
Maxime
Hi,
On Fri, Mar 30, 2018 at 02:58:31PM +0800, Junjie Chen wrote:
>When creating vdev dynamically, vhost pmd driver starts directly without
>checking TX/RX queues are ready or not, and thus causes segmentation fault
>when vhost library accesses queues. This patch adds a flag to check whether
>queues are setup or not, and adds queues setup into dev_start function to
>allow user to start them after setting up.
for me, with this patch vhost enqueue/dequeue code is never called because
if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0))
this check in eth_vhost_rx() is always true.
When I revert this patch it works as usual.
My testpmd cmdline is:
gdb --args $RTE_SDK/install/bin/testpmd -l 0,2,3,4,5 --socket-mem=1024 -n 4 \
--vdev 'net_vhost0,iface=/tmp/vhost-user1' \
--vdev 'net_vhost1,iface=/tmp/vhost-user2' -- \
--portmask=f --rxq=1 --txq=1 \
--nb-cores=4 --forward-mode=io -i
After starting testpmd I issue commands "set portlist 0,2,1,3", start
my guest and start another testpmd issue in the guest.
Another problem I see: Before this patch I could start testpmd, issue
the portlist command and type "start". If I do this now I get an infinite
loop of "VHOST CONFIG device not found" messages.
regards,
Jens
Thanks for report, I just summited this patch to fix:
https://dpdk.org/dev/patchwork/patch/37765/
>
> Hi,
>
> On Fri, Mar 30, 2018 at 02:58:31PM +0800, Junjie Chen wrote:
> >When creating vdev dynamically, vhost pmd driver starts directly
> >without checking TX/RX queues are ready or not, and thus causes
> >segmentation fault when vhost library accesses queues. This patch adds
> >a flag to check whether queues are setup or not, and adds queues setup
> >into dev_start function to allow user to start them after setting up.
>
> for me, with this patch vhost enqueue/dequeue code is never called because
>
> if (unlikely(rte_atomic32_read(&r->allow_queuing) == 0))
>
> this check in eth_vhost_rx() is always true.
>
> When I revert this patch it works as usual.
>
> My testpmd cmdline is:
>
> gdb --args $RTE_SDK/install/bin/testpmd -l 0,2,3,4,5 --socket-mem=1024 -n 4 \
> --vdev 'net_vhost0,iface=/tmp/vhost-user1' \
> --vdev 'net_vhost1,iface=/tmp/vhost-user2' -- \
> --portmask=f --rxq=1 --txq=1 \
> --nb-cores=4 --forward-mode=io -i
>
> After starting testpmd I issue commands "set portlist 0,2,1,3", start my guest
> and start another testpmd issue in the guest.
>
> Another problem I see: Before this patch I could start testpmd, issue the
> portlist command and type "start". If I do this now I get an infinite loop of
> "VHOST CONFIG device not found" messages.
>
>
> regards,
> Jens
@@ -117,6 +117,7 @@ struct pmd_internal {
char *dev_name;
char *iface_name;
uint16_t max_queues;
+ uint16_t vid;
rte_atomic32_t started;
};
@@ -527,8 +528,10 @@ update_queuing_status(struct rte_eth_dev *dev)
unsigned int i;
int allow_queuing = 1;
- if (rte_atomic32_read(&internal->started) == 0 ||
- rte_atomic32_read(&internal->dev_attached) == 0)
+ if (rte_atomic32_read(&internal->dev_attached) == 0)
+ return;
+
+ if (rte_atomic32_read(&internal->started) == 0)
allow_queuing = 0;
/* Wait until rx/tx_pkt_burst stops accessing vhost device */
@@ -551,13 +554,36 @@ update_queuing_status(struct rte_eth_dev *dev)
}
}
+static void
+queue_setup(struct rte_eth_dev *eth_dev, struct pmd_internal *internal)
+{
+ struct vhost_queue *vq;
+ int i;
+
+ for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
+ vq = eth_dev->data->rx_queues[i];
+ if (!vq)
+ continue;
+ vq->vid = internal->vid;
+ vq->internal = internal;
+ vq->port = eth_dev->data->port_id;
+ }
+ for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
+ vq = eth_dev->data->tx_queues[i];
+ if (!vq)
+ continue;
+ vq->vid = internal->vid;
+ vq->internal = internal;
+ vq->port = eth_dev->data->port_id;
+ }
+}
+
static int
new_device(int vid)
{
struct rte_eth_dev *eth_dev;
struct internal_list *list;
struct pmd_internal *internal;
- struct vhost_queue *vq;
unsigned i;
char ifname[PATH_MAX];
#ifdef RTE_LIBRTE_VHOST_NUMA
@@ -580,21 +606,13 @@ new_device(int vid)
eth_dev->data->numa_node = newnode;
#endif
- for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
- vq = eth_dev->data->rx_queues[i];
- if (vq == NULL)
- continue;
- vq->vid = vid;
- vq->internal = internal;
- vq->port = eth_dev->data->port_id;
- }
- for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
- vq = eth_dev->data->tx_queues[i];
- if (vq == NULL)
- continue;
- vq->vid = vid;
- vq->internal = internal;
- vq->port = eth_dev->data->port_id;
+ internal->vid = vid;
+ if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
+ queue_setup(eth_dev, internal);
+ rte_atomic32_set(&internal->dev_attached, 1);
+ } else {
+ RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
+ rte_atomic32_set(&internal->dev_attached, 0);
}
for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
@@ -604,7 +622,6 @@ new_device(int vid)
eth_dev->data->dev_link.link_status = ETH_LINK_UP;
- rte_atomic32_set(&internal->dev_attached, 1);
update_queuing_status(eth_dev);
RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
@@ -634,8 +651,9 @@ destroy_device(int vid)
eth_dev = list->eth_dev;
internal = eth_dev->data->dev_private;
- rte_atomic32_set(&internal->dev_attached, 0);
+ rte_atomic32_set(&internal->started, 0);
update_queuing_status(eth_dev);
+ rte_atomic32_set(&internal->dev_attached, 0);
eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
@@ -770,12 +788,17 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
}
static int
-eth_dev_start(struct rte_eth_dev *dev)
+eth_dev_start(struct rte_eth_dev *eth_dev)
{
- struct pmd_internal *internal = dev->data->dev_private;
+ struct pmd_internal *internal = eth_dev->data->dev_private;
+
+ if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) {
+ queue_setup(eth_dev, internal);
+ rte_atomic32_set(&internal->dev_attached, 1);
+ }
rte_atomic32_set(&internal->started, 1);
- update_queuing_status(dev);
+ update_queuing_status(eth_dev);
return 0;
}