On Thu, Aug 04, 2016 at 05:14:14PM +0200, Ladi Prosek wrote: > On Wed, Aug 3, 2016 at 9:25 AM, Ladi Prosek <lpro...@redhat.com> wrote: > > On Tue, Aug 2, 2016 at 2:11 AM, Michael S. Tsirkin <m...@redhat.com> 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 <liang.z...@intel.com> > >>> > > > > > > Suggested-by: Paolo Bonzini <pbonz...@redhat.com> > >>> > > > > > > Cc: Michael S. Tsirkin <m...@redhat.com> > >>> > > > > > > Cc: Ladi Prosek <lpro...@redhat.com> > >>> > > > > > > Cc: Paolo Bonzini <pbonz...@redhat.com> > >>> > > > > > > >>> > > > > > 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 <lpro...@redhat.com> > Date: Thu, 4 Aug 2016 15:22:05 +0200 > Subject: [PATCH] balloon: preserve stats virtqueue state across migrations > > Signed-off-by: Ladi Prosek <lpro...@redhat.com> > --- > 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 >