[PATCH] Update event idx if guest has made extra buffers during double check

[PATCH] Update event idx if guest has made extra buffers during double check

[thomas]

Fixes: 06b12970174 ("virtio-net: fix network stall under load")

If guest has made some buffers available during double check,
but the total buffer size available is lower than @bufsize,
notify the guest with the latest available idx(event idx)
seen by the host.
---
 hw/net/virtio-net.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9c7e85caea..23c6c8c898 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1654,6 +1654,7 @@ static int virtio_net_has_buffers(VirtIONetQueue *q, int bufsize)
         if (virtio_queue_empty(q->rx_vq) ||
             (n->mergeable_rx_bufs &&
              !virtqueue_avail_bytes(q->rx_vq, bufsize, 0))) {
+            virtio_queue_set_notification(q->rx_vq, 1);
             return 0;
         }
     }
-- 
2.39.0

Re: [PATCH] Update event idx if guest has made extra buffers during double check

[Jason Wang]

On Thu, Jun 13, 2024 at 10:22 AM thomas <east.moutain.yang@gmail.com> wrote:
>
> Fixes: 06b12970174 ("virtio-net: fix network stall under load")
>
> If guest has made some buffers available during double check,
> but the total buffer size available is lower than @bufsize,
> notify the guest with the latest available idx(event idx)
> seen by the host.
> ---
>  hw/net/virtio-net.c | 1 +
>  1 file changed, 1 insertion(+)

Patch looks good to me, but it misses some other stuff for example:

- the sob tag.
- fixes should be placed above sob tag
- need to cc qemu-stable@nongnu.org

Thanks

>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9c7e85caea..23c6c8c898 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1654,6 +1654,7 @@ static int virtio_net_has_buffers(VirtIONetQueue *q, int bufsize)
>          if (virtio_queue_empty(q->rx_vq) ||
>              (n->mergeable_rx_bufs &&
>               !virtqueue_avail_bytes(q->rx_vq, bufsize, 0))) {
> +            virtio_queue_set_notification(q->rx_vq, 1);
>              return 0;
>          }
>      }
> --
> 2.39.0
>

Re: [PATCH] Update event idx if guest has made extra buffers during double check

[Michael S. Tsirkin]

Thanks for the patch!
Yet something to improve:



subject should list the affected component, and be shorter.

On Thu, Jun 13, 2024 at 10:21:47AM +0800, thomas wrote:
> Fixes: 06b12970174 ("virtio-net: fix network stall under load")

this should come at the end. and what exactly does this
refer to? did this commit cause a regression of some sort?

> If guest has made some buffers available during double check,

what does "double check" refer to?

> but the total buffer size available is lower than @bufsize,
> notify the guest with the latest available idx(event idx)
> seen by the host.

which makes sense why?  And which changes the correct behavious of what
to a new behaviour of what which is better why?

Pls review docs/devel/submitting-a-patch.rst and follow the
process there.



> ---
>  hw/net/virtio-net.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9c7e85caea..23c6c8c898 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1654,6 +1654,7 @@ static int virtio_net_has_buffers(VirtIONetQueue *q, int bufsize)
>          if (virtio_queue_empty(q->rx_vq) ||
>              (n->mergeable_rx_bufs &&
>               !virtqueue_avail_bytes(q->rx_vq, bufsize, 0))) {
> +            virtio_queue_set_notification(q->rx_vq, 1);
>              return 0;
>          }
>      }
> -- 
> 2.39.0

Re: [PATCH] Update event idx if guest has made extra buffers during double check

[Yang Dongshan]

[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]

hi,

subject should list the affected component, and be shorter.

ok, I will rewrite the subject:
"update the latest available idx seen by the host to event idx"

Fixes: 06b12970174 ("virtio-net: fix network stall under load")

this should come at the end.

I have submitted v2, it's at the end now.

and what exactly does this refer to?
>
Commit 06b12970174 double-checks whether the guest has made some
buffers available after the first check,  it will be lucky if the available
buffer
size can satisfy the request.

did this commit cause a regression of some sort?
>
No regression. If the buffer size still can't satisfy the request even if
the
guest has made some buffers.  this commit doesn't notify the latest
shadow_avail_idx seen by the host to the guest. Similar to the first
check, if the available buffer is not enough, notify the guest with the
updated shadow_avail_idx.

what does "double check" refer to?
>
it refers to the second nested if condition judgment in
virtio_net_has_buffers().

which makes sense why?  And which changes the correct behavious of what
> to a new behaviour of what which is better why?
>
Similar to the first check, if the buffer size still can't satisfy the
request, notify the
guest with the updated shadow_avail_idx, it's better than the old one.

On Mon, Jun 17, 2024 at 6:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:

>
> Thanks for the patch!
> Yet something to improve:
>
>
>
> subject should list the affected component, and be shorter.
>
> On Thu, Jun 13, 2024 at 10:21:47AM +0800, thomas wrote:
> > Fixes: 06b12970174 ("virtio-net: fix network stall under load")
>
> this should come at the end. and what exactly does this
> refer to? did this commit cause a regression of some sort?
>
> > If guest has made some buffers available during double check,
>
> what does "double check" refer to?
>
> > but the total buffer size available is lower than @bufsize,
> > notify the guest with the latest available idx(event idx)
> > seen by the host.
>
> which makes sense why?  And which changes the correct behavious of what
> to a new behaviour of what which is better why?
>
> Pls review docs/devel/submitting-a-patch.rst and follow the
> process there.
>
>
>
> > ---
> >  hw/net/virtio-net.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 9c7e85caea..23c6c8c898 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1654,6 +1654,7 @@ static int virtio_net_has_buffers(VirtIONetQueue
> *q, int bufsize)
> >          if (virtio_queue_empty(q->rx_vq) ||
> >              (n->mergeable_rx_bufs &&
> >               !virtqueue_avail_bytes(q->rx_vq, bufsize, 0))) {
> > +            virtio_queue_set_notification(q->rx_vq, 1);
> >              return 0;
> >          }
> >      }
> > --
> > 2.39.0
>
>

[-- Attachment #2: Type: text/html, Size: 4699 bytes --]

Re: [PATCH] Update event idx if guest has made extra buffers during double check

[Michael S. Tsirkin]

On Mon, Jun 17, 2024 at 09:51:33PM +0800, Yang Dongshan wrote:
> Similar to the first check, if the buffer size still can't satisfy the request,
> notify the
> guest with the updated shadow_avail_idx, it's better than the old one.

So it's an optimization then? Same as any optimization, it should
come with measurements showing the performance benefit.

-- 
MST