[dpdk-dev,v3] net/vhost: fix segfault when creating vdev dynamically

Message ID 20180330065831.107558-1-junjie.j.chen@intel.com (mailing list archive)
State Accepted, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply issues

Commit Message

junjie.j.chen@intel.com March 30, 2018, 6:58 a.m. UTC
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

Yang, Zhiyong March 30, 2018, 7:32 a.m. UTC | #1
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
  
Maxime Coquelin March 30, 2018, 7:35 a.m. UTC | #2
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
  
Maxime Coquelin March 30, 2018, 7:36 a.m. UTC | #3
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
>
  
Maxime Coquelin March 30, 2018, 7:43 a.m. UTC | #4
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
  
Jens Freimann April 9, 2018, 12:37 p.m. UTC | #5
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
  
junjie.j.chen@intel.com April 10, 2018, 8:11 a.m. UTC | #6
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
  

Patch

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 3aae01c39..11b607650 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -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;
 }