From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1baibX-00033F-6S for qemu-devel@nongnu.org; Fri, 19 Aug 2016 08:08:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1baibT-0005LO-ST for qemu-devel@nongnu.org; Fri, 19 Aug 2016 08:08:55 -0400 Received: from mail-ua0-f181.google.com ([209.85.217.181]:34771) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1baibT-0005LB-NA for qemu-devel@nongnu.org; Fri, 19 Aug 2016 08:08:51 -0400 Received: by mail-ua0-f181.google.com with SMTP id k90so76198365uak.1 for ; Fri, 19 Aug 2016 05:08:51 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20160819113701.hvlxflmmmazh4ycx@rkaganb.sw.ru> References: <1471544874-26996-1-git-send-email-rkagan@virtuozzo.com> <1471544874-26996-5-git-send-email-rkagan@virtuozzo.com> <20160819113701.hvlxflmmmazh4ycx@rkaganb.sw.ru> From: Ladi Prosek Date: Fri, 19 Aug 2016 14:08:50 +0200 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Kagan , Ladi Prosek , qemu-devel , "Denis V. Lunev" , "Michael S. Tsirkin" , Liang Li On Fri, Aug 19, 2016 at 1:37 PM, Roman Kagan wrote: > On Fri, Aug 19, 2016 at 11:57:44AM +0200, Ladi Prosek wrote: >> On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan wrote: >> > Upon save/restore virtio-balloon stats acquisition stops. The reason is >> > that the fact that the (only) virtqueue element is being used by QEMU is >> > not recorded anywhere on save, so upon restore it's not released to the >> > guest, making further progress impossible. >> > >> > Saving the information about the used element would introduce unjustified >> > vmstate incompatibility. >> > >> > So instead just make sure the element is pushed before save, leaving the >> > ball on the guest side. For that, add vm state change handler to >> > virtio-ballon which would take care of pushing the element if there is >> > one. >> >> Please take a look at: >> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01038.html > > Thanks for the pointer, I wish I did a better maillist search before > submitting. > >> virtqueue_discard looks like a better choice than virtqueue_push. >> There's no need to cause churn on the guest side and receive an extra >> set of stats in between the regular timer callbacks. > > I forgot about virtqueue_discard; however, I still tend to slightly > prefer the push version as it IMO makes things easier to follow: the > pumping of stats is always initiated by the guest (both initially on > guest driver load and upon restore), and ->stats_vq_elem being non-NULL > is enough to know that it needs pushing (both in the timer callback and > in vm state change handler). I think I agree, it is nicer this way. virtqueue_discard would add another state of the system (element needs to be popped without first getting notified by the guest) which is not necessary. Thanks! >> Also in that thread see the note about stats_vq_offset which is >> misused and can be killed. > > Indeed. > > Thanks, > Roman. > >> > Signed-off-by: Roman Kagan >> > Cc: "Michael S. Tsirkin" >> > --- >> > hw/virtio/virtio-balloon.c | 27 ++++++++++++++++++++++----- >> > include/hw/virtio/virtio-balloon.h | 1 + >> > 2 files changed, 23 insertions(+), 5 deletions(-) >> > >> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c >> > index b56fecd..66a926a 100644 >> > --- a/hw/virtio/virtio-balloon.c >> > +++ b/hw/virtio/virtio-balloon.c >> > @@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs) >> > timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000); >> > } >> > >> > +static void balloon_stats_push_elem(VirtIOBalloon *s) >> > +{ >> > + VirtIODevice *vdev = VIRTIO_DEVICE(s); >> > + >> > + virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); >> > + virtio_notify(vdev, s->svq); >> > + g_free(s->stats_vq_elem); >> > + s->stats_vq_elem = NULL; >> > +} >> > + >> > static void balloon_stats_poll_cb(void *opaque) >> > { >> > VirtIOBalloon *s = opaque; >> > - VirtIODevice *vdev = VIRTIO_DEVICE(s); >> > >> > if (!s->stats_vq_elem) { >> > /* The guest hasn't sent the stats yet (either not enabled or we came >> > @@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque) >> > return; >> > } >> > >> > - virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset); >> > - virtio_notify(vdev, s->svq); >> > - g_free(s->stats_vq_elem); >> > - s->stats_vq_elem = NULL; >> > + balloon_stats_push_elem(s); >> > } >> > >> > static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name, >> > @@ -411,6 +417,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f, >> > return 0; >> > } >> > >> > +static void balloon_vm_state_change(void *opaque, int running, RunState state) >> > +{ >> > + VirtIOBalloon *s = opaque; >> > + >> > + if (!running && s->stats_vq_elem) { >> > + balloon_stats_push_elem(s); >> > + } >> > +} >> > + >> > static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) >> > { >> > VirtIODevice *vdev = VIRTIO_DEVICE(dev); >> > @@ -433,6 +448,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp) >> > s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output); >> > s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats); >> > >> > + s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s); >> > reset_stats(s); >> > } >> > >> > @@ -441,6 +457,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp) >> > VirtIODevice *vdev = VIRTIO_DEVICE(dev); >> > VirtIOBalloon *s = VIRTIO_BALLOON(dev); >> > >> > + qemu_del_vm_change_state_handler(s->change); >> > balloon_stats_destroy_timer(s); >> > qemu_remove_balloon_handler(s); >> > virtio_cleanup(vdev); >> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h >> > index 1ea13bd..d72ff7f 100644 >> > --- a/include/hw/virtio/virtio-balloon.h >> > +++ b/include/hw/virtio/virtio-balloon.h >> > @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon { >> > int64_t stats_last_update; >> > int64_t stats_poll_interval; >> > uint32_t host_features; >> > + VMChangeStateEntry *change; >> > } VirtIOBalloon; >> > >> > #endif >> > -- >> > 2.7.4 >> > >> >