DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage
@ 2018-01-31 17:46 Stefan Hajnoczi
  2018-01-31 17:46 ` [dpdk-dev] [PATCH 1/2] vhost: add flag for built-in virtio_net.c driver Stefan Hajnoczi
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Stefan Hajnoczi @ 2018-01-31 17:46 UTC (permalink / raw)
  To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, Stefan Hajnoczi

These patches fix a recent regression in librte_vhost that breaks the
vhost_scsi example application.  vhost_user.c assumes all devices are vhost net
backends when handling the VIRTIO_NET_F_MQ feature bit.  The code is triggered
by vhost scsi devices and causes virtqueues to be removed.  See Patch 2 for
details.

Patch 1 puts the infrastructure in place to distinguish between the built-in
virtio_net.c driver and generic vhost device backend usage.

Patch 2 fixes the regression by handling VIRTIO_NET_F_MQ only when the built-in
virtio_net.c driver is in use.

Stefan Hajnoczi (2):
  vhost: add flag for built-in virtio_net.c driver
  vhost: only drop vqs with built-in virtio_net.c driver

 lib/librte_vhost/vhost.h      |  3 +++
 lib/librte_vhost/socket.c     | 15 +++++++++++++++
 lib/librte_vhost/vhost.c      | 17 ++++++++++++++++-
 lib/librte_vhost/vhost_user.c |  3 ++-
 lib/librte_vhost/virtio_net.c | 14 ++++++++++++++
 5 files changed, 50 insertions(+), 2 deletions(-)

-- 
2.14.3

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

* [dpdk-dev] [PATCH 1/2] vhost: add flag for built-in virtio_net.c driver
  2018-01-31 17:46 [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Stefan Hajnoczi
@ 2018-01-31 17:46 ` Stefan Hajnoczi
  2018-02-01 14:46   ` Yuanhan Liu
  2018-01-31 17:46 ` [dpdk-dev] [PATCH 2/2] vhost: only drop vqs with " Stefan Hajnoczi
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Stefan Hajnoczi @ 2018-01-31 17:46 UTC (permalink / raw)
  To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, Stefan Hajnoczi

The librte_vhost API is used in two ways:
1. As a vhost net device backend via rte_vhost_enqueue/dequeue_burst().
2. As a library for implementing vhost device backends.

There is no distinction between the two at the API level or in the
librte_vhost implementation.  For example, device state is kept in
"struct virtio_net" regardless of whether this is actually a net device
backend or whether the built-in virtio_net.c driver is in use.

The virtio_net.c driver should be a librte_vhost API client just like
the vhost-scsi code and have no special access to vhost.h internals.
Unfortunately, fixing this requires significant librte_vhost API
changes.

This patch takes a different approach: keep the librte_vhost API
unchanged but track whether the built-in virtio_net.c driver is in use.
See the next patch for a bug fix that requires knowledge of whether
virtio_net.c is in use.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost.h      |  3 +++
 lib/librte_vhost/socket.c     | 15 +++++++++++++++
 lib/librte_vhost/vhost.c      | 17 ++++++++++++++++-
 lib/librte_vhost/virtio_net.c | 14 ++++++++++++++
 4 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index ba805843b..8f24d4c7e 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -24,6 +24,8 @@
 #define VIRTIO_DEV_RUNNING 1
 /* Used to indicate that the device is ready to operate */
 #define VIRTIO_DEV_READY 2
+/* Used to indicate that the built-in vhost net device backend is enabled */
+#define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
 
 /* Backend value set by guest. */
 #define VIRTIO_DEV_STOPPED -1
@@ -353,6 +355,7 @@ int alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx);
 
 void vhost_set_ifname(int, const char *if_name, unsigned int if_len);
 void vhost_enable_dequeue_zero_copy(int vid);
