dpdk virtio_user lack of notifications make vhost_net+napi stops tx buffers

https://bugs.dpdk.org/show_bug.cgi?id=383

Bug ID: 383
Summary: dpdk virtio_user lack of notifications make
vhost_net+napi stops tx buffers
Product: DPDK
Version: unspecified
Hardware: All
OS: Linux
Status: UNCONFIRMED
Severity: normal
Priority: Normal
Component: vhost/virtio
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---

Using the current testpmd vhost_user as:

./app/testpmd -l 6,7,8 --vdev='net_vhost1,iface=/tmp/vhost-user1' --vdev='net_vhost2,iface=/tmp/vhost-user2' -- -a -i --rxq=1 --txq=1 --txd=1024
--forward-mode=rxonly

And starting qemu using packed=on on the interface:

-netdev vhost-user,chardev=charnet1,id=hostnet1 -device virtio-net-pci,rx_queue_size=256,...,packed=on

And start to tx in the guest using:

./dpdk/build/app/testpmd -l 1,2 --vdev=eth_af_packet0,iface=eth0 -- \
    --forward-mode=txonly --txq=1 --txd=256 --auto-start --txpkts

1500 \
--stats-period 1

After first burst of packets (512 or a little more), sendto() will start to
return EBUSY. kernel NAPI is refusing to send more packets to virtio_net device
until it free old skbs.

However, virtio_net driver is unable to free old buffers since host does not return them in vhost_flush_dequeue_packed until shadow queue is full
except for MAX_PKT_BURST (32) packets.

Sometimes we are lucky and reach this point, or packets are small enough to
fill the queue and flush, but if the packets and the virtqueue are big enough,
we will not be able to tx anymore.

-----Original Message-----
From: dev <[email protected]> On Behalf Of [email protected]
Sent: Thursday, January 09, 2020 11:56 PM
To: [email protected]; [email protected]; Maxime Coquelin <[email protected]>
Cc: Jason Wang <[email protected]>; Michael S. Tsirkin <[email protected]>; Adrian Moreno Zapata <[email protected]>
Subject: Re: [dpdk-dev] [Bug 383] dpdk virtio_user lack of notifications make vhost_net+napi stops tx buffers

Proposal for patch - Requesting For Comments.

Just running the shadow copy-flush-call unconditionally in vhost_flush_dequeue_packed solve the issue, and it gives the best latency I can get in the tests (8101.959 trans/sec if I run netperf TCP_RR, 1331.735 trans/sec if I run TCP_STREAM at the same time). Apart from that, testpmd is able to tx about 820Kpps to the guest.

However, to still do a little bit of batching I replace the condition for the one attached here. Although it implies a read barrier, I am able to achieve a little more of throughput (about 890Kpps), reducing to 8048.919 the numbers of transactions/sec in TCP_RR test (1372.327 if it runs in parallel with TCP_STREAM).

I also tried to move the vhost_flush_dequeue_shadow_packed and host_flush_dequeue_shadow_packed after the do_data_copy_dequeue in virtio_dev_tx_packed, more or less the same way virtio_dev_rx_packed do, but I repeatedly find less throughput in this case, even if I add the !next_desc_is_avail(vq) test. Not sure why, since both ways should be very similar. About 836Kpps are achieved this way, and TCP_RR is able to do 8120.154 trans/sec by itself and 1363.341 trans/sec if it runs with another TCP_STREAM test in parallel.

So, is there room for improvement, either in the patches or in the tests? Is one of the solutions preferred over another?

All tests were run with:
* producer in a different processor than consumer (host testpmd and VM never run in the same core)
* 256 descriptors queues in guest's testpmd and tx vq

Thanks!

PS: Sorry for the from mail address change, DPDK bugzilla doesn't send me the confirmation mail to this account.

lib/librte_vhost/virtio_net.c +14 -0

index 21c311732..f7137149c 100644
@@ -382,6 +382,20 @@ vhost_shadow_enqueue_single_packed(struct
 virtio_net *dev,
        }
 }
 
+static __rte_always_inline bool
+next_desc_is_avail(const struct vhost_virtqueue *vq)
+{
+       bool wrap_counter = vq->avail_wrap_counter;
+       uint16_t next_used_idx = vq->last_used_idx + 1;
+
+       if (next_used_idx >= vq->size) {
+               next_used_idx -= vq->size;
+               wrap_counter ^= 1;
+       }
+
+       return desc_is_avail(&vq->desc_packed[next_used_idx],
 wrap_counter);
+}
+
 static __rte_always_inline void

vhost_flush_dequeue_packed(struct virtio_net *dev,
struct vhost_virtqueue *vq)

lib/librte_vhost/virtio_net.c +2 -1

@@ -394,7 +408,8 @@ vhost_flush_dequeue_packed(struct virtio_net *dev,
        if (shadow_count <= 0)
                shadow_count += vq->size;
 
-       if ((uint32_t)shadow_count >= (vq->size - MAX_PKT_BURST)) {
+       if ((uint32_t)shadow_count >= (vq->size - MAX_PKT_BURST)
+               || !next_desc_is_avail(vq)) {
                do_data_copy_dequeue(vq);
                vhost_flush_dequeue_shadow_packed(dev, vq);
                vhost_vring_call_packed(dev, vq);

Quoted 53 lines · On Thu, 2020-01-09 at 15:47 +0000, [email protected] wrote:

https://bugs.dpdk.org/show_bug.cgi?id=383

Bug ID: 383
Summary: dpdk virtio_user lack of notifications make
vhost_net+napi stops tx buffers
Product: DPDK
Version: unspecified
Hardware: All
OS: Linux
Status: UNCONFIRMED
Severity: normal
Priority: Normal
Component: vhost/virtio
Assignee: [email protected]
Reporter: [email protected]
Target Milestone: ---

Using the current testpmd vhost_user as:

./app/testpmd -l 6,7,8 --vdev='net_vhost1,iface=/tmp/vhost-user1' --vdev='net_vhost2,iface=/tmp/vhost-user2' -- -a -i --rxq=1 --txq=1 --txd=1024
--forward-mode=rxonly

And starting qemu using packed=on on the interface:

-netdev vhost-user,chardev=charnet1,id=hostnet1 -device virtio-net-pci,rx_queue_size=256,...,packed=on

And start to tx in the guest using:

./dpdk/build/app/testpmd -l 1,2 --vdev=eth_af_packet0,iface=eth0 -- \
    --forward-mode=txonly --txq=1 --txd=256 --auto-start --txpkts

1500 \
--stats-period 1

After first burst of packets (512 or a little more), sendto() will start to
return EBUSY. kernel NAPI is refusing to send more packets to virtio_net device
until it free old skbs.

However, virtio_net driver is unable to free old buffers since host does not return them in vhost_flush_dequeue_packed until shadow queue is full
except for MAX_PKT_BURST (32) packets.

Sometimes we are lucky and reach this point, or packets are small enough to
fill the queue and flush, but if the packets and the virtqueue are big enough,
we will not be able to tx anymore.

The current implementation of vhost_net in packed vring tries to fill the shadow vector before send any actual changes to the guest. While this can be beneficial for the throughput, it conflicts with some bufferfloats methods like the linux kernel napi, that stops transmitting packets if there are too much bytes/buffers in the driver.

To solve it, we flush the shadow packets at the end of virtio_dev_tx_packed if we have starved the vring, i.e., the next buffer is not available for the device.

Since this last check can be expensive because of the atomic, we only check it if we have not obtained the expected (count) packets. If it happens to obtain "count" packets and there is no more available packets the caller needs to keep call virtio_dev_tx_packed again.

Signed-off-by Eugenio Pérez <[email protected]>

---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

lib/librte_vhost/virtio_net.c +26 -1

index 21c311732..ac2842b2d 100644
@@ -2133,6 +2133,20 @@ virtio_dev_tx_packed_zmbuf(struct virtio_net *dev,
 	return pkt_idx;
 }
 
+static __rte_always_inline bool
+next_desc_is_avail(const struct vhost_virtqueue *vq)
+{
+	bool wrap_counter = vq->avail_wrap_counter;
+	uint16_t next_used_idx = vq->last_used_idx + 1;
+
+	if (next_used_idx >= vq->size) {
+		next_used_idx -= vq->size;
+		wrap_counter ^= 1;
+	}
+
+	return desc_is_avail(&vq->desc_packed[next_used_idx], wrap_counter);
+}
+
 static __rte_noinline uint16_t
 virtio_dev_tx_packed(struct virtio_net *dev,
 		     struct vhost_virtqueue *vq,

@@ -2165,9 +2179,20 @@ virtio_dev_tx_packed(struct virtio_net *dev,
 
 	} while (remained);
 
-	if (vq->shadow_used_idx)
+	if (vq->shadow_used_idx) {
 		do_data_copy_dequeue(vq);
 
+		if (remained && !next_desc_is_avail(vq)) {
+			/*
+			 * The guest may be waiting to TX some buffers to
+			 * enqueue more to avoid bufferfloat, so we try to
+			 * reduce latency here.
+			 */
+			vhost_flush_dequeue_shadow_packed(dev, vq);
+			vhost_vring_call_packed(dev, vq);
+		}
+	}
+
 	return pkt_idx;
 }
 

Hi Eugenio,

Quoted 7 lines · Eugenio Pérez

The current implementation of vhost_net in packed vring tries to fill the shadow vector before send any actual changes to the guest. While this can be beneficial for the throughput, it conflicts with some bufferfloats methods like the linux kernel napi, that stops transmitting packets if there are too much bytes/buffers in the driver.

Quoted 9 lines

To solve it, we flush the shadow packets at the end of virtio_dev_tx_packed if we have starved the vring, i.e., the next buffer is not available for the device.

Since this last check can be expensive because of the atomic, we only check it if we have not obtained the expected (count) packets. If it happens to obtain "count" packets and there is no more available packets the caller needs to keep call virtio_dev_tx_packed again.

It seems to be fixing an issue and should be considered for stable branches? You can add the tags needed in the commit message here:

Fixes <commit that introduced bug/missed this case>

Cc [email protected]

Review

Quoted 6 lines

Signed-off-by Eugenio Pérez <[email protected]>

---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/lib/librte_vhost/virtio_net.c

Quoted 9 lines

index 21c311732..ac2842b2d 100644

lib/librte_vhost/virtio_net.c +2 -0

@@ -2133,6 +2133,20 @@ virtio_dev_tx_packed_zmbuf(struct virtio_net *dev,
      return pkt_idx;
 }
 
+static __rte_always_inline bool
+next_desc_is_avail(const struct vhost_virtqueue *vq)

lib/librte_vhost/virtio_net.c virtio_dev_tx_packed_zmbuf(struct virtio_net *dev, · line 2147

Quoted 10 lines

lib/librte_vhost/virtio_net.c +10 -0

+{
+     bool wrap_counter = vq->avail_wrap_counter;
+     uint16_t next_used_idx = vq->last_used_idx + 1;
+
+     if (next_used_idx >= vq->size) {
+             next_used_idx -= vq->size;
+             wrap_counter ^= 1;
+     }
+
+     return desc_is_avail(&vq->desc_packed[next_used_idx],

wrap_counter);

Quoted 28 lines

lib/librte_vhost/virtio_net.c +14 -1

+}
+
 static __rte_noinline uint16_t
 virtio_dev_tx_packed(struct virtio_net *dev,
                   struct vhost_virtqueue *vq,

@@ -2165,9 +2179,20 @@ virtio_dev_tx_packed(struct virtio_net *dev,
 
      } while (remained);
 
-     if (vq->shadow_used_idx)
+     if (vq->shadow_used_idx) {
              do_data_copy_dequeue(vq);
 
+             if (remained && !next_desc_is_avail(vq)) {
+                     /*
+                      * The guest may be waiting to TX some buffers to
+                      * enqueue more to avoid bufferfloat, so we try to
+                      * reduce latency here.
+                      */
+                     vhost_flush_dequeue_shadow_packed(dev, vq);
+                     vhost_vring_call_packed(dev, vq);
+             }
+     }
+
      return pkt_idx;
 }
 

Hi Kevin!

Sorry, thanks for noticing it! It fixes commit ("31d6c6a5b vhost: optimize packed ring dequeue"), what was not present on 18.11 version (I've checked that v19.08 does not contain the failure).

Do I need to send another patch version with corrected commit message?

Thanks!

Quoted 83 lines · Kevin Traynor

Hi Eugenio,

Quoted 7 lines · Eugenio Pérez

The current implementation of vhost_net in packed vring tries to fill the shadow vector before send any actual changes to the guest. While this can be beneficial for the throughput, it conflicts with some bufferfloats methods like the linux kernel napi, that stops transmitting packets if there are too much bytes/buffers in the driver.

Quoted 9 lines

To solve it, we flush the shadow packets at the end of virtio_dev_tx_packed if we have starved the vring, i.e., the next buffer is not available for the device.

Since this last check can be expensive because of the atomic, we only check it if we have not obtained the expected (count) packets. If it happens to obtain "count" packets and there is no more available packets the caller needs to keep call virtio_dev_tx_packed again.

It seems to be fixing an issue and should be considered for stable branches? You can add the tags needed in the commit message here:

Fixes <commit that introduced bug/missed this case>

Cc [email protected]

Review

Quoted 6 lines

Signed-off-by Eugenio Pérez <[email protected]>

---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/lib/librte_vhost/virtio_net.c

Quoted 9 lines

index 21c311732..ac2842b2d 100644

lib/librte_vhost/virtio_net.c +2 -0

@@ -2133,6 +2133,20 @@ virtio_dev_tx_packed_zmbuf(struct virtio_net *dev,
      return pkt_idx;
 }
 
+static __rte_always_inline bool
+next_desc_is_avail(const struct vhost_virtqueue *vq)

lib/librte_vhost/virtio_net.c virtio_dev_tx_packed_zmbuf(struct virtio_net *dev, · line 2147

Quoted 10 lines

lib/librte_vhost/virtio_net.c +10 -0

+{
+     bool wrap_counter = vq->avail_wrap_counter;
+     uint16_t next_used_idx = vq->last_used_idx + 1;
+
+     if (next_used_idx >= vq->size) {
+             next_used_idx -= vq->size;
+             wrap_counter ^= 1;
+     }
+
+     return desc_is_avail(&vq->desc_packed[next_used_idx],

wrap_counter);

Quoted 28 lines

lib/librte_vhost/virtio_net.c +14 -1

+}
+
 static __rte_noinline uint16_t
 virtio_dev_tx_packed(struct virtio_net *dev,
                   struct vhost_virtqueue *vq,

@@ -2165,9 +2179,20 @@ virtio_dev_tx_packed(struct virtio_net *dev,
 
      } while (remained);
 
-     if (vq->shadow_used_idx)
+     if (vq->shadow_used_idx) {
              do_data_copy_dequeue(vq);
 
+             if (remained && !next_desc_is_avail(vq)) {
+                     /*
+                      * The guest may be waiting to TX some buffers to
+                      * enqueue more to avoid bufferfloat, so we try to
+                      * reduce latency here.
+                      */
+                     vhost_flush_dequeue_shadow_packed(dev, vq);
+                     vhost_vring_call_packed(dev, vq);
+             }
+     }
+
      return pkt_idx;
 }
 

Quoted 3 lines · Eugenio Perez Martin

Hi Kevin!

Sorry, thanks for noticing it! It fixes commit ("31d6c6a5b vhost:

optimize

Quoted 1 line

packed ring dequeue"), what was not present on 18.11 version (I've

checked

Quoted 2 lines

that v19.08 does not contain the failure).

Right, in that case the issue is present on 19.11 stable, so it's worth adding the tags to get it fixed in 19.11 stable.

Quoted 2 lines

Do I need to send another patch version with corrected commit message?

Probably Maxime can do it on applying if you ask nicely :-)

Quoted 3 lines

Thanks!

On Fri, Jan 31, 2020 at 7:38 PM Kevin Traynor <[email protected]>

Quoted 32 lines · wrote:

Quoted 32 lines

Hi Eugenio,

Quoted 7 lines · Eugenio Pérez

The current implementation of vhost_net in packed vring tries to fill the shadow vector before send any actual changes to the guest. While this can be beneficial for the throughput, it conflicts with some bufferfloats methods like the linux kernel napi, that stops transmitting packets if there are too much bytes/buffers in the driver.

Quoted 9 lines

To solve it, we flush the shadow packets at the end of virtio_dev_tx_packed if we have starved the vring, i.e., the next buffer is not available for the device.

Since this last check can be expensive because of the atomic, we only check it if we have not obtained the expected (count) packets. If it happens to obtain "count" packets and there is no more available packets the caller needs to keep call virtio_dev_tx_packed again.

It seems to be fixing an issue and should be considered for stable branches? You can add the tags needed in the commit message here:

Fixes <commit that introduced bug/missed this case>

Cc [email protected]

Quoted 5 lines

Signed-off-by Eugenio Pérez <[email protected]>

---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Quoted 6 lines

Quoted 6 lines

Review

Quoted 1 line

(quoted diff) +0 -0

b/lib/librte_vhost/virtio_net.c

lib/librte_vhost/virtio_net.c virtio_dev_tx_packed_zmbuf(struct virtio_net

Quoted 4 lines

index 21c311732..ac2842b2d 100644

lib/librte_vhost/virtio_net.c +0 -0

@@ -2133,6 +2133,20 @@ virtio_dev_tx_packed_zmbuf(struct virtio_net

*dev,

Quoted 25 lines

Quoted 25 lines

Quoted 5 lines

lib/librte_vhost/virtio_net.c +2 -0

      return pkt_idx;
 }
 
+static __rte_always_inline bool
+next_desc_is_avail(const struct vhost_virtqueue *vq)

lib/librte_vhost/virtio_net.c virtio_dev_tx_packed_zmbuf(struct virtio_net *dev,

Quoted 10 lines

lib/librte_vhost/virtio_net.c +10 -0

+{
+     bool wrap_counter = vq->avail_wrap_counter;
+     uint16_t next_used_idx = vq->last_used_idx + 1;
+
+     if (next_used_idx >= vq->size) {
+             next_used_idx -= vq->size;
+             wrap_counter ^= 1;
+     }
+
+     return desc_is_avail(&vq->desc_packed[next_used_idx],

wrap_counter);

Quoted 9 lines

lib/librte_vhost/virtio_net.c +2 -0

+}
+
 static __rte_noinline uint16_t
 virtio_dev_tx_packed(struct virtio_net *dev,
                   struct vhost_virtqueue *vq,

@@ -2165,9 +2179,20 @@ virtio_dev_tx_packed(struct virtio_net *dev,
 
      } while (remained);
 

Quoted 8 lines

Quoted 8 lines

lib/librte_vhost/virtio_net.c virtio_dev_tx_packed(struct virtio_net *dev,

Quoted 8 lines

lib/librte_vhost/virtio_net.c +5 -1

-     if (vq->shadow_used_idx)
+     if (vq->shadow_used_idx) {
              do_data_copy_dequeue(vq);
 
+             if (remained && !next_desc_is_avail(vq)) {
+                     /*
+                      * The guest may be waiting to TX some buffers to
+                      * enqueue more to avoid bufferfloat, so we try

to

Quoted 14 lines

Quoted 13 lines

Quoted 11 lines

lib/librte_vhost/virtio_net.c +7 -0

+                      * reduce latency here.
+                      */
+                     vhost_flush_dequeue_shadow_packed(dev, vq);
+                     vhost_vring_call_packed(dev, vq);
+             }
+     }
+
      return pkt_idx;
 }
 

Ouch, my bad again, sorry :).

I've forwarded the patch to stable@, please let me know if I need to do something else.

Maxime, please let me know if I need to send a new version with the "Fixed:
" tag :).

Thanks!

Quoted 111 lines · Kevin Traynor

Quoted 3 lines · Eugenio Perez Martin

Hi Kevin!

Sorry, thanks for noticing it! It fixes commit ("31d6c6a5b vhost:

optimize

Quoted 1 line

packed ring dequeue"), what was not present on 18.11 version (I've

checked

Quoted 2 lines

that v19.08 does not contain the failure).

Right, in that case the issue is present on 19.11 stable, so it's worth adding the tags to get it fixed in 19.11 stable.

Quoted 2 lines

Do I need to send another patch version with corrected commit message?

Probably Maxime can do it on applying if you ask nicely :-)

Quoted 3 lines

Thanks!

On Fri, Jan 31, 2020 at 7:38 PM Kevin Traynor <[email protected]>

Quoted 32 lines · wrote:

Quoted 32 lines

Hi Eugenio,

Quoted 7 lines · Eugenio Pérez

The current implementation of vhost_net in packed vring tries to fill the shadow vector before send any actual changes to the guest. While this can be beneficial for the throughput, it conflicts with some bufferfloats methods like the linux kernel napi, that stops transmitting packets if there are too much bytes/buffers in the driver.

Quoted 9 lines

To solve it, we flush the shadow packets at the end of virtio_dev_tx_packed if we have starved the vring, i.e., the next buffer is not available for the device.

Since this last check can be expensive because of the atomic, we only check it if we have not obtained the expected (count) packets. If it happens to obtain "count" packets and there is no more available packets the caller needs to keep call virtio_dev_tx_packed again.

It seems to be fixing an issue and should be considered for stable branches? You can add the tags needed in the commit message here:

Fixes <commit that introduced bug/missed this case>

Cc [email protected]

Quoted 5 lines

Signed-off-by Eugenio Pérez <[email protected]>

---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Quoted 6 lines

Quoted 6 lines

Review

Quoted 1 line

(quoted diff) +0 -0

b/lib/librte_vhost/virtio_net.c

lib/librte_vhost/virtio_net.c virtio_dev_tx_packed_zmbuf(struct virtio_net

Quoted 4 lines

index 21c311732..ac2842b2d 100644

lib/librte_vhost/virtio_net.c +0 -0

@@ -2133,6 +2133,20 @@ virtio_dev_tx_packed_zmbuf(struct virtio_net

*dev,

Quoted 25 lines

Quoted 25 lines

Quoted 5 lines

lib/librte_vhost/virtio_net.c +2 -0

      return pkt_idx;
 }
 
+static __rte_always_inline bool
+next_desc_is_avail(const struct vhost_virtqueue *vq)

lib/librte_vhost/virtio_net.c virtio_dev_tx_packed_zmbuf(struct virtio_net *dev,

Quoted 10 lines

lib/librte_vhost/virtio_net.c +10 -0

+{
+     bool wrap_counter = vq->avail_wrap_counter;
+     uint16_t next_used_idx = vq->last_used_idx + 1;
+
+     if (next_used_idx >= vq->size) {
+             next_used_idx -= vq->size;
+             wrap_counter ^= 1;
+     }
+
+     return desc_is_avail(&vq->desc_packed[next_used_idx],

wrap_counter);

Quoted 9 lines

lib/librte_vhost/virtio_net.c +2 -0

+}
+
 static __rte_noinline uint16_t
 virtio_dev_tx_packed(struct virtio_net *dev,
                   struct vhost_virtqueue *vq,

@@ -2165,9 +2179,20 @@ virtio_dev_tx_packed(struct virtio_net *dev,
 
      } while (remained);
 

Quoted 8 lines

Quoted 8 lines

lib/librte_vhost/virtio_net.c virtio_dev_tx_packed(struct virtio_net *dev,

Quoted 8 lines

lib/librte_vhost/virtio_net.c +5 -1

-     if (vq->shadow_used_idx)
+     if (vq->shadow_used_idx) {
              do_data_copy_dequeue(vq);
 
+             if (remained && !next_desc_is_avail(vq)) {
+                     /*
+                      * The guest may be waiting to TX some buffers to
+                      * enqueue more to avoid bufferfloat, so we try

to

Quoted 14 lines

Quoted 13 lines

Quoted 11 lines

lib/librte_vhost/virtio_net.c +7 -0

+                      * reduce latency here.
+                      */
+                     vhost_flush_dequeue_shadow_packed(dev, vq);
+                     vhost_vring_call_packed(dev, vq);
+             }
+     }
+
      return pkt_idx;
 }
 

The current implementation of vhost_net in packed vring tries to fill the shadow vector before send any actual changes to the guest. While this can be beneficial for the throughput, it conflicts with some bufferfloats methods like the linux kernel napi, that stops transmitting packets if there are too much bytes/buffers in the driver.

To solve it, we flush the shadow packets at the end of virtio_dev_tx_packed if we have starved the vring, i.e., the next buffer is not available for the device.

Since this last check can be expensive because of the atomic, we only check it if we have not obtained the expected (count) packets. If it happens to obtain "count" packets and there is no more available packets the caller needs to keep call virtio_dev_tx_packed again.

Signed-off-by Eugenio Pérez <[email protected]>

---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

lib/librte_vhost/virtio_net.c +19 -1

index 21c311732..ac2842b2d 100644
@@ -2133,6 +2133,20 @@ virtio_dev_tx_packed_zmbuf(struct virtio_net *dev,
 	return pkt_idx;
 }
 
+static __rte_always_inline bool
+next_desc_is_avail(const struct vhost_virtqueue *vq)
+{
+	bool wrap_counter = vq->avail_wrap_counter;
+	uint16_t next_used_idx = vq->last_used_idx + 1;
+
+	if (next_used_idx >= vq->size) {
+		next_used_idx -= vq->size;
+		wrap_counter ^= 1;
+	}
+
+	return desc_is_avail(&vq->desc_packed[next_used_idx], wrap_counter);
+}
+
 static __rte_noinline uint16_t
 virtio_dev_tx_packed(struct virtio_net *dev,
 		     struct vhost_virtqueue *vq,

@@ -2165,9 +2179,20 @@ virtio_dev_tx_packed(struct virtio_net *dev,
 
 	} while (remained);
 
-	if (vq->shadow_used_idx)
+	if (vq->shadow_used_idx) {
 		do_data_copy_dequeue(vq);
 
+		if (remained && !next_desc_is_avail(vq)) {
+			/*
+			 * The guest may be waiting to TX some buffers to
+			 * enqueue more to avoid bufferfloat, so we try to

The current implementation of vhost_net in packed vring tries to fill the shadow vector before send any actual changes to the guest. While this can be beneficial for the throughput, it conflicts with some bufferfloats methods like the linux kernel napi, that stops transmitting packets if there are too much bytes/buffers in the driver.

To solve it, we flush the shadow packets at the end of virtio_dev_tx_packed if we have starved the vring, i.e., the next buffer is not available for the device.

Since this last check can be expensive because of the atomic, we only check it if we have not obtained the expected (count) packets. If it happens to obtain "count" packets and there is no more available packets the caller needs to keep call virtio_dev_tx_packed again.

Signed-off-by Eugenio Pérez <[email protected]>

---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Quoted 16 lines · Eugenio Pérez

The current implementation of vhost_net in packed vring tries to fill the shadow vector before send any actual changes to the guest. While this can be beneficial for the throughput, it conflicts with some bufferfloats methods like the linux kernel napi, that stops transmitting packets if there are too much bytes/buffers in the driver.

To solve it, we flush the shadow packets at the end of virtio_dev_tx_packed if we have starved the vring, i.e., the next buffer is not available for the device.

Since this last check can be expensive because of the atomic, we only check it if we have not obtained the expected (count) packets. If it happens to obtain "count" packets and there is no more available packets the caller needs to keep call virtio_dev_tx_packed again.

Quoted 4 lines

Signed-off-by Eugenio Pérez <[email protected]>

---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Applied to dpdk-next-virtio tree.

Thanks,
Maxime