On Thu, Sep 1, 2016 at 4:17 PM, Roman Kagan <rka...@virtuozzo.com> wrote:
> On Thu, Sep 01, 2016 at 10:35:49AM +0200, Ladi Prosek wrote:
>> On Fri, Aug 19, 2016 at 3:39 PM, Roman Kagan <rka...@virtuozzo.com> 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.
>> >
>> > Signed-off-by: Roman Kagan <rka...@virtuozzo.com>
>> > Cc: "Michael S. Tsirkin" <m...@redhat.com>
>> > Cc: Ladi Prosek <lpro...@redhat.com>
>> > Cc: Stefan Hajnoczi <stefa...@redhat.com>
>> > ---
>> > 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 6d4c57c..f00ad8e 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,
>> > @@ -414,6 +420,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);
>> > @@ -436,6 +451,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);
>> > }
>> >
>> > @@ -444,6 +460,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
>> >
>>
>> Hi Roman,
>>
>> I talked to Michael Tsirkin and he agrees with merging this patch for
>> 2.7.
>
> I'm not happy with this patch: it tries to solve the problem on the
> "save" side and therefore doesn't fix the bug when migrating from an
> earlier QEMU version.
>
> I wonder if we can do better and solve it on the "load" side. (At first
> I thought that your patch did that but on a closer look it turned out
> not the case).
>
> In particular, with Stefan's patch to restore VirtQueue->inuse, we
> should be able to just rewind ->last_avail_idx by ->inuse during "load",
> which AFAICS would also fix the bug. What do you think?
Stefan was actually going to do exactly that in
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg02455.html
but then dropped the patch in favor of this "save" side approach.
Going with Stefan's fix seems kind of unfortunate from a future-proof
perspective. It wouldn't be easy to revert it if we ever decide that
we want to leave something in the queue. But I understand that solving
the problem on the "load" side would be more helpful for real-world
deployments. Maybe we could do both but gate the load side hack with a
source QEMU version check? I wonder what Stefan thinks.
>> Could you please resubmit and use the set_status callback instead
>> of adding another VM state change handler?
>>
>> static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>> VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
>>
>> if (!vdev->vm_running && s->stats_vq_elem) {
>> balloon_stats_push_elem(s);
>> }
>> }
>>
>> and
>>
>> vdc->set_status = virtio_balloon_set_status;
>> in virtio_balloon_class_init.
>
> If the scheme I described above works out this won't be needed at all.
>
>> This is somewhat urgent because 2.7 will be out soon. If you're busy
>> or if I don't hear from you I'll post it on your behalf.
>
> OK will focus on it now.
>
> Thanks,
> Roman.
