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

[Ladi Prosek]

On Thu, Aug 4, 2016 at 6:01 PM, Michael S. Tsirkin <address@hidden> wrote:
> 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).

Will do. Thanks!

>
>> --
>> 2.5.5
>>
>