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

[Eduardo Habkost]

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?

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:

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

-- 
Eduardo