* Re: [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status
2016-08-05 11:36 [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status Ladi Prosek
@ 2016-08-15 12:09 ` Stefan Hajnoczi
2016-08-15 12:35 ` Ladi Prosek
2016-08-15 13:39 ` Michael S. Tsirkin
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2016-08-15 12:09 UTC (permalink / raw)
To: Ladi Prosek; +Cc: qemu-devel, pbonzini, liang.z.li, dgilbert, mst
[-- Attachment #1: Type: text/plain, Size: 848 bytes --]
On Fri, Aug 05, 2016 at 01:36:32PM +0200, Ladi Prosek wrote:
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af429a..65457e9 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -423,6 +423,26 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> return 0;
> }
>
> +static void virtio_balloon_vmstate_cb(void *opaque, int running,
> + RunState state)
> +{
> + VirtIOBalloon *s = opaque;
> +
> + if (!running) {
> + /* put the stats element back if the VM is not running */
> + if (s->stats_vq_elem != NULL) {
> + virtqueue_discard(s->svq, s->stats_vq_elem, s->stats_vq_offset);
The third argument should be 0 because we did not write anything into
in_sg[].
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status
2016-08-15 12:09 ` Stefan Hajnoczi
@ 2016-08-15 12:35 ` Ladi Prosek
2016-08-15 12:50 ` Stefan Hajnoczi
0 siblings, 1 reply; 8+ messages in thread
From: Ladi Prosek @ 2016-08-15 12:35 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Paolo Bonzini, Liang Li, David Gilbert,
Michael Tsirkin
On Mon, Aug 15, 2016 at 2:09 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Aug 05, 2016 at 01:36:32PM +0200, Ladi Prosek wrote:
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index 5af429a..65457e9 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -423,6 +423,26 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
>> return 0;
>> }
>>
>> +static void virtio_balloon_vmstate_cb(void *opaque, int running,
>> + RunState state)
>> +{
>> + VirtIOBalloon *s = opaque;
>> +
>> + if (!running) {
>> + /* put the stats element back if the VM is not running */
>> + if (s->stats_vq_elem != NULL) {
>> + virtqueue_discard(s->svq, s->stats_vq_elem, s->stats_vq_offset);
>
> The third argument should be 0 because we did not write anything into
> in_sg[].
Thanks! Then it looks like the regular virtqueue_push in
balloon_stats_poll_cb should also be called with 0 and the
stats_vq_offset field can be deleted altogether. We never write to
in_sg.
Ladi
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status
2016-08-15 12:35 ` Ladi Prosek
@ 2016-08-15 12:50 ` Stefan Hajnoczi
2016-08-15 12:56 ` Ladi Prosek
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2016-08-15 12:50 UTC (permalink / raw)
To: Ladi Prosek
Cc: qemu-devel, Paolo Bonzini, Liang Li, David Gilbert,
Michael Tsirkin
On Mon, Aug 15, 2016 at 1:35 PM, Ladi Prosek <lprosek@redhat.com> wrote:
> On Mon, Aug 15, 2016 at 2:09 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Fri, Aug 05, 2016 at 01:36:32PM +0200, Ladi Prosek wrote:
>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>> index 5af429a..65457e9 100644
>>> --- a/hw/virtio/virtio-balloon.c
>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -423,6 +423,26 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
>>> return 0;
>>> }
>>>
>>> +static void virtio_balloon_vmstate_cb(void *opaque, int running,
>>> + RunState state)
>>> +{
>>> + VirtIOBalloon *s = opaque;
>>> +
>>> + if (!running) {
>>> + /* put the stats element back if the VM is not running */
>>> + if (s->stats_vq_elem != NULL) {
>>> + virtqueue_discard(s->svq, s->stats_vq_elem, s->stats_vq_offset);
>>
>> The third argument should be 0 because we did not write anything into
>> in_sg[].
>
> Thanks! Then it looks like the regular virtqueue_push in
> balloon_stats_poll_cb should also be called with 0 and the
> stats_vq_offset field can be deleted altogether. We never write to
> in_sg.
Yes.
Stefan
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status
2016-08-15 12:50 ` Stefan Hajnoczi
@ 2016-08-15 12:56 ` Ladi Prosek
0 siblings, 0 replies; 8+ messages in thread
From: Ladi Prosek @ 2016-08-15 12:56 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Paolo Bonzini, Liang Li, David Gilbert,
Michael Tsirkin
On Mon, Aug 15, 2016 at 2:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Aug 15, 2016 at 1:35 PM, Ladi Prosek <lprosek@redhat.com> wrote:
>> On Mon, Aug 15, 2016 at 2:09 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> On Fri, Aug 05, 2016 at 01:36:32PM +0200, Ladi Prosek wrote:
>>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>>>> index 5af429a..65457e9 100644
>>>> --- a/hw/virtio/virtio-balloon.c
>>>> +++ b/hw/virtio/virtio-balloon.c
>>>> @@ -423,6 +423,26 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
>>>> return 0;
>>>> }
>>>>
>>>> +static void virtio_balloon_vmstate_cb(void *opaque, int running,
>>>> + RunState state)
>>>> +{
>>>> + VirtIOBalloon *s = opaque;
>>>> +
>>>> + if (!running) {
>>>> + /* put the stats element back if the VM is not running */
>>>> + if (s->stats_vq_elem != NULL) {
>>>> + virtqueue_discard(s->svq, s->stats_vq_elem, s->stats_vq_offset);
>>>
>>> The third argument should be 0 because we did not write anything into
>>> in_sg[].
>>
>> Thanks! Then it looks like the regular virtqueue_push in
>> balloon_stats_poll_cb should also be called with 0 and the
>> stats_vq_offset field can be deleted altogether. We never write to
>> in_sg.
>
> Yes.
I'll send a follow-up patch fixing this later if that's ok. Liang is
already testing this patch and passing non-0 does no harm, in_sg is
empty anyway so it doesn't make a difference really.
Thanks,
Ladi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status
2016-08-05 11:36 [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status Ladi Prosek
2016-08-15 12:09 ` Stefan Hajnoczi
@ 2016-08-15 13:39 ` Michael S. Tsirkin
2016-08-15 23:08 ` Michael S. Tsirkin
2016-08-22 2:00 ` Michael S. Tsirkin
3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2016-08-15 13:39 UTC (permalink / raw)
To: Ladi Prosek; +Cc: qemu-devel, liang.z.li, pbonzini, dgilbert
On Fri, Aug 05, 2016 at 01:36:32PM +0200, Ladi Prosek wrote:
> The stats_vq_elem field keeps track of the state of the balloon stats
> virtqueue protocol but it wasn't preserved across migrations, resulting
> in losing guest memory status updates on the receiving VM.
>
> This commit adds a new VM state change handler which resets stats_vq_elem
> to NULL when the VM is stopped, eliminating the need for the field to be
> migrated. When the VM starts running again, the field is reinitialized by
> re-popping the element from the virtqueue.
>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Almost there. Except we should not touch rings
unless guest set DRIVER_OK in status register, and we should
not touch status vq at all unless VIRTIO_BALLOON_F_STATS_VQ
has been negotiated.
I'll post a patch to do this.
> ---
> hw/virtio/virtio-balloon.c | 23 +++++++++++++++++++++++
> include/hw/virtio/virtio-balloon.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af429a..65457e9 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -423,6 +423,26 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> return 0;
> }
>
> +static void virtio_balloon_vmstate_cb(void *opaque, int running,
> + RunState state)
> +{
> + VirtIOBalloon *s = opaque;
> +
> + if (!running) {
> + /* put the stats element back if the VM is not running */
> + if (s->stats_vq_elem != NULL) {
> + virtqueue_discard(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> + g_free(s->stats_vq_elem);
> + s->stats_vq_elem = NULL;
> + }
> +
> + } else {
> + /* poll stats queue for the element we may have discarded
> + * when the VM was stopped */
> + virtio_balloon_receive_stats(VIRTIO_DEVICE(s), s->svq);
> + }
> +}
> +
> static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -446,6 +466,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
>
> reset_stats(s);
> +
> + s->change = qemu_add_vm_change_state_handler(virtio_balloon_vmstate_cb, s);
> }
>
> static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> @@ -453,6 +475,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>
> + qemu_del_vm_change_state_handler(s->change);
> balloon_stats_destroy_timer(s);
> qemu_remove_balloon_handler(s);
> virtio_cleanup(vdev);
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 1ea13bd..d72ff7f 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
> int64_t stats_last_update;
> int64_t stats_poll_interval;
> uint32_t host_features;
> + VMChangeStateEntry *change;
> } VirtIOBalloon;
>
> #endif
> --
> 2.5.5
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status
2016-08-05 11:36 [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status Ladi Prosek
2016-08-15 12:09 ` Stefan Hajnoczi
2016-08-15 13:39 ` Michael S. Tsirkin
@ 2016-08-15 23:08 ` Michael S. Tsirkin
2016-08-22 2:00 ` Michael S. Tsirkin
3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2016-08-15 23:08 UTC (permalink / raw)
To: Ladi Prosek; +Cc: qemu-devel, liang.z.li, pbonzini, dgilbert
On Fri, Aug 05, 2016 at 01:36:32PM +0200, Ladi Prosek wrote:
> The stats_vq_elem field keeps track of the state of the balloon stats
> virtqueue protocol but it wasn't preserved across migrations, resulting
> in losing guest memory status updates on the receiving VM.
>
> This commit adds a new VM state change handler which resets stats_vq_elem
> to NULL when the VM is stopped, eliminating the need for the field to be
> migrated. When the VM starts running again, the field is reinitialized by
> re-popping the element from the virtqueue.
>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
I think that for the 2.7 machine type, we should
also migrate the guest-stats-polling-interval property.
Otherwise the value that was set on source
is not forwarded to the destination
properly.
And at that point, one wanders whether we should
check the interval is set before receive calling
stats.
> ---
> hw/virtio/virtio-balloon.c | 23 +++++++++++++++++++++++
> include/hw/virtio/virtio-balloon.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af429a..65457e9 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -423,6 +423,26 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> return 0;
> }
>
> +static void virtio_balloon_vmstate_cb(void *opaque, int running,
> + RunState state)
> +{
> + VirtIOBalloon *s = opaque;
> +
> + if (!running) {
> + /* put the stats element back if the VM is not running */
> + if (s->stats_vq_elem != NULL) {
> + virtqueue_discard(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> + g_free(s->stats_vq_elem);
> + s->stats_vq_elem = NULL;
> + }
> +
> + } else {
> + /* poll stats queue for the element we may have discarded
> + * when the VM was stopped */
> + virtio_balloon_receive_stats(VIRTIO_DEVICE(s), s->svq);
> + }
> +}
> +
> static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -446,6 +466,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
>
> reset_stats(s);
> +
> + s->change = qemu_add_vm_change_state_handler(virtio_balloon_vmstate_cb, s);
> }
>
> static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> @@ -453,6 +475,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>
> + qemu_del_vm_change_state_handler(s->change);
> balloon_stats_destroy_timer(s);
> qemu_remove_balloon_handler(s);
> virtio_cleanup(vdev);
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 1ea13bd..d72ff7f 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
> int64_t stats_last_update;
> int64_t stats_poll_interval;
> uint32_t host_features;
> + VMChangeStateEntry *change;
> } VirtIOBalloon;
>
> #endif
> --
> 2.5.5
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status
2016-08-05 11:36 [Qemu-devel] [PATCH v3] balloon: Fix failure of updating guest memory status Ladi Prosek
` (2 preceding siblings ...)
2016-08-15 23:08 ` Michael S. Tsirkin
@ 2016-08-22 2:00 ` Michael S. Tsirkin
3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2016-08-22 2:00 UTC (permalink / raw)
To: Ladi Prosek; +Cc: qemu-devel, liang.z.li, pbonzini, dgilbert
On Fri, Aug 05, 2016 at 01:36:32PM +0200, Ladi Prosek wrote:
> The stats_vq_elem field keeps track of the state of the balloon stats
> virtqueue protocol but it wasn't preserved across migrations, resulting
> in losing guest memory status updates on the receiving VM.
>
> This commit adds a new VM state change handler which resets stats_vq_elem
> to NULL when the VM is stopped, eliminating the need for the field to be
> migrated. When the VM starts running again, the field is reinitialized by
> re-popping the element from the virtqueue.
>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
OK, but we also must migrate the stats timer value,
otherwise management must specify it again on destination.
Only for compat machine types unfortunately otherwise
we break migration from old QEMU...
> ---
> hw/virtio/virtio-balloon.c | 23 +++++++++++++++++++++++
> include/hw/virtio/virtio-balloon.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af429a..65457e9 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -423,6 +423,26 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> return 0;
> }
>
> +static void virtio_balloon_vmstate_cb(void *opaque, int running,
> + RunState state)
> +{
> + VirtIOBalloon *s = opaque;
> +
> + if (!running) {
> + /* put the stats element back if the VM is not running */
> + if (s->stats_vq_elem != NULL) {
> + virtqueue_discard(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> + g_free(s->stats_vq_elem);
> + s->stats_vq_elem = NULL;
> + }
> +
> + } else {
> + /* poll stats queue for the element we may have discarded
> + * when the VM was stopped */
> + virtio_balloon_receive_stats(VIRTIO_DEVICE(s), s->svq);
> + }
> +}
> +
> static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -446,6 +466,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
>
> reset_stats(s);
> +
> + s->change = qemu_add_vm_change_state_handler(virtio_balloon_vmstate_cb, s);
> }
>
> static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> @@ -453,6 +475,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>
> + qemu_del_vm_change_state_handler(s->change);
> balloon_stats_destroy_timer(s);
> qemu_remove_balloon_handler(s);
> virtio_cleanup(vdev);
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 1ea13bd..d72ff7f 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
> int64_t stats_last_update;
> int64_t stats_poll_interval;
> uint32_t host_features;
> + VMChangeStateEntry *change;
> } VirtIOBalloon;
>
> #endif
> --
> 2.5.5
^ permalink raw reply [flat|nested] 8+ messages in thread