[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] balloon: Fix failure of updating guest memor
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v2] balloon: Fix failure of updating guest memory status |
Date: |
Thu, 4 Aug 2016 19:01:40 +0300 |
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
>