Re: [Qemu-devel] [PATCH v2] balloon: Fix failure of updating guest memory status

[Michael S. Tsirkin]

On Thu, Aug 04, 2016 at 05:14:14PM +0200, Ladi Prosek wrote:
> On Wed, Aug 3, 2016 at 9:25 AM, Ladi Prosek <address@hidden> wrote:
> > On Tue, Aug 2, 2016 at 2:11 AM, Michael S. Tsirkin <address@hidden> wrote:
> >> On Mon, Aug 01, 2016 at 11:59:31PM +0000, Li, Liang Z wrote:
> >>> > On Wed, Jul 06, 2016 at 12:49:06PM +0000, Li, Liang Z wrote:
> >>> > > > > > > After live migration, 'guest-stats' can't get the expected
> >>> > > > > > > memory status in the guest. This issue is caused by commit
> >>> > 4eae2a657d.
> >>> > > > > > > The value of 's->stats_vq_elem' will be NULL after live
> >>> > > > > > > migration, and the check in the function
> >>> > > > > > > 'balloon_stats_poll_cb()' will prevent the 'virtio_notify()'
> >>> > > > > > > from executing. So guest will not update the memory status.
> >>> > > > > > >
> >>> > > > > > > Commit 4eae2a657d is doing the right thing, but 
> >>> > > > > > > 's->stats_vq_elem'
> >>> > > > > > > should be treated as part of balloon device state and migrated
> >>> > > > > > > to destination if it's not NULL to make everything works well.
> >>> > > > > > >
> >>> > > > > > > Signed-off-by: Liang Li <address@hidden>
> >>> > > > > > > Suggested-by: Paolo Bonzini <address@hidden>
> >>> > > > > > > Cc: Michael S. Tsirkin <address@hidden>
> >>> > > > > > > Cc: Ladi Prosek <address@hidden>
> >>> > > > > > > Cc: Paolo Bonzini <address@hidden>
> >>> > > > > >
> >>> > > > > > I agree there's an issue but we don't change versions anymore.
> >>> > > > > > Breaking migrations for everyone is also not nice.
> >>> > > > > >
> >>> > > > > > How about queueing virtio_balloon_receive_stats so it will get
> >>> > > > > > invoked when vm starts?
> >>> > > > > >
> >>> > > > >
> >>> > > > > Could you give more explanation about how it works?  I can't 
> >>> > > > > catch you.
> >>> > > > >
> >>> > > > > Thanks!
> >>> > > > > Liang
> >>> > > >
> >>> > > > virtqueue_discard before migration
> >>> > > >
> >>> > > > virtio_balloon_receive_stats after migration
> >>> > > >
> >>> > >
> >>> > > Sorry, I still can't catch you. Maybe it's easier for you to submit a
> >>> > > patch than writing a lot a words to make me understand your idea.
> >>> >
> >>> > I'm rather busy now.  I might look into it towards end of the month.
> >>> >
> >>> > > I just don't understand why not to use the version to make things
> >>> > > easier, is that not the original intent of version id?
> >>> >
> >>> > This was the original idea but we stopped using version ids since they 
> >>> > have
> >>> > many shortcomings.
> >>> >
> >>> > > If we want to extend the device and more states are needed, the idea
> >>> > > you suggest can be used as a common solution?
> >>> > >
> >>> > > Thanks!
> >>> > > Liang
> >>> >
> >>> > The idea is to try to avoid adding more state. that's not always 
> >>> > possible but in
> >>> > this case element was seen but not consumed yet, so it should be 
> >>> > possible
> >>> > for destination to simply get it from the VQ again.
> >>> >
> >>> > > > --
> >>> > > > MST
> >>>
> >>> Hi Michel,
> >>>
> >>> Do you have time for this issue recently?
> >>>
> >>> Thanks!
> >>> Liang
> >
> > Hi Liang,
> >
> > I should be able to look into it this week if you help me with testing.
> >
> > Thanks,
> > Ladi
> 
> Please try the attached patch. I have tested it with a simple
> 'migrate' to save the state and then '-incoming' to load it back.
> 
> One question for you: is it expected that stats_poll_interval is not
> preserved by save/load? I had to explicitly set
> guest-stats-polling-interval on the receiving VM to start getting
> stats again. It's also the reason why the new
> virtio_balloon_receive_stats call is not under if
> (balloon_stats_enabled(s)) because this condition always evaluates to
> false for me.
> 
> Thanks!
> Ladi
> 
> >> Sorry, doesn't look like I will.
> >> Idea is to make sure balloon_stats_poll_cb runs
> >> on source. This will set stats_vq_elem to NULL.
> >>
> >>
> >> --
> >> MST

> From f2f779e12f4aa4d3469d1b44e54484e66f82a2d7 Mon Sep 17 00:00:00 2001
> From: Ladi Prosek <address@hidden>
> Date: Thu, 4 Aug 2016 15:22:05 +0200
> Subject: [PATCH] balloon: preserve stats virtqueue state across migrations
> 
> Signed-off-by: Ladi Prosek <address@hidden>
> ---
>  hw/virtio/virtio-balloon.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af429a..1293be0 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -396,6 +396,19 @@ static void virtio_balloon_to_target(void *opaque, 
> ram_addr_t target)
>      trace_virtio_balloon_to_target(target, dev->num_pages);
>  }
>  
> +static void virtio_balloon_save(QEMUFile *f, void *opaque, size_t size)
> +{
> +    VirtIOBalloon *s = VIRTIO_BALLOON(opaque);
> +
> +    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;
> +    }
> +
> +    virtio_save(VIRTIO_DEVICE(opaque), f);
> +}
> +
>  static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)
>  {
>      VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> @@ -417,6 +430,9 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, 
> QEMUFile *f,
>      s->num_pages = qemu_get_be32(f);
>      s->actual = qemu_get_be32(f);
>  
> +    /* poll the queue for the element we may have discarded on save */
> +    virtio_balloon_receive_stats(VIRTIO_DEVICE(s), s->svq);
> +
>      if (balloon_stats_enabled(s)) {
>          balloon_stats_change_timer(s, s->stats_poll_interval);
>      }
> @@ -481,7 +497,7 @@ static void virtio_balloon_instance_init(Object *obj)
>                          NULL, s, NULL);
>  }
>  
> -VMSTATE_VIRTIO_DEVICE(balloon, 1, virtio_balloon_load, virtio_vmstate_save);
> +VMSTATE_VIRTIO_DEVICE(balloon, 1, virtio_balloon_load, virtio_balloon_save);
>  
>  static Property virtio_balloon_properties[] = {
>      DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features,

So almost, but I think I'd be happier if instead of save/load
we handled vm stop/run. Simply specify vmstate_change
callback and check vm_running value.

This way we don't modify guest memory when vm is not
running, and that is a useful invariant to keep
(e.g. save+load+save will produce two identical images).


> -- 
> 2.5.5
>