+void vhost_set_builtin_virtio_net(int vid, bool enable);
 
 struct vhost_device_ops const *vhost_driver_callback_get(const char *path);
 
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 6e3857e7a..83befdced 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -40,6 +40,7 @@ struct vhost_user_socket {
 	bool reconnect;
 	bool dequeue_zero_copy;
 	bool iommu_support;
+	bool use_builtin_virtio_net;
 
 	/*
 	 * The "supported_features" indicates the feature bits the
@@ -195,6 +196,8 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 	size = strnlen(vsocket->path, PATH_MAX);
 	vhost_set_ifname(vid, vsocket->path, size);
 
+	vhost_set_builtin_virtio_net(vid, vsocket->use_builtin_virtio_net);
+
 	if (vsocket->dequeue_zero_copy)
 		vhost_enable_dequeue_zero_copy(vid);
 
@@ -527,6 +530,12 @@ rte_vhost_driver_disable_features(const char *path, uint64_t features)
 
 	pthread_mutex_lock(&vhost_user.mutex);
 	vsocket = find_vhost_user_socket(path);
+
+	/* Note that use_builtin_virtio_net is not affected by this function
+	 * since callers may want to selectively disable features of the
+	 * built-in vhost net device backend.
+	 */
+
 	if (vsocket)
 		vsocket->features &= ~features;
 	pthread_mutex_unlock(&vhost_user.mutex);
@@ -567,6 +576,11 @@ rte_vhost_driver_set_features(const char *path, uint64_t features)
 	if (vsocket) {
 		vsocket->supported_features = features;
 		vsocket->features = features;
+
+		/* Anyone setting feature bits is implementing their own vhost
+		 * device backend.
+		 */
+		vsocket->use_builtin_virtio_net = false;
 	}
 	pthread_mutex_unlock(&vhost_user.mutex);
 
@@ -647,6 +661,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
 	 * rte_vhost_driver_set_features(), which will overwrite following
 	 * two values.
 	 */
+	vsocket->use_builtin_virtio_net = true;
 	vsocket->supported_features = VIRTIO_NET_SUPPORTED_FEATURES;
 	vsocket->features           = VIRTIO_NET_SUPPORTED_FEATURES;
 
diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 1dd9adbc7..a31ca5002 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -251,7 +251,7 @@ reset_device(struct virtio_net *dev)
 
 	dev->features = 0;
 	dev->protocol_features = 0;
-	dev->flags = 0;
+	dev->flags &= VIRTIO_DEV_BUILTIN_VIRTIO_NET;
 
 	for (i = 0; i < dev->nr_vring; i++)
 		reset_vring_queue(dev, i);
@@ -287,6 +287,7 @@ vhost_new_device(void)
 
 	vhost_devices[i] = dev;
 	dev->vid = i;
+	dev->flags = VIRTIO_DEV_BUILTIN_VIRTIO_NET;
 	dev->slave_req_fd = -1;
 
 	return i;
@@ -343,6 +344,20 @@ vhost_enable_dequeue_zero_copy(int vid)
 	dev->dequeue_zero_copy = 1;
 }
 
+void
+vhost_set_builtin_virtio_net(int vid, bool enable)
+{
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL)
+		return;
+
+	if (enable)
+		dev->flags |= VIRTIO_DEV_BUILTIN_VIRTIO_NET;
+	else
+		dev->flags &= ~VIRTIO_DEV_BUILTIN_VIRTIO_NET;
+}
+
 int
 rte_vhost_get_mtu(int vid, uint16_t *mtu)
 {
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index edfab3ba6..700aca7ce 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -703,6 +703,13 @@ rte_vhost_enqueue_burst(int vid, uint16_t queue_id,
 	if (!dev)
 		return 0;
 
+	if (unlikely(!(dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET))) {
+		RTE_LOG(ERR, VHOST_DATA,
+			"(%d) %s: built-in vhost net backend is disabled.\n",
+			dev->vid, __func__);
+		return 0;
+	}
+
 	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF))
 		return virtio_dev_merge_rx(dev, queue_id, pkts, count);
 	else
@@ -1128,6 +1135,13 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
 	if (!dev)
 		return 0;
 
