[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on sa
From: |
Roman Kagan |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore |
Date: |
Fri, 19 Aug 2016 14:37:01 +0300 |
User-agent: |
Mutt/1.6.2-neo (2016-08-08) |
On Fri, Aug 19, 2016 at 11:57:44AM +0200, Ladi Prosek wrote:
> On Thu, Aug 18, 2016 at 8:27 PM, Roman Kagan <address@hidden> 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).
> 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 <address@hidden>
> > Cc: "Michael S. Tsirkin" <address@hidden>
> > ---
> > 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
> >
> >
Re: [Qemu-devel] [PATCH 0/4] virtio-balloon: assorted fixes, Stefan Hajnoczi, 2016/08/19