[dpdk-dev] [Bug 383] dpdk virtio_user lack of notifications make vhost_net+napi stops tx buffers

[dpdk-dev] [Bug 383] dpdk virtio_user lack of notifications make vhost_net+napi stops tx buffers

[bugzilla]

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: dev@dpdk.org
          Reporter: eupm90@gmail.com
  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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: [dpdk-dev] [Bug 383] dpdk virtio_user lack of notifications make vhost_net+napi stops tx buffers

[eperezma]

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.

diff --git a/lib/librte_vhost/virtio_net.c
b/lib/librte_vhost/virtio_net.c
index 21c311732..f7137149c 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -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)
@@ -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);

On Thu, 2020-01-09 at 15:47 +0000, bugzilla@dpdk.org 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: dev@dpdk.org
>           Reporter: eupm90@gmail.com
>   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.
> 

Re: [dpdk-dev] [Bug 383] dpdk virtio_user lack of notifications make vhost_net+napi stops tx buffers

[Liu, Yong]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of eperezma@redhat.com
> Sent: Thursday, January 09, 2020 11:56 PM
> To: bugzilla@dpdk.org; dev@dpdk.org; Maxime Coquelin <mcoqueli@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>; Michael S. Tsirkin <mst@redhat.com>;
> Adrian Moreno Zapata <amorenoz@redhat.com>
> 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.
> 

Hi Eugenio,

Shadow method is aimed to maximum the throughput. In our experimental, there's no 
clear performance gain when shadowed size over half of the ring size (eg.256).

Unconditional do shadow flush will harm the performance a lot with virtio-user frontend.
Checking next descriptors will has less impact in performance, I prefer this solution.

Thanks,
Marvin


> 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.
> 
> diff --git a/lib/librte_vhost/virtio_net.c
> b/lib/librte_vhost/virtio_net.c
> index 21c311732..f7137149c 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -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)
> @@ -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);
> 
> On Thu, 2020-01-09 at 15:47 +0000, bugzilla@dpdk.org 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: dev@dpdk.org
> >           Reporter: eupm90@gmail.com
> >   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.
> >

[dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

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

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 21c311732..ac2842b2d 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -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;
 }
 
-- 
2.18.1

Re: [dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

[Kevin Traynor]

Hi Eugenio,

On 29/01/2020 19:33, Eugenio Pérez wrote:
> 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.
> 

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: stable@dpdk.org

> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 21c311732..ac2842b2d 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -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;
>  }
>  
> 

Re: [dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

[Eugenio Perez Martin]

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!

On Fri, Jan 31, 2020 at 7:38 PM Kevin Traynor <ktraynor@redhat.com> wrote:

> Hi Eugenio,
>
> On 29/01/2020 19:33, Eugenio Pérez wrote:
> > 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.
> >
>
> 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: stable@dpdk.org
>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_vhost/virtio_net.c
> b/lib/librte_vhost/virtio_net.c
> > index 21c311732..ac2842b2d 100644
> > --- a/lib/librte_vhost/virtio_net.c
> > +++ b/lib/librte_vhost/virtio_net.c
> > @@ -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;
> >  }
> >
> >
>
>

Re: [dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

[Kevin Traynor]

On 04/02/2020 09:23, Eugenio Perez Martin wrote:
> 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).
> 

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.

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

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

> Thanks!
> 
> On Fri, Jan 31, 2020 at 7:38 PM Kevin Traynor <ktraynor@redhat.com> wrote:
> 
>> Hi Eugenio,
>>
>> On 29/01/2020 19:33, Eugenio Pérez wrote:
>>> 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.
>>>
>>
>> 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: stable@dpdk.org
>>
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> ---
>>>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/virtio_net.c
>> b/lib/librte_vhost/virtio_net.c
>>> index 21c311732..ac2842b2d 100644
>>> --- a/lib/librte_vhost/virtio_net.c
>>> +++ b/lib/librte_vhost/virtio_net.c
>>> @@ -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;
>>>  }
>>>
>>>
>>
>>
> 