+	if (unlikely(!(dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET))) {
+		RTE_LOG(ERR, VHOST_DATA,
+			"(%d) %s: built-in vhost net backend is disabled.\n",
+			dev->vid, __func__);
+		return 0;
+	}
+
 	if (unlikely(!is_valid_virt_queue_idx(queue_id, 1, dev->nr_vring))) {
 		RTE_LOG(ERR, VHOST_DATA, "(%d) %s: invalid virtqueue idx %d.\n",
 			dev->vid, __func__, queue_id);
-- 
2.14.3

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

* [dpdk-dev] [PATCH 2/2] vhost: only drop vqs with built-in virtio_net.c driver
  2018-01-31 17:46 [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Stefan Hajnoczi
  2018-01-31 17:46 ` [dpdk-dev] [PATCH 1/2] vhost: add flag for built-in virtio_net.c driver Stefan Hajnoczi
@ 2018-01-31 17:46 ` Stefan Hajnoczi
  2018-01-31 18:07   ` Maxime Coquelin
  2018-02-01 12:58 ` [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Maxime Coquelin
  2018-02-01 14:46 ` Yuanhan Liu
  3 siblings, 1 reply; 9+ messages in thread
From: Stefan Hajnoczi @ 2018-01-31 17:46 UTC (permalink / raw)
  To: dev; +Cc: Maxime Coquelin, Yuanhan Liu, Stefan Hajnoczi

Commit e29109323595beb3884da58126ebb3b878cb66f5 ("vhost: destroy unused
virtqueues when multiqueue not negotiated") broke vhost-scsi by removing
virtqueues when the virtio-net-specific VIRTIO_NET_F_MQ feature bit is
missing.

The vhost_user.c code shouldn't assume all devices are vhost net device
backends.  Use the new VIRTIO_DEV_BUILTIN_VIRTIO_NET flag to check
whether virtio_net.c is being used.

This fixes examples/vhost_scsi.

Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 1dd1a61b6..65ee33919 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -187,7 +187,8 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 		(dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
 		(dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
 
-	if (!(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
+	if ((dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET) &&
+	    !(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
 		/*
 		 * Remove all but first queue pair if MQ hasn't been
 		 * negotiated. This is safe because the device is not
-- 
2.14.3

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

* Re: [dpdk-dev] [PATCH 2/2] vhost: only drop vqs with built-in virtio_net.c driver
  2018-01-31 17:46 ` [dpdk-dev] [PATCH 2/2] vhost: only drop vqs with " Stefan Hajnoczi
@ 2018-01-31 18:07   ` Maxime Coquelin
       [not found]     ` <20180201102428.GA5783@stefanha-x1.localdomain>
  0 siblings, 1 reply; 9+ messages in thread
From: Maxime Coquelin @ 2018-01-31 18:07 UTC (permalink / raw)
  To: Stefan Hajnoczi, dev; +Cc: Yuanhan Liu

Hi Stefan,

On 01/31/2018 06:46 PM, Stefan Hajnoczi wrote:
> Commit e29109323595beb3884da58126ebb3b878cb66f5 ("vhost: destroy unused
> virtqueues when multiqueue not negotiated") broke vhost-scsi by removing
> virtqueues when the virtio-net-specific VIRTIO_NET_F_MQ feature bit is
> missing.
> 
> The vhost_user.c code shouldn't assume all devices are vhost net device
> backends.  Use the new VIRTIO_DEV_BUILTIN_VIRTIO_NET flag to check
> whether virtio_net.c is being used.
> 
> This fixes examples/vhost_scsi.
> 
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   lib/librte_vhost/vhost_user.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 1dd1a61b6..65ee33919 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -187,7 +187,8 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>   		(dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
>   		(dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
>   
> -	if (!(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
> +	if ((dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET) &&
> +	    !(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {

If we had an external net backend using the library, we would also need 
to remove extra queues if it advertised VIRTIO_NET_F_MQ, but the feature
isn't negotiated.

In this case, the fix I suggested yesterday would work:
if ((vhost_features & (1ULL << VIRTIO_NET_F_MQ)) &&
     !(dev->features & (1ULL << VIRTIO_NET_F_MQ)) {
...
}

For any backend that does not advertise the feature, no queues will be
destroyed.

Thanks,
Maxime
>   		/*
>   		 * Remove all but first queue pair if MQ hasn't been
>   		 * negotiated. This is safe because the device is not
> 

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

* Re: [dpdk-dev] [PATCH 2/2] vhost: only drop vqs with built-in virtio_net.c driver
       [not found]     ` <20180201102428.GA5783@stefanha-x1.localdomain>
@ 2018-02-01 12:49       ` Maxime Coquelin
  0 siblings, 0 replies; 9+ messages in thread
From: Maxime Coquelin @ 2018-02-01 12:49 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: dev, Yuanhan Liu



On 02/01/2018 11:24 AM, Stefan Hajnoczi wrote:
> On Wed, Jan 31, 2018 at 07:07:50PM +0100, Maxime Coquelin wrote:
>> Hi Stefan,
>>
>> On 01/31/2018 06:46 PM, Stefan Hajnoczi wrote:
>>> Commit e29109323595beb3884da58126ebb3b878cb66f5 ("vhost: destroy unused
>>> virtqueues when multiqueue not negotiated") broke vhost-scsi by removing
>>> virtqueues when the virtio-net-specific VIRTIO_NET_F_MQ feature bit is
>>> missing.
>>>
>>> The vhost_user.c code shouldn't assume all devices are vhost net device
>>> backends.  Use the new VIRTIO_DEV_BUILTIN_VIRTIO_NET flag to check
>>> whether virtio_net.c is being used.
>>>
>>> This fixes examples/vhost_scsi.
>>>
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>    lib/librte_vhost/vhost_user.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 1dd1a61b6..65ee33919 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -187,7 +187,8 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>>>    		(dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
>>>    		(dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
>>> -	if (!(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
>>> +	if ((dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET) &&
>>> +	    !(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
>>
>> If we had an external net backend using the library, we would also need to
>> remove extra queues if it advertised VIRTIO_NET_F_MQ, but the feature
>> isn't negotiated.
> 
> A net-specific fix inside librte_vhost is not enough since non-net
> drivers may only initialize a subset of the virtqueues.

You are right, and even with net backend, the driver may decide to only
initialize a subset of the virtqueues even if VIRTIO_NET_F_MQ is
negotiated.

This is not the case today with virtio-net Linux kernel driver and
DPDK's Virtio PMD, but Windows virtio-net driver only initialize as much
queue pairs as online vCPUs.

> I suggest starting over.  Get rid of the net-specific fix and instead
> look at when new_device() needs to be called.

I agree, and already started to work on it. But my understanding is that
we will need a vhost-user protocol update. So I implemented this
workaround to support the VIRTIO_NET_F_MQ not negotiated case, which
happens with iPXE.

> Virtqueues will not be changed after the DRIVER_OK status bit has been
> set.  The VIRTIO 1.0 specification says, "The device MUST NOT consume
> buffers before DRIVER_OK, and the driver MUST NOT notify the device
> before it sets DRIVER_OK" (3.1 Device Initialization).
> 
> http://docs.oasis-open.org/virtio/virtio/v1.0/csprd01/virtio-v1.0-csprd01.html#x1-230001
> 
> However, it also says "legacy device implementations often used the
> device before setting the DRIVER_OK bit" (3.1.1 Legacy Interface: Device
> Initialization).
> 
> VIRTIO 1.0 can be supported fine by the current librte_vhost API.
> Legacy cannot be supported without API changes - there is no magic way
> to detect when new_device() can be invoked if the driver might require
> some virtqueue processing before the device is fully initialized.
> 
> I think this is the main discussion that needs to happen.  This patch
> series and the original VIRTIO_NET_F_MQ fix are just workarounds for the
> real problem.

Yes.

>> In this case, the fix I suggested yesterday would work:
>> if ((vhost_features & (1ULL << VIRTIO_NET_F_MQ)) &&
>>      !(dev->features & (1ULL << VIRTIO_NET_F_MQ)) {
>> ...
>> }
>>
>> For any backend that does not advertise the feature, no queues will be
>> destroyed.
> 
> The feature bit space is shared by all device types.  Another device can
> use bit 22 (VIRTIO_NET_F_MQ) for another purpose.  This code would
> incorrectly assume it's a net device.

Thanks for pointing this out, I missed that.

> No other device type in VIRTIO 1.0 uses bit 22 yet, but this solution is
> not future-proof.  If you decide to use your fix, please include a
> comment in the code so whoever needs to debug this again in the future
> can spot the problem more quickly.

No, I agree this is not future proof. I now think your patch is better.

Thanks for the insights!
Maxime

> Stefan
> 

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

* Re: [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage
  2018-01-31 17:46 [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Stefan Hajnoczi
  2018-01-31 17:46 ` [dpdk-dev] [PATCH 1/2] vhost: add flag for built-in virtio_net.c driver Stefan Hajnoczi
  2018-01-31 17:46 ` [dpdk-dev] [PATCH 2/2] vhost: only drop vqs with " Stefan Hajnoczi
@ 2018-02-01 12:58 ` Maxime Coquelin
  2018-02-05 14:20   ` Ferruh Yigit
  2018-02-01 14:46 ` Yuanhan Liu
  3 siblings, 1 reply; 9+ messages in thread
From: Maxime Coquelin @ 2018-02-01 12:58 UTC (permalink / raw)
  To: Stefan Hajnoczi, dev; +Cc: Yuanhan Liu



On 01/31/2018 06:46 PM, Stefan Hajnoczi wrote:
> These patches fix a recent regression in librte_vhost that breaks the
> vhost_scsi example application.  vhost_user.c assumes all devices are vhost net
> backends when handling the VIRTIO_NET_F_MQ feature bit.  The code is triggered
> by vhost scsi devices and causes virtqueues to be removed.  See Patch 2 for
> details.
> 
> Patch 1 puts the infrastructure in place to distinguish between the built-in
> virtio_net.c driver and generic vhost device backend usage.
> 
> Patch 2 fixes the regression by handling VIRTIO_NET_F_MQ only when the built-in
> virtio_net.c driver is in use.
> 
> Stefan Hajnoczi (2):
>    vhost: add flag for built-in virtio_net.c driver
>    vhost: only drop vqs with built-in virtio_net.c driver
> 
>   lib/librte_vhost/vhost.h      |  3 +++
>   lib/librte_vhost/socket.c     | 15 +++++++++++++++
>   lib/librte_vhost/vhost.c      | 17 ++++++++++++++++-
>   lib/librte_vhost/vhost_user.c |  3 ++-
>   lib/librte_vhost/virtio_net.c | 14 ++++++++++++++
>   5 files changed, 50 insertions(+), 2 deletions(-)
> 

For the series:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

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

* Re: [dpdk-dev] [PATCH 1/2] vhost: add flag for built-in virtio_net.c driver
  2018-01-31 17:46 ` [dpdk-dev] [PATCH 1/2] vhost: add flag for built-in virtio_net.c driver Stefan Hajnoczi
@ 2018-02-01 14:46   ` Yuanhan Liu
  0 siblings, 0 replies; 9+ messages in thread
From: Yuanhan Liu @ 2018-02-01 14:46 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: dev, Maxime Coquelin

Hi Stefan,

On Wed, Jan 31, 2018 at 05:46:50PM +0000, Stefan Hajnoczi wrote:
> The librte_vhost API is used in two ways:
> 1. As a vhost net device backend via rte_vhost_enqueue/dequeue_burst().

This is how DPDK vhost-user firstly implemented.

> 2. As a library for implementing vhost device backends.

This is how DPDK vhost-use extended later, and vhost-user scsi is
the first one being added.

> There is no distinction between the two at the API level or in the
> librte_vhost implementation.  For example, device state is kept in
> "struct virtio_net" regardless of whether this is actually a net device
> backend or whether the built-in virtio_net.c driver is in use.

Indeed. virtio_net should be renamed to "vhost_dev" or something like
this. It's part of something un-finished in the last vhost-user extension
refactoring.

> 
> The virtio_net.c driver should be a librte_vhost API client just like
> the vhost-scsi code and have no special access to vhost.h internals.
> Unfortunately, fixing this requires significant librte_vhost API
> changes.

The way I thought was to move the virtio_net.c completely to vhost
pmd (drivers/net/vhost). And let vhost-user just be a generic lib
without any device specific stuff.

Unfortunately, it can not be done recently, as there are still a lot
of applications using rte_vhost_enqueue/dequeue_burst directly, for
example, OVS.

> This patch takes a different approach: keep the librte_vhost API
> unchanged but track whether the built-in virtio_net.c driver is in use.
> See the next patch for a bug fix that requires knowledge of whether
> virtio_net.c is in use.

LGTM.

Thanks.

	--yliu

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

* Re: [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage
  2018-01-31 17:46 [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Stefan Hajnoczi
                   ` (2 preceding siblings ...)
  2018-02-01 12:58 ` [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Maxime Coquelin
@ 2018-02-01 14:46 ` Yuanhan Liu
  3 siblings, 0 replies; 9+ messages in thread
From: Yuanhan Liu @ 2018-02-01 14:46 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: dev, Maxime Coquelin

On Wed, Jan 31, 2018 at 05:46:49PM +0000, Stefan Hajnoczi wrote:
> These patches fix a recent regression in librte_vhost that breaks the
> vhost_scsi example application.  vhost_user.c assumes all devices are vhost net
> backends when handling the VIRTIO_NET_F_MQ feature bit.  The code is triggered
> by vhost scsi devices and causes virtqueues to be removed.  See Patch 2 for
> details.
> 
> Patch 1 puts the infrastructure in place to distinguish between the built-in
> virtio_net.c driver and generic vhost device backend usage.
> 
> Patch 2 fixes the regression by handling VIRTIO_NET_F_MQ only when the built-in
> virtio_net.c driver is in use.
> 
> Stefan Hajnoczi (2):
>   vhost: add flag for built-in virtio_net.c driver
>   vhost: only drop vqs with built-in virtio_net.c driver

Series Acked-by: Yuanhan Liu <yliu@fridaylinux.org>

Thanks.

	--yliu
> 
>  lib/librte_vhost/vhost.h      |  3 +++
>  lib/librte_vhost/socket.c     | 15 +++++++++++++++
>  lib/librte_vhost/vhost.c      | 17 ++++++++++++++++-
>  lib/librte_vhost/vhost_user.c |  3 ++-
>  lib/librte_vhost/virtio_net.c | 14 ++++++++++++++
>  5 files changed, 50 insertions(+), 2 deletions(-)
> 
> -- 
> 2.14.3

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

* Re: [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage
  2018-02-01 12:58 ` [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Maxime Coquelin
@ 2018-02-05 14:20   ` Ferruh Yigit
  0 siblings, 0 replies; 9+ messages in thread
From: Ferruh Yigit @ 2018-02-05 14:20 UTC (permalink / raw)
  To: Maxime Coquelin, Stefan Hajnoczi, dev; +Cc: Yuanhan Liu

On 2/1/2018 12:58 PM, Maxime Coquelin wrote:
> 
> 
> On 01/31/2018 06:46 PM, Stefan Hajnoczi wrote:
>> These patches fix a recent regression in librte_vhost that breaks the
>> vhost_scsi example application.  vhost_user.c assumes all devices are vhost net
>> backends when handling the VIRTIO_NET_F_MQ feature bit.  The code is triggered
>> by vhost scsi devices and causes virtqueues to be removed.  See Patch 2 for
>> details.
>>
>> Patch 1 puts the infrastructure in place to distinguish between the built-in
>> virtio_net.c driver and generic vhost device backend usage.
>>
>> Patch 2 fixes the regression by handling VIRTIO_NET_F_MQ only when the built-in
>> virtio_net.c driver is in use.
>>
>> Stefan Hajnoczi (2):
>>    vhost: add flag for built-in virtio_net.c driver
>>    vhost: only drop vqs with built-in virtio_net.c driver
>>
>>   lib/librte_vhost/vhost.h      |  3 +++
>>   lib/librte_vhost/socket.c     | 15 +++++++++++++++
>>   lib/librte_vhost/vhost.c      | 17 ++++++++++++++++-
>>   lib/librte_vhost/vhost_user.c |  3 ++-
>>   lib/librte_vhost/virtio_net.c | 14 ++++++++++++++
>>   5 files changed, 50 insertions(+), 2 deletions(-)
>>
> 
> For the series:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Series
Acked-by: Yuanhan Liu <yliu@fridaylinux.org>

Series applied to dpdk-next-net/master, thanks.

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

end of thread, other threads:[~2018-02-05 14:20 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-31 17:46 [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Stefan Hajnoczi
2018-01-31 17:46 ` [dpdk-dev] [PATCH 1/2] vhost: add flag for built-in virtio_net.c driver Stefan Hajnoczi
2018-02-01 14:46   ` Yuanhan Liu
2018-01-31 17:46 ` [dpdk-dev] [PATCH 2/2] vhost: only drop vqs with " Stefan Hajnoczi
2018-01-31 18:07   ` Maxime Coquelin
     [not found]     ` <20180201102428.GA5783@stefanha-x1.localdomain>
2018-02-01 12:49       ` Maxime Coquelin
2018-02-01 12:58 ` [dpdk-dev] [PATCH 0/2] vhost: fix VIRTIO_NET_F_MQ vhost_scsi breakage Maxime Coquelin
2018-02-05 14:20   ` Ferruh Yigit
2018-02-01 14:46 ` Yuanhan Liu

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).