| Message ID | 20240613022147.5886-1-east.moutain.yang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update event idx if guest has made extra buffers during double check | expand |
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 >
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
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
>
>
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.
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; } }
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(+)