On Thu, Sep 01, 2016 at 05:43:05PM +0200, Ladi Prosek wrote: > 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.
It's a valid point. Try to resubmit Stefan's patches and we'll discuss? > >> 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.