From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVOZx-0006p8-S8 for qemu-devel@nongnu.org; Thu, 04 Aug 2016 15:45:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVOZt-0007Hy-Lg for qemu-devel@nongnu.org; Thu, 04 Aug 2016 15:45:16 -0400 Received: from mail-ua0-f180.google.com ([209.85.217.180]:34014) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVOZt-0007Hq-Ga for qemu-devel@nongnu.org; Thu, 04 Aug 2016 15:45:13 -0400 Received: by mail-ua0-f180.google.com with SMTP id 35so182126803uap.1 for ; Thu, 04 Aug 2016 12:45:13 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20160804185900-mutt-send-email-mst@kernel.org> References: <1467772593-29703-1-git-send-email-liang.z.li@intel.com> <20160706111832-mutt-send-email-mst@redhat.com> <20160706132529-mutt-send-email-mst@redhat.com> <20160706163656-mutt-send-email-mst@redhat.com> <20160802030708-mutt-send-email-mst@kernel.org> <20160804185900-mutt-send-email-mst@kernel.org> From: Ladi Prosek Date: Thu, 4 Aug 2016 21:45:12 +0200 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v2] balloon: Fix failure of updating guest memory status List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: "Li, Liang Z" , "qemu-devel@nongnu.org" , Paolo Bonzini , "dgilbert@redhat.com" On Thu, Aug 4, 2016 at 6:01 PM, Michael S. Tsirkin 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 wrote: >> > On Tue, Aug 2, 2016 at 2:11 AM, Michael S. Tsirkin 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 >> >>> > > > > > > Suggested-by: Paolo Bonzini >> >>> > > > > > > Cc: Michael S. Tsirkin >> >>> > > > > > > Cc: Ladi Prosek >> >>> > > > > > > Cc: Paolo Bonzini >> >>> > > > > > >> >>> > > > > > 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 >> Date: Thu, 4 Aug 2016 15:22:05 +0200 >> Subject: [PATCH] balloon: preserve stats virtqueue state across migrations >> >> Signed-off-by: Ladi Prosek >> --- >> 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 >> >