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

[Adam Litke]

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:
> > 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.
> > 
> > Signed-off-by: Adam Litke <address@hidden>
> > ---
> >  hw/virtio-balloon.c |   12 +++++++++++-
> >  1 files changed, 11 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> > index 9fe3886..dcdada6 100644
> > --- a/hw/virtio-balloon.c
> > +++ b/hw/virtio-balloon.c
> > @@ -190,7 +190,17 @@ static void virtio_balloon_set_config(VirtIODevice 
> > *vdev,
> >  
> >  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.

> Isn't it better to disable it on the "info balloon" side, so the guest
> knows that the host may start requesting stat info eventually? I suggest
> doing this:

While I think this method would also work, I would really like to use
the feature bit if possible, since that is what the mechanism is
designed for.

> ---
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 1e4dfdd..92e9057 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -30,6 +30,10 @@
>  #include <sys/mman.h>
>  #endif
> 
> +/* Disable guest-provided stats by now (bz#623903, bz#626544) */
> +#define ENABLE_GUEST_STATS   0
> +
> +
>  typedef struct VirtIOBalloon
>  {
>      VirtIODevice vdev;
> @@ -84,12 +88,14 @@ static QObject *get_stats_qobject(VirtIOBalloon *dev)
>                                    VIRTIO_BALLOON_PFN_SHIFT);
> 
>      stat_put(dict, "actual", actual);
> +#if ENABLE_GUEST_STATS
>      stat_put(dict, "mem_swapped_in", dev->stats[VIRTIO_BALLOON_S_SWAP_IN]);
>      stat_put(dict, "mem_swapped_out", dev->stats[VIRTIO_BALLOON_S_SWAP_OUT]);
>      stat_put(dict, "major_page_faults", dev->stats[VIRTIO_BALLOON_S_MAJFLT]);
>      stat_put(dict, "minor_page_faults", dev->stats[VIRTIO_BALLOON_S_MINFLT]);
>      stat_put(dict, "free_mem", dev->stats[VIRTIO_BALLOON_S_MEMFREE]);
>      stat_put(dict, "total_mem", dev->stats[VIRTIO_BALLOON_S_MEMTOT]);
> +#endif
> 
>      return QOBJECT(dict);
>  }
> @@ -215,7 +221,7 @@ static void virtio_balloon_to_target(void *opaque, 
> ram_addr_t target,
>          }
>          dev->stats_callback = cb;
>          dev->stats_opaque_callback_data = cb_data; 
> -        if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
> +        if (ENABLE_GUEST_STATS && (dev->vdev.guest_features & (1 << 
> VIRTIO_BALLOON_F_STATS_VQ))) {
>              virtqueue_push(dev->svq, &dev->stats_vq_elem, 
> dev->stats_vq_offset);
>              virtio_notify(&dev->vdev, dev->svq);
>          } else {
> 

-- 
Thanks,
Adam