* [PATCH] virtio-balloon: correct used length @ 2021-11-24 4:32 Jason Wang 2021-11-24 7:03 ` Michael S. Tsirkin 0 siblings, 1 reply; 7+ messages in thread From: Jason Wang @ 2021-11-24 4:32 UTC (permalink / raw) To: mst, david; +Cc: mpe, Jason Wang, qemu-devel Spec said: "and len the total of bytes written into the buffer." For inflateq, deflateq and statsq, we don't process in_sg so the used length should be zero. For free_page_vq, though the free pages are supplied via in_sgs, zero used length should still be fine since anyway driver is expected to use the length in this case and it simplifies the error handling path. Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/virtio/virtio-balloon.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index c6962fcbfe..3e52daa793 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque) return; } - virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); + virtqueue_push(s->svq, s->stats_vq_elem, 0); virtio_notify(vdev, s->svq); g_free(s->stats_vq_elem); s->stats_vq_elem = NULL; @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) memory_region_unref(section.mr); } - virtqueue_push(vq, elem, offset); + virtqueue_push(vq, elem, 0); virtio_notify(vdev, vq); g_free(elem); virtio_balloon_pbp_free(&pbp); @@ -549,7 +549,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev) } out: - virtqueue_push(vq, elem, 1); + virtqueue_push(vq, elem, 0); g_free(elem); return ret; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-balloon: correct used length 2021-11-24 4:32 [PATCH] virtio-balloon: correct used length Jason Wang @ 2021-11-24 7:03 ` Michael S. Tsirkin 2021-11-24 7:58 ` Jason Wang 0 siblings, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2021-11-24 7:03 UTC (permalink / raw) To: Jason Wang; +Cc: mpe, qemu-devel, david On Wed, Nov 24, 2021 at 12:32:55PM +0800, Jason Wang wrote: > Spec said: > > "and len the total of bytes written into the buffer." > > For inflateq, deflateq and statsq, we don't process in_sg so the used > length should be zero. For free_page_vq, though the free pages are > supplied via in_sgs, zero used length should still be fine since > anyway driver is expected to use the length in this case and it > simplifies the error handling path. > > Signed-off-by: Jason Wang <jasowang@redhat.com> I think for free page vq the point is that the pages are zeroed by hypervisor, so we must set used len accordingly. No? > --- > hw/virtio/virtio-balloon.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index c6962fcbfe..3e52daa793 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque) > return; > } > > - virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); > + virtqueue_push(s->svq, s->stats_vq_elem, 0); > virtio_notify(vdev, s->svq); > g_free(s->stats_vq_elem); > s->stats_vq_elem = NULL; > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > memory_region_unref(section.mr); > } > > - virtqueue_push(vq, elem, offset); > + virtqueue_push(vq, elem, 0); > virtio_notify(vdev, vq); > g_free(elem); > virtio_balloon_pbp_free(&pbp); > @@ -549,7 +549,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev) > } > > out: > - virtqueue_push(vq, elem, 1); > + virtqueue_push(vq, elem, 0); > g_free(elem); > return ret; > } > -- > 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-balloon: correct used length 2021-11-24 7:03 ` Michael S. Tsirkin @ 2021-11-24 7:58 ` Jason Wang 2021-11-24 8:25 ` Michael S. Tsirkin 0 siblings, 1 reply; 7+ messages in thread From: Jason Wang @ 2021-11-24 7:58 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Michael Ellerman, qemu-devel, David Hildenbrand On Wed, Nov 24, 2021 at 3:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Nov 24, 2021 at 12:32:55PM +0800, Jason Wang wrote: > > Spec said: > > > > "and len the total of bytes written into the buffer." > > > > For inflateq, deflateq and statsq, we don't process in_sg so the used > > length should be zero. For free_page_vq, though the free pages are > > supplied via in_sgs, zero used length should still be fine since > > anyway driver is expected to use the length in this case and it > > simplifies the error handling path. > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > I think for free page vq the point is that the pages are > zeroed by hypervisor, so we must set used len accordingly. No? I may miss something, I think it is used for clearing the dirty bitmaps to avoid migrating unused pages, the zeroing is not needed. Thanks > > > --- > > hw/virtio/virtio-balloon.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index c6962fcbfe..3e52daa793 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque) > > return; > > } > > > > - virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); > > + virtqueue_push(s->svq, s->stats_vq_elem, 0); > > virtio_notify(vdev, s->svq); > > g_free(s->stats_vq_elem); > > s->stats_vq_elem = NULL; > > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > memory_region_unref(section.mr); > > } > > > > - virtqueue_push(vq, elem, offset); > > + virtqueue_push(vq, elem, 0); > > virtio_notify(vdev, vq); > > g_free(elem); > > virtio_balloon_pbp_free(&pbp); > > @@ -549,7 +549,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev) > > } > > > > out: > > - virtqueue_push(vq, elem, 1); > > + virtqueue_push(vq, elem, 0); > > g_free(elem); > > return ret; > > } > > -- > > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-balloon: correct used length 2021-11-24 7:58 ` Jason Wang @ 2021-11-24 8:25 ` Michael S. Tsirkin 2021-11-24 8:28 ` Jason Wang 0 siblings, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2021-11-24 8:25 UTC (permalink / raw) To: Jason Wang; +Cc: Michael Ellerman, qemu-devel, David Hildenbrand On Wed, Nov 24, 2021 at 03:58:22PM +0800, Jason Wang wrote: > On Wed, Nov 24, 2021 at 3:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, Nov 24, 2021 at 12:32:55PM +0800, Jason Wang wrote: > > > Spec said: > > > > > > "and len the total of bytes written into the buffer." > > > > > > For inflateq, deflateq and statsq, we don't process in_sg so the used > > > length should be zero. For free_page_vq, though the free pages are > > > supplied via in_sgs, zero used length should still be fine since > > > anyway driver is expected to use the length in this case and it > > > simplifies the error handling path. > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > I think for free page vq the point is that the pages are > > zeroed by hypervisor, so we must set used len accordingly. No? > > I may miss something, I think it is used for clearing the dirty > bitmaps to avoid migrating unused pages, the zeroing is not needed. > > Thanks And, once the page is migrated, it's value on destination may differ from one on source. > > > > > --- > > > hw/virtio/virtio-balloon.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > index c6962fcbfe..3e52daa793 100644 > > > --- a/hw/virtio/virtio-balloon.c > > > +++ b/hw/virtio/virtio-balloon.c > > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque) > > > return; > > > } > > > > > > - virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); > > > + virtqueue_push(s->svq, s->stats_vq_elem, 0); > > > virtio_notify(vdev, s->svq); > > > g_free(s->stats_vq_elem); > > > s->stats_vq_elem = NULL; > > > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > memory_region_unref(section.mr); > > > } > > > > > > - virtqueue_push(vq, elem, offset); > > > + virtqueue_push(vq, elem, 0); > > > virtio_notify(vdev, vq); > > > g_free(elem); > > > virtio_balloon_pbp_free(&pbp); > > > @@ -549,7 +549,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev) > > > } > > > > > > out: > > > - virtqueue_push(vq, elem, 1); > > > + virtqueue_push(vq, elem, 0); > > > g_free(elem); > > > return ret; > > > } > > > -- > > > 2.25.1 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-balloon: correct used length 2021-11-24 8:25 ` Michael S. Tsirkin @ 2021-11-24 8:28 ` Jason Wang 2021-11-24 8:32 ` Jason Wang 0 siblings, 1 reply; 7+ messages in thread From: Jason Wang @ 2021-11-24 8:28 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Michael Ellerman, qemu-devel, David Hildenbrand On Wed, Nov 24, 2021 at 4:25 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Nov 24, 2021 at 03:58:22PM +0800, Jason Wang wrote: > > On Wed, Nov 24, 2021 at 3:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Wed, Nov 24, 2021 at 12:32:55PM +0800, Jason Wang wrote: > > > > Spec said: > > > > > > > > "and len the total of bytes written into the buffer." > > > > > > > > For inflateq, deflateq and statsq, we don't process in_sg so the used > > > > length should be zero. For free_page_vq, though the free pages are > > > > supplied via in_sgs, zero used length should still be fine since > > > > anyway driver is expected to use the length in this case and it > > > > simplifies the error handling path. > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > I think for free page vq the point is that the pages are > > > zeroed by hypervisor, so we must set used len accordingly. No? > > > > I may miss something, I think it is used for clearing the dirty > > bitmaps to avoid migrating unused pages, the zeroing is not needed. > > > > Thanks > > And, once the page is migrated, it's value on destination may differ > from one on source. Right, I will fix this in the next version. Thanks > > > > > > > > --- > > > > hw/virtio/virtio-balloon.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > index c6962fcbfe..3e52daa793 100644 > > > > --- a/hw/virtio/virtio-balloon.c > > > > +++ b/hw/virtio/virtio-balloon.c > > > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque) > > > > return; > > > > } > > > > > > > > - virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); > > > > + virtqueue_push(s->svq, s->stats_vq_elem, 0); > > > > virtio_notify(vdev, s->svq); > > > > g_free(s->stats_vq_elem); > > > > s->stats_vq_elem = NULL; > > > > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > memory_region_unref(section.mr); > > > > } > > > > > > > > - virtqueue_push(vq, elem, offset); > > > > + virtqueue_push(vq, elem, 0); > > > > virtio_notify(vdev, vq); > > > > g_free(elem); > > > > virtio_balloon_pbp_free(&pbp); > > > > @@ -549,7 +549,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev) > > > > } > > > > > > > > out: > > > > - virtqueue_push(vq, elem, 1); > > > > + virtqueue_push(vq, elem, 0); > > > > g_free(elem); > > > > return ret; > > > > } > > > > -- > > > > 2.25.1 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-balloon: correct used length 2021-11-24 8:28 ` Jason Wang @ 2021-11-24 8:32 ` Jason Wang 2021-11-24 9:17 ` Michael S. Tsirkin 0 siblings, 1 reply; 7+ messages in thread From: Jason Wang @ 2021-11-24 8:32 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Michael Ellerman, qemu-devel, David Hildenbrand On Wed, Nov 24, 2021 at 4:28 PM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Nov 24, 2021 at 4:25 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, Nov 24, 2021 at 03:58:22PM +0800, Jason Wang wrote: > > > On Wed, Nov 24, 2021 at 3:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Wed, Nov 24, 2021 at 12:32:55PM +0800, Jason Wang wrote: > > > > > Spec said: > > > > > > > > > > "and len the total of bytes written into the buffer." > > > > > > > > > > For inflateq, deflateq and statsq, we don't process in_sg so the used > > > > > length should be zero. For free_page_vq, though the free pages are > > > > > supplied via in_sgs, zero used length should still be fine since > > > > > anyway driver is expected to use the length in this case and it > > > > > simplifies the error handling path. > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > I think for free page vq the point is that the pages are > > > > zeroed by hypervisor, so we must set used len accordingly. No? > > > > > > I may miss something, I think it is used for clearing the dirty > > > bitmaps to avoid migrating unused pages, the zeroing is not needed. > > > > > > Thanks > > > > And, once the page is migrated, it's value on destination may differ > > from one on source. > > Right, I will fix this in the next version. > > Thanks Btw in the get_free_page_hints() I see this: if (elem->in_num && dev->free_page_hint_status == FREE_PAGE_HINT_S_START) { qemu_guest_free_page_hint(elem->in_sg[0].iov_base, elem->in_sg[0].iov_len); } It looks like only the first in sg is used, it looks like a bug? Thanks > > > > > > > > > > > > > --- > > > > > hw/virtio/virtio-balloon.c | 6 +++--- > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > > index c6962fcbfe..3e52daa793 100644 > > > > > --- a/hw/virtio/virtio-balloon.c > > > > > +++ b/hw/virtio/virtio-balloon.c > > > > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque) > > > > > return; > > > > > } > > > > > > > > > > - virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); > > > > > + virtqueue_push(s->svq, s->stats_vq_elem, 0); > > > > > virtio_notify(vdev, s->svq); > > > > > g_free(s->stats_vq_elem); > > > > > s->stats_vq_elem = NULL; > > > > > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > > memory_region_unref(section.mr); > > > > > } > > > > > > > > > > - virtqueue_push(vq, elem, offset); > > > > > + virtqueue_push(vq, elem, 0); > > > > > virtio_notify(vdev, vq); > > > > > g_free(elem); > > > > > virtio_balloon_pbp_free(&pbp); > > > > > @@ -549,7 +549,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev) > > > > > } > > > > > > > > > > out: > > > > > - virtqueue_push(vq, elem, 1); > > > > > + virtqueue_push(vq, elem, 0); > > > > > g_free(elem); > > > > > return ret; > > > > > } > > > > > -- > > > > > 2.25.1 > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-balloon: correct used length 2021-11-24 8:32 ` Jason Wang @ 2021-11-24 9:17 ` Michael S. Tsirkin 0 siblings, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2021-11-24 9:17 UTC (permalink / raw) To: Jason Wang; +Cc: Michael Ellerman, qemu-devel, David Hildenbrand On Wed, Nov 24, 2021 at 04:32:06PM +0800, Jason Wang wrote: > On Wed, Nov 24, 2021 at 4:28 PM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Nov 24, 2021 at 4:25 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Wed, Nov 24, 2021 at 03:58:22PM +0800, Jason Wang wrote: > > > > On Wed, Nov 24, 2021 at 3:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Wed, Nov 24, 2021 at 12:32:55PM +0800, Jason Wang wrote: > > > > > > Spec said: > > > > > > > > > > > > "and len the total of bytes written into the buffer." > > > > > > > > > > > > For inflateq, deflateq and statsq, we don't process in_sg so the used > > > > > > length should be zero. For free_page_vq, though the free pages are > > > > > > supplied via in_sgs, zero used length should still be fine since > > > > > > anyway driver is expected to use the length in this case and it > > > > > > simplifies the error handling path. > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > I think for free page vq the point is that the pages are > > > > > zeroed by hypervisor, so we must set used len accordingly. No? > > > > > > > > I may miss something, I think it is used for clearing the dirty > > > > bitmaps to avoid migrating unused pages, the zeroing is not needed. > > > > > > > > Thanks > > > > > > And, once the page is migrated, it's value on destination may differ > > > from one on source. > > > > Right, I will fix this in the next version. > > > > Thanks > > Btw in the get_free_page_hints() I see this: > > if (elem->in_num && dev->free_page_hint_status == FREE_PAGE_HINT_S_START) { > qemu_guest_free_page_hint(elem->in_sg[0].iov_base, > elem->in_sg[0].iov_len); > } > > It looks like only the first in sg is used, it looks like a bug? > > Thanks It sure does. > > > > > > > > > > > > > > > > > > --- > > > > > > hw/virtio/virtio-balloon.c | 6 +++--- > > > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > > > > > index c6962fcbfe..3e52daa793 100644 > > > > > > --- a/hw/virtio/virtio-balloon.c > > > > > > +++ b/hw/virtio/virtio-balloon.c > > > > > > @@ -231,7 +231,7 @@ static void balloon_stats_poll_cb(void *opaque) > > > > > > return; > > > > > > } > > > > > > > > > > > > - virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); > > > > > > + virtqueue_push(s->svq, s->stats_vq_elem, 0); > > > > > > virtio_notify(vdev, s->svq); > > > > > > g_free(s->stats_vq_elem); > > > > > > s->stats_vq_elem = NULL; > > > > > > @@ -438,7 +438,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > > > > > memory_region_unref(section.mr); > > > > > > } > > > > > > > > > > > > - virtqueue_push(vq, elem, offset); > > > > > > + virtqueue_push(vq, elem, 0); > > > > > > virtio_notify(vdev, vq); > > > > > > g_free(elem); > > > > > > virtio_balloon_pbp_free(&pbp); > > > > > > @@ -549,7 +549,7 @@ static bool get_free_page_hints(VirtIOBalloon *dev) > > > > > > } > > > > > > > > > > > > out: > > > > > > - virtqueue_push(vq, elem, 1); > > > > > > + virtqueue_push(vq, elem, 0); > > > > > > g_free(elem); > > > > > > return ret; > > > > > > } > > > > > > -- > > > > > > 2.25.1 > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-24 9:20 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-24 4:32 [PATCH] virtio-balloon: correct used length Jason Wang 2021-11-24 7:03 ` Michael S. Tsirkin 2021-11-24 7:58 ` Jason Wang 2021-11-24 8:25 ` Michael S. Tsirkin 2021-11-24 8:28 ` Jason Wang 2021-11-24 8:32 ` Jason Wang 2021-11-24 9:17 ` Michael S. Tsirkin
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).