Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Eduardo Habkost]

On Tue, Sep 14, 2010 at 11:41:55AM -0300, Eduardo Habkost wrote:
> On Tue, Sep 14, 2010 at 09:24:11AM -0500, Adam Litke wrote:
> > On Tue, 2010-09-14 at 11:09 -0300, Eduardo Habkost wrote:
> > > On Wed, Sep 08, 2010 at 09:21:16AM -0500, Adam Litke wrote:
> [...]
> > > >  static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, 
> > > > uint32_t f)
> > > >  {
> > > > -    f |= (1 << VIRTIO_BALLOON_F_STATS_VQ);
> > > > +    /*
> > > > +     * The addition of memory stats reporting to the virtio balloon 
> > > > causes
> > > > +     * the 'info balloon' command to become asynchronous.  This is a 
> > > > regression
> > > > +     * because in some cases it can hang the user monitor.
> > > > +     *
> > > > +     * Disable this feature until a better interface for asynchronous 
> > > > commands
> > > > +     * can be worked out.
> > > > +     *
> > > > +     * -aglitke
> > > > +     */
> > > > +    /* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */
> > > 
> > > 
> > > This field is guest-visible, won't this cause problems on migration?
> > 
> > I haven't followed migration very closely, but isn't this a common
> > problem whenever one migrates a vm to a newer version of qemu that has
> > more features.  I thought that virtio feature negotiation would ensure
> > that stats have been disabled at the device level and would remain
> > disabled post migration.  Please correct me if I am mistaken.
> 
> If this is the case, then it's another reason to keep the feature bit
> enabled: the feature bit just let the guest know that the host may
> request balloon stats at any moment, and it's better to keep that
> capability in case the guest is migrated, isn't it?
> 
> Also, what happens if we try to migrate from a qemu version that had the
> feature bit set to a qemu version without this feature bit?
> 
> I don't know the details either, but even if this works gracefully for
> migration in both directions, it sound much simpler to not change the
> guest-visible machine model and just change the "info balloon" behavior.

It looks worse than that: by disabling this feature you will break migration
from older qemu versions that had the old "info balloon" behavior.

This is the incoming virtio migration code:

int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
    uint32_t supported_features =
        vdev->binding->get_features(vdev->binding_opaque);
[...]
    qemu_get_8s(f, &vdev->status);
    qemu_get_8s(f, &vdev->isr);
    qemu_get_be16s(f, &vdev->queue_sel);
    qemu_get_be32s(f, &features);
    if (features & ~supported_features) {
        fprintf(stderr, "Features 0x%x unsupported. Allowed features: 0x%x\n",
                features, supported_features);
        return -1;
    }
[...]

To make things worse: virtio_net_load() doesn't check the return value of
virtio_load(), so it will probably crash in an unpredictable way.

I keep my suggestion: if all we need is to change the way "info balloon"
behaves, then we can simply change the "info balloon" behavior, intead
of changing the guest-visible machine model.

-- 
Eduardo