Re: [dpdk-stable] [dpdk-dev] [PATCH 0/3] fix virtio_user issues

Re: [dpdk-stable] [dpdk-dev] [PATCH 0/3] fix virtio_user issues

[Christian Ehrhardt]

On Fri, Aug 5, 2016 at 1:36 PM, Jianfeng Tan <jianfeng.tan@intel.com> wrote:

> Patch 1: fix issue when using virtio_user with OVS-DPDK.
> Patch 2: fix issue when using virtio_user with VPP.
> Patch 3: fix issue when failing to start virtio_user devices.
>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>
> Jianfeng Tan (3):
>   net/virtio_user: fix queue pair not enabled
>   net/virtio_user: fix wrong sequence of messages
>   net/virtio_user: fix dev not freed after init error
>
>
Hi,
I think those were post 16.07 - would they be reasonable for the stable
tree?

[dpdk-stable] [PATCH v2 0/3] fix virtio_user issues

[Jianfeng Tan]

v2:
  -- Patch 1: Enable 1st queue pair unconditionaly, instead of depending
  on VHOST_USER_GET_PROTOCOL_FEATURES.
  -- Patch 3: address Stephen's comment on confusion laying in two local
  variables named ret and result. Now we just keep one.

Patch 1: fix issue when using virtio_user with OVS-DPDK.
Patch 2: fix issue when using virtio_user with VPP.
Patch 3: fix issue when failing to start virtio_user devices.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Jianfeng Tan (3):
  net/virtio_user: fix queue pair not enabled
  net/virtio_user: fix wrong sequence of messages
  net/virtio_user: fix dev not freed after init error

 drivers/net/virtio/virtio_user/virtio_user_dev.c | 112 ++++++++++++++---------
 drivers/net/virtio/virtio_user_ethdev.c          |  42 +++++----
 2 files changed, 96 insertions(+), 58 deletions(-)

-- 
2.7.4

[dpdk-stable] [PATCH v2 1/3] net/virtio_user: fix queue pair not enabled

[Jianfeng Tan]

When virtio_user is used with OVS-DPDK (with mq disabled), it cannot
receive any packets.

It's because when vhost provides VHOST_USER_GET_PROTOCOL_FEATURES,
all queue pairs are initialized in the disabled state. Quote
QEMU/docs/specs/vhost-user.txt:
    If VHOST_USER_F_PROTOCOL_FEATURES has not been negotiated, the
    ring is initialized in an enabled state.
    If VHOST_USER_F_PROTOCOL_FEATURES has been negotiated, the ring
    is initialized in a disabled state.

In OVS-DPDK, all queue pairs are in the disabled state by default.
When used with QEMU, QEMU will set it as enabled in the process of
initialization. So this patch is to add similar logic in virtio_user.

Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")

Reported-by: Ning Li <lining18@jd.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 376c9cf..bf1155c 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -131,6 +131,9 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 		}
 	}
 
+	/* we enable the 1st queue pair by default. */
+	vhost_user_enable_queue_pair(dev->vhostfd, 0, 1);
+
 	/* After setup all virtqueues, we need to set_features so that these
 	 * features can be set into each virtqueue in vhost side. And before
 	 * that, make sure VHOST_USER_F_PROTOCOL_FEATURES is added if mq is
-- 
2.7.4

[dpdk-stable] [PATCH v2 2/3] net/virtio_user: fix wrong sequence of messages

[Jianfeng Tan]

When virtio_user is used with VPP's native vhost user, it cannot
send/receive any packets.

The root cause is that vpp-vhost-user translates the message
VHOST_USER_SET_FEATURES as puting this device into init state,
aka, zero all related structures. However, previous code
puts this message at last in the whole initialization process,
which leads to all previous information are zeroed.

To fix this issue, we rearrange the sequence of those messages.
  - step 0, send VHOST_USER_SET_VRING_CALL so that vhost allocates
    virtqueue structures;
  - step 1, send VHOST_USER_SET_FEATURES to confirm the features;
  - step 2, send VHOST_USER_SET_MEM_TABLE to share mem regions;
  - step 3, send VHOST_USER_SET_VRING_NUM, VHOST_USER_SET_VRING_BASE,
    VHOST_USER_SET_VRING_ADDR, VHOST_USER_SET_VRING_KICK for each
    queue;
  - ...

Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")

Reported-by: Zhihong Wang <zhihong.wang@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 113 ++++++++++++++---------
 1 file changed, 69 insertions(+), 44 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index bf1155c..e239e0e 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -45,20 +45,14 @@
 #include "../virtio_ethdev.h"
 
 static int
-virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
+virtio_user_create_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
 {
-	int callfd, kickfd;
+	/* Of all per virtqueue MSGs, make sure VHOST_SET_VRING_CALL come
+	 * firstly because vhost depends on this msg to allocate virtqueue
+	 * pair.
+	 */
+	int callfd;
 	struct vhost_vring_file file;
-	struct vhost_vring_state state;
-	struct vring *vring = &dev->vrings[queue_sel];
-	struct vhost_vring_addr addr = {
-		.index = queue_sel,
-		.desc_user_addr = (uint64_t)(uintptr_t)vring->desc,
-		.avail_user_addr = (uint64_t)(uintptr_t)vring->avail,
-		.used_user_addr = (uint64_t)(uintptr_t)vring->used,
-		.log_guest_addr = 0,
-		.flags = 0, /* disable log */
-	};
 
 	/* May use invalid flag, but some backend leverages kickfd and callfd as
 	 * criteria to judge if dev is alive. so finally we use real event_fd.
@@ -68,22 +62,30 @@ virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
 		PMD_DRV_LOG(ERR, "callfd error, %s\n", strerror(errno));
 		return -1;
 	}
-	kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
-	if (kickfd < 0) {
-		close(callfd);
-		PMD_DRV_LOG(ERR, "kickfd error, %s\n", strerror(errno));
-		return -1;
-	}
-
-	/* Of all per virtqueue MSGs, make sure VHOST_SET_VRING_CALL come
-	 * firstly because vhost depends on this msg to allocate virtqueue
-	 * pair.
-	 */
 	file.index = queue_sel;
 	file.fd = callfd;
 	vhost_user_sock(dev->vhostfd, VHOST_USER_SET_VRING_CALL, &file);
 	dev->callfds[queue_sel] = callfd;
 
+	return 0;
+}
+
+static int
+virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
+{
+	int kickfd;
+	struct vhost_vring_file file;
+	struct vhost_vring_state state;
+	struct vring *vring = &dev->vrings[queue_sel];
+	struct vhost_vring_addr addr = {
+		.index = queue_sel,
+		.desc_user_addr = (uint64_t)(uintptr_t)vring->desc,
+		.avail_user_addr = (uint64_t)(uintptr_t)vring->avail,
+		.used_user_addr = (uint64_t)(uintptr_t)vring->used,
+		.log_guest_addr = 0,
+		.flags = 0, /* disable log */
+	};
+
 	state.index = queue_sel;
 	state.num = vring->num;
 	vhost_user_sock(dev->vhostfd, VHOST_USER_SET_VRING_NUM, &state);
@@ -97,6 +99,12 @@ virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
 	 * lastly because vhost depends on this msg to judge if
 	 * virtio is ready.
 	 */
+	kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
+	if (kickfd < 0) {
+		PMD_DRV_LOG(ERR, "kickfd error, %s\n", strerror(errno));
+		return -1;
+	}
+	file.index = queue_sel;
 	file.fd = kickfd;
 	vhost_user_sock(dev->vhostfd, VHOST_USER_SET_VRING_KICK, &file);
 	dev->kickfds[queue_sel] = kickfd;
@@ -104,40 +112,43 @@ virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
 	return 0;
 }
 
-int
-virtio_user_start_device(struct virtio_user_dev *dev)
+static int
+virtio_user_queue_setup(struct virtio_user_dev *dev,
+			int (*fn)(struct virtio_user_dev *, uint32_t))
 {
-	uint64_t features;
 	uint32_t i, queue_sel;
-	int ret;
-
-	/* construct memory region inside each implementation */
-	ret = vhost_user_sock(dev->vhostfd, VHOST_USER_SET_MEM_TABLE, NULL);
-	if (ret < 0)
-		goto error;
 
 	for (i = 0; i < dev->max_queue_pairs; ++i) {
 		queue_sel = 2 * i + VTNET_SQ_RQ_QUEUE_IDX;
-		if (virtio_user_kick_queue(dev, queue_sel) < 0) {
-			PMD_DRV_LOG(INFO, "kick rx vq fails: %u", i);
-			goto error;
+		if (fn(dev, queue_sel) < 0) {
+			PMD_DRV_LOG(INFO, "setup rx vq fails: %u", i);
+			return -1;
 		}
 	}
 	for (i = 0; i < dev->max_queue_pairs; ++i) {
 		queue_sel = 2 * i + VTNET_SQ_TQ_QUEUE_IDX;
-		if (virtio_user_kick_queue(dev, queue_sel) < 0) {
-			PMD_DRV_LOG(INFO, "kick tx vq fails: %u", i);
-			goto error;
+		if (fn(dev, queue_sel) < 0) {
+			PMD_DRV_LOG(INFO, "setup tx vq fails: %u", i);
+			return -1;
 		}
 	}
 
-	/* we enable the 1st queue pair by default. */
-	vhost_user_enable_queue_pair(dev->vhostfd, 0, 1);
+	return 0;
+}
+
+int
+virtio_user_start_device(struct virtio_user_dev *dev)
+{
+	uint64_t features;
+	int ret;
+
+	/* Step 0: tell vhost to create queues */
+	if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
+		goto error;
 
-	/* After setup all virtqueues, we need to set_features so that these
-	 * features can be set into each virtqueue in vhost side. And before
-	 * that, make sure VHOST_USER_F_PROTOCOL_FEATURES is added if mq is
-	 * enabled, and VIRTIO_NET_F_MAC is stripped.
+	/* Step 1: set features
+	 * Make sure VHOST_USER_F_PROTOCOL_FEATURES is added if mq is enabled,
+	 * and VIRTIO_NET_F_MAC is stripped.
 	 */
 	features = dev->features;
 	if (dev->max_queue_pairs > 1)
@@ -148,6 +159,20 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 		goto error;
 	PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
 
+	/* Step 2: share memory regions */
+	ret = vhost_user_sock(dev->vhostfd, VHOST_USER_SET_MEM_TABLE, NULL);
+	if (ret < 0)
+		goto error;
+
+	/* Step 3: kick queues */
+	if (virtio_user_queue_setup(dev, virtio_user_kick_queue) < 0)
+		goto error;
+
+	/* Step 4: enable queues
+	 * we enable the 1st queue pair by default.
+	 */
+	vhost_user_enable_queue_pair(dev->vhostfd, 0, 1);
+
 	return 0;
 error:
 	/* TODO: free resource here or caller to check */
-- 
2.7.4

[dpdk-stable] [PATCH v2 3/3] net/virtio_user: fix dev not freed after init error

[Jianfeng Tan]

Currently, when virtio_user device fails to be started (e.g., vhost
unix socket does not exit), the init function does not return struct
rte_eth_dev (and some other structs) back to ether layer. And what's
more, it does not report the error to upper layer.

The fix is to free those structs and report error when failing to
start virtio_user devices.

Fixes: ce2eabdd43ec ("net/virtio-user: add virtual device")

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 42 ++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index daef09b..bba7402 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -313,6 +313,17 @@ virtio_user_eth_dev_alloc(const char *name)
 	return eth_dev;
 }
 
+static void
+virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev)
+{
+	struct rte_eth_dev_data *data = eth_dev->data;
+	struct virtio_hw *hw = data->dev_private;
+
+	rte_free(hw->virtio_user_dev);
+	rte_free(hw);
+	rte_eth_dev_release_port(eth_dev);
+}
+
 /* Dev initialization routine. Invoked once for each virtio vdev at
  * EAL init time, see rte_eal_dev_init().
  * Returns 0 on success.
@@ -343,9 +354,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
 	}
 
 	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_PATH) == 1) {
-		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PATH,
-					 &get_string_arg, &path);
-		if (ret < 0) {
+		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PATH,
+				       &get_string_arg, &path) < 0) {
 			PMD_INIT_LOG(ERR, "error to parse %s",
 				     VIRTIO_USER_ARG_PATH);
 			goto end;
@@ -357,9 +367,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
 	}
 
 	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_MAC) == 1) {
-		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_MAC,
-					 &get_string_arg, &mac_addr);
-		if (ret < 0) {
+		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_MAC,
+				       &get_string_arg, &mac_addr) < 0) {
 			PMD_INIT_LOG(ERR, "error to parse %s",
 				     VIRTIO_USER_ARG_MAC);
 			goto end;
@@ -367,9 +376,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
 	}
 
 	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE) == 1) {
-		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE,
-					 &get_integer_arg, &queue_size);
-		if (ret < 0) {
+		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE,
+				       &get_integer_arg, &queue_size) < 0) {
 			PMD_INIT_LOG(ERR, "error to parse %s",
 				     VIRTIO_USER_ARG_QUEUE_SIZE);
 			goto end;
@@ -377,9 +385,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
 	}
 
 	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_QUEUES_NUM) == 1) {
-		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUES_NUM,
-					 &get_integer_arg, &queues);
-		if (ret < 0) {
+		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUES_NUM,
+				       &get_integer_arg, &queues) < 0) {
 			PMD_INIT_LOG(ERR, "error to parse %s",
 				     VIRTIO_USER_ARG_QUEUES_NUM);
 			goto end;
@@ -387,9 +394,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
 	}
 
 	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1) {
-		ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
-					 &get_integer_arg, &cq);
-		if (ret < 0) {
+		if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
+				       &get_integer_arg, &cq) < 0) {
 			PMD_INIT_LOG(ERR, "error to parse %s",
 				     VIRTIO_USER_ARG_CQ_NUM);
 			goto end;
@@ -411,12 +417,16 @@ virtio_user_pmd_devinit(const char *name, const char *params)
 
 	hw = eth_dev->data->dev_private;
 	if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
-				 queue_size, mac_addr) < 0)
+				 queue_size, mac_addr) < 0) {
+		PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
+		virtio_user_eth_dev_free(eth_dev);
 		goto end;
+	}
 
 	/* previously called by rte_eal_pci_probe() for physical dev */
 	if (eth_virtio_dev_init(eth_dev) < 0) {
 		PMD_INIT_LOG(ERR, "eth_virtio_dev_init fails");
+		virtio_user_eth_dev_free(eth_dev);
 		goto end;
 	}
 	ret = 0;
-- 
2.7.4

Re: [dpdk-stable] [PATCH v2 1/3] net/virtio_user: fix queue pair not enabled

[Yuanhan Liu]

On Tue, Sep 27, 2016 at 07:11:04PM +0000, Jianfeng Tan wrote:
> When virtio_user is used with OVS-DPDK (with mq disabled), it cannot
> receive any packets.
> 
> It's because when vhost provides VHOST_USER_GET_PROTOCOL_FEATURES,
> all queue pairs are initialized in the disabled state. Quote
> QEMU/docs/specs/vhost-user.txt:
>     If VHOST_USER_F_PROTOCOL_FEATURES has not been negotiated, the
>     ring is initialized in an enabled state.
>     If VHOST_USER_F_PROTOCOL_FEATURES has been negotiated, the ring
>     is initialized in a disabled state.

As stated before, it has nothing to do with VHOST_USER_F_PROTOCOL_FEATURES.
You seems forgot to change the commit log.
>
> In OVS-DPDK, all queue pairs are in the disabled state by default.
> When used with QEMU, QEMU will set it as enabled in the process of
> initialization. So this patch is to add similar logic in virtio_user.

I would reword the commit log to someting like following:

    When virtio_user is used with OVS-DPDK (with mq disabled), it cannot
    receive any packets. This is because no queue is enabled at all when
    mq is disabled.
    
    To fix it, we should consistently make sure the 1st queue is
    enabled,which is also the behaviour QEMU takes.
 
> Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")
> 
> Reported-by: Ning Li <lining18@jd.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Series applied to dpdk-next-virtio, with above commit reword.

Thanks.

	--yliu