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

[Roman Kagan]

On Thu, Sep 01, 2016 at 05:43:05PM +0200, Ladi Prosek wrote:
> On Thu, Sep 1, 2016 at 4:17 PM, Roman Kagan <address@hidden> wrote:
> > 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

Hmm, obviously my mail search skills need to be improved :(  I already
coded a patch before I saw this.

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

I woudn't say it looked like a hack so I wouldn't bother with a version
check.  Anyway let's look at the code and decide.

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

Sure I was wrong here, I used it to kick the stats collection on load.

I'll post the patch in a minute.

Roman.