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