Re: [dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

[Eugenio Perez Martin]

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!

On Tue, Feb 4, 2020 at 2:49 PM Kevin Traynor <ktraynor@redhat.com> wrote:

> On 04/02/2020 09:23, Eugenio Perez Martin wrote:
> > 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).
> >
>
> 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.
>
> > Do I need to send another patch version with corrected commit message?
> >
>
> Probably Maxime can do it on applying if you ask nicely :-)
>
> > Thanks!
> >
> > On Fri, Jan 31, 2020 at 7:38 PM Kevin Traynor <ktraynor@redhat.com>
> wrote:
> >
> >> Hi Eugenio,
> >>
> >> On 29/01/2020 19:33, Eugenio Pérez wrote:
> >>> 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.
> >>>
> >>
> >> 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: stable@dpdk.org
> >>
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>> ---
> >>>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
> >>>  1 file changed, 26 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/librte_vhost/virtio_net.c
> >> b/lib/librte_vhost/virtio_net.c
> >>> index 21c311732..ac2842b2d 100644
> >>> --- a/lib/librte_vhost/virtio_net.c
> >>> +++ b/lib/librte_vhost/virtio_net.c
> >>> @@ -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;
> >>>  }
> >>>
> >>>
> >>
> >>
> >
>
>

Re: [dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

[Maxime Coquelin]

On 2/4/20 4:05 PM, Eugenio Perez Martin wrote:
> 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 :).

Not needed, I will handle it.

Thanks for the fix,
Maxime

> Thanks!
> 
> On Tue, Feb 4, 2020 at 2:49 PM Kevin Traynor <ktraynor@redhat.com> wrote:
> 
>> On 04/02/2020 09:23, Eugenio Perez Martin wrote:
>>> 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).
>>>
>>
>> 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.
>>
>>> Do I need to send another patch version with corrected commit message?
>>>
>>
>> Probably Maxime can do it on applying if you ask nicely :-)
>>
>>> Thanks!
>>>
>>> On Fri, Jan 31, 2020 at 7:38 PM Kevin Traynor <ktraynor@redhat.com>
>> wrote:
>>>
>>>> Hi Eugenio,
>>>>
>>>> On 29/01/2020 19:33, Eugenio Pérez wrote:
>>>>> 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.
>>>>>
>>>>
>>>> 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: stable@dpdk.org
>>>>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> ---
>>>>>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
>>>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/librte_vhost/virtio_net.c
>>>> b/lib/librte_vhost/virtio_net.c
>>>>> index 21c311732..ac2842b2d 100644
>>>>> --- a/lib/librte_vhost/virtio_net.c
>>>>> +++ b/lib/librte_vhost/virtio_net.c
>>>>> @@ -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;
>>>>>  }
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
> 

Re: [dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

[Maxime Coquelin]

On 2/4/20 3:47 PM, Eugenio Pérez wrote:
> 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 <eperezma@redhat.com>
> ---
>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 21c311732..ac2842b2d 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -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;
>  }
>  
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@¶edhat.com>

I'll fix the commit message while applying.

Thanks,
Maxime

Re: [dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

[Maxime Coquelin]

On 2/4/20 3:47 PM, Eugenio Pérez wrote:
> 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 <eperezma@redhat.com>
> ---
>  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)

Applied to dpdk-next-virtio tree.

Thanks,
Maxime

Re: [dpdk-dev] [PATCH] vhost: flush shadow tx if there is no more packets

[Eugenio Perez Martin]

Thanks!

On Wed, Feb 5, 2020 at 10:48 AM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

>
>
> On 2/4/20 3:47 PM, Eugenio Pérez wrote:
> > 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 <eperezma@redhat.com>
> > ---
> >  lib/librte_vhost/virtio_net.c | 27 ++++++++++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
>
> Applied to dpdk-next-virtio tree.
>
> Thanks,
> Maxime
>
>