Re: [Qemu-devel] [PATCH 4/4] virtio-balloon: keep collecting stats on save/restore

[Ladi Prosek]

On Fri, Aug 19, 2016 at 1:37 PM, Roman Kagan <address@hidden> 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 <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).

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 <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
>> >
>> >