[Qemu-devel] [PATCH 0/6] QMP: add balloon-get-memory-stats command

[Qemu-devel] [PATCH 0/6] QMP: add balloon-get-memory-stats command

[Luiz Capitulino]

This series re-enables the balloon memory statistics feature disabled
long ago by commit 11724ff.

The feature has been disabled because it added a severe bug: query-balloon
would hang if the guest didn't respond. This, in turn, would also cause a
hang in libvirt.

To avoid that bug, the solution implemented in this series uses a command
and an event. The command gets the process started by sending a request to
guest and returns. Later, when the guest makes the memory stats info
available, it's sent to the client by means of an QMP event (please, take a
look at patch 6/6 for full details).

This series has an issue though: it lacks HMP support. The reason for this
is that there's no way to wait for an event in HMP. There are two ways to
solve this:

 1. We wait to have support for events in the QAPI. This way it will be
    possible to wait for events from HMP

 2. Add a query-balloon-memory-stats command which polls for the guest
    response (and immediatelly returns if the guest response is not
    available)

I'm going on only with the event for now, as this is slightly simpler.

 QMP/qmp-events.txt  |   36 ++++++++++++++++++++++++
 balloon.c           |   61 +++++++++++++++++++++++++++++++---------
 balloon.h           |    7 +++--
 hmp.c               |   25 +----------------
 hw/virtio-balloon.c |   76 ++++++++++++++++++++++++++++++++++-----------------
 monitor.c           |    2 +
 monitor.h           |    1 +
 qapi-schema.json    |   42 +++++++++++++++-------------
 qmp-commands.hx     |    5 +++
 9 files changed, 169 insertions(+), 86 deletions(-)

[Qemu-devel] [PATCH 1/6] balloon: qmp_balloon(): Use error_set()

[Luiz Capitulino]

Commit d72f326431e280a619a0fd55e27d3737747f8178 converted the
balloon command to the QAPI, but forgot to convert one qerror_report()
usage. Fix it.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 balloon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/balloon.c b/balloon.c
index 0166744..aa354f7 100644
--- a/balloon.c
+++ b/balloon.c
@@ -108,7 +108,7 @@ void qmp_balloon(int64_t value, Error **errp)
     }
 
     if (value <= 0) {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, "target", "a size");
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size");
         return;
     }
     
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 2/6] balloon: Drop unused include

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 balloon.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/balloon.h b/balloon.h
index b60fd5d..17fe300 100644
--- a/balloon.h
+++ b/balloon.h
@@ -14,7 +14,6 @@
 #ifndef _QEMU_BALLOON_H
 #define _QEMU_BALLOON_H
 
-#include "monitor.h"
 #include "qapi-types.h"
 
 typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 3/6] balloon: Rename QEMUBalloonStatus to QEMUBalloonInfo

[Luiz Capitulino]

Next commit will introduce the QEMUBalloonStats type, it can cause
confusion with QEMUBalloonStatus. Also, QEMUBalloonInfo matches
better with BalloonInfo, which is the type used to return balloon
information in QMP.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 balloon.c           |   18 +++++++++---------
 balloon.h           |    4 ++--
 hw/virtio-balloon.c |    4 ++--
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/balloon.c b/balloon.c
index aa354f7..b32b487 100644
--- a/balloon.c
+++ b/balloon.c
@@ -32,13 +32,13 @@
 #include "qmp-commands.h"
 
 static QEMUBalloonEvent *balloon_event_fn;
-static QEMUBalloonStatus *balloon_stat_fn;
+static QEMUBalloonInfo *balloon_info_fn;
 static void *balloon_opaque;
 
 int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
-                             QEMUBalloonStatus *stat_func, void *opaque)
+                             QEMUBalloonInfo *info_func, void *opaque)
 {
-    if (balloon_event_fn || balloon_stat_fn || balloon_opaque) {
+    if (balloon_event_fn || balloon_info_fn || balloon_opaque) {
         /* We're already registered one balloon handler.  How many can
          * a guest really have?
          */
@@ -46,7 +46,7 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
         return -1;
     }
     balloon_event_fn = event_func;
-    balloon_stat_fn = stat_func;
+    balloon_info_fn = info_func;
     balloon_opaque = opaque;
     return 0;
 }
@@ -57,7 +57,7 @@ void qemu_remove_balloon_handler(void *opaque)
         return;
     }
     balloon_event_fn = NULL;
-    balloon_stat_fn = NULL;
+    balloon_info_fn = NULL;
     balloon_opaque = NULL;
 }
 
@@ -71,12 +71,12 @@ static int qemu_balloon(ram_addr_t target)
     return 1;
 }
 
-static int qemu_balloon_status(BalloonInfo *info)
+static int qemu_balloon_info(BalloonInfo *info)
 {
-    if (!balloon_stat_fn) {
+    if (!balloon_info_fn) {
         return 0;
     }
-    balloon_stat_fn(balloon_opaque, info);
+    balloon_info_fn(balloon_opaque, info);
     return 1;
 }
 
@@ -91,7 +91,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
 
     info = g_malloc0(sizeof(*info));
 
-    if (qemu_balloon_status(info) == 0) {
+    if (qemu_balloon_info(info) == 0) {
         error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
         qapi_free_BalloonInfo(info);
         return NULL;
diff --git a/balloon.h b/balloon.h
index 17fe300..a539354 100644
--- a/balloon.h
+++ b/balloon.h
@@ -17,10 +17,10 @@
 #include "qapi-types.h"
 
 typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
-typedef void (QEMUBalloonStatus)(void *opaque, BalloonInfo *info);
+typedef void (QEMUBalloonInfo)(void *opaque, BalloonInfo *info);
 
 int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
-			     QEMUBalloonStatus *stat_func, void *opaque);
+			     QEMUBalloonInfo *info_func, void *opaque);
 void qemu_remove_balloon_handler(void *opaque);
 
 #endif
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index ce9d2c9..4307f4c 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -156,7 +156,7 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
     return f;
 }
 
-static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
+static void virtio_balloon_info(void *opaque, BalloonInfo *info)
 {
     VirtIOBalloon *dev = opaque;
 
@@ -236,7 +236,7 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
     s->vdev.get_features = virtio_balloon_get_features;
 
     ret = qemu_add_balloon_handler(virtio_balloon_to_target,
-                                   virtio_balloon_stat, s);
+                                   virtio_balloon_info, s);
     if (ret < 0) {
         virtio_cleanup(&s->vdev);
         return NULL;
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 4/6] balloon: Refactor kvm sync mmu check

[Luiz Capitulino]

qmp_query_balloon() and qmp_balloon() perform the same check, move it
to check_kvm_sync_mmu().

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 balloon.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/balloon.c b/balloon.c
index b32b487..d340ae3 100644
--- a/balloon.c
+++ b/balloon.c
@@ -80,12 +80,20 @@ static int qemu_balloon_info(BalloonInfo *info)
     return 1;
 }
 
+static bool check_kvm_sync_mmu(Error **errp)
+{
+    if (kvm_enabled() && !kvm_has_sync_mmu()) {
+        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+        return false;
+    }
+    return true;
+}
+
 BalloonInfo *qmp_query_balloon(Error **errp)
 {
     BalloonInfo *info;
 
-    if (kvm_enabled() && !kvm_has_sync_mmu()) {
-        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+    if (!check_kvm_sync_mmu(errp)) {
         return NULL;
     }
 
@@ -102,8 +110,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
 
 void qmp_balloon(int64_t value, Error **errp)
 {
-    if (kvm_enabled() && !kvm_has_sync_mmu()) {
-        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+    if (!check_kvm_sync_mmu(errp)) {
         return;
     }
 
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 5/6] balloon: Drop old stats interface

[Luiz Capitulino]

It has never been used and next patches will introduce a new,
usable interface.

Note that dropping this won't break compatibility because all
fields are optional.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp.c            |   25 +------------------------
 qapi-schema.json |   21 +--------------------
 2 files changed, 2 insertions(+), 44 deletions(-)

diff --git a/hmp.c b/hmp.c
index 8ff8c94..4cd99a1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -381,30 +381,7 @@ void hmp_info_balloon(Monitor *mon)
         return;
     }
 
-    monitor_printf(mon, "balloon: actual=%" PRId64, info->actual >> 20);
-    if (info->has_mem_swapped_in) {
-        monitor_printf(mon, " mem_swapped_in=%" PRId64, info->mem_swapped_in);
-    }
-    if (info->has_mem_swapped_out) {
-        monitor_printf(mon, " mem_swapped_out=%" PRId64, info->mem_swapped_out);
-    }
-    if (info->has_major_page_faults) {
-        monitor_printf(mon, " major_page_faults=%" PRId64,
-                       info->major_page_faults);
-    }
-    if (info->has_minor_page_faults) {
-        monitor_printf(mon, " minor_page_faults=%" PRId64,
-                       info->minor_page_faults);
-    }
-    if (info->has_free_mem) {
-        monitor_printf(mon, " free_mem=%" PRId64, info->free_mem);
-    }
-    if (info->has_total_mem) {
-        monitor_printf(mon, " total_mem=%" PRId64, info->total_mem);
-    }
-
-    monitor_printf(mon, "\n");
-
+    monitor_printf(mon, "balloon: actual=%" PRId64 "\n", info->actual >> 20);
     qapi_free_BalloonInfo(info);
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index d02ee86..24a42e3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -679,28 +679,9 @@
 #
 # @actual: the number of bytes the balloon currently contains
 #
-# @mem_swapped_in: #optional number of pages swapped in within the guest
-#
-# @mem_swapped_out: #optional number of pages swapped out within the guest
-#
-# @major_page_faults: #optional number of major page faults within the guest
-#
-# @minor_page_faults: #optional number of minor page faults within the guest
-#
-# @free_mem: #optional amount of memory (in bytes) free in the guest
-#
-# @total_mem: #optional amount of memory (in bytes) visible to the guest
-#
 # Since: 0.14.0
-#
-# Notes: all current versions of QEMU do not fill out optional information in
-#        this structure.
 ##
-{ 'type': 'BalloonInfo',
-  'data': {'actual': 'int', '*mem_swapped_in': 'int',
-           '*mem_swapped_out': 'int', '*major_page_faults': 'int',
-           '*minor_page_faults': 'int', '*free_mem': 'int',
-           '*total_mem': 'int'} }
+{ 'type': 'BalloonInfo', 'data': { 'actual': 'int' } }
 
 ##
 # @query-balloon:
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Luiz Capitulino]

This commit adds a QMP API for the guest provided memory statistics
(long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).

The approach taken by the original commit
(625a5befc2e3200b396594f002218d235e375da5) was to extend the
query-balloon command. It introduced a severe bug though: query-balloon
would hang if the guest didn't respond.

The approach taken by this commit is asynchronous and thus avoids
any QMP hangs.

First, a client has to issue the balloon-get-memory-stats command.
That command gets the process started by only sending a request to
the guest, it doesn't block. When the memory stats are made available
by the guest, they are returned to the client as an QMP event.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt  |   36 ++++++++++++++++++++++++
 balloon.c           |   30 +++++++++++++++++++-
 balloon.h           |    4 ++-
 hw/virtio-balloon.c |   76 ++++++++++++++++++++++++++++++++++-----------------
 monitor.c           |    2 +
 monitor.h           |    1 +
 qapi-schema.json    |   21 ++++++++++++++
 qmp-commands.hx     |    5 +++
 8 files changed, 147 insertions(+), 28 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 06cb404..b34b289 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -1,6 +1,42 @@
                    QEMU Monitor Protocol Events
                    ============================
 
+BALLOON_MEMORY_STATS
+--------------------
+
+Emitted when memory statistics information is made available by the guest.
+
+Data:
+
+- "memory-swapped-in": number of pages swapped in within the guest
+  (json-int, optional)
+- "memory-swapped-out": number of pages swapped out within the guest
+  (json-int, optional)
+- "major-page-faults": number of major page faults within the guest
+  (json-int, optional)
+- "minor-page-faults": number of minor page faults within the guest
+  (json-int, optional)
+- "memory-free": amount of memory (in bytes) free in the guest
+  (json-int, optional)
+- "memory-total": amount of memory (in bytes) visible to the guest
+  (json-int, optional)
+
+Example:
+
+{ "event": "BALLOON_MEMORY_STATS",
+    "data": { "memory-free": 847941632,
+              "major-page-faults": 225,
+              "memory-swapped-in": 0,
+              "minor-page-faults": 222317,
+              "memory-total": 1045516288,
+              "memory-swapped-out": 0 },
+    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+
+Notes: o The balloon-get-memory-stats command must be issued first, to tell
+         the guest to make the memory statistics available.
+       o The event may not contain all data members when emitted
+
+
 BLOCK_IO_ERROR
 --------------
 
diff --git a/balloon.c b/balloon.c
index d340ae3..1c1a3c1 100644
--- a/balloon.c
+++ b/balloon.c
@@ -33,12 +33,15 @@
 
 static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonInfo *balloon_info_fn;
+static QEMUBalloonStats *balloon_stats_fn;
 static void *balloon_opaque;
 
 int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
-                             QEMUBalloonInfo *info_func, void *opaque)
+                             QEMUBalloonInfo *info_func,
+                             QEMUBalloonStats *stats_func, void *opaque)
 {
-    if (balloon_event_fn || balloon_info_fn || balloon_opaque) {
+    if (balloon_event_fn || balloon_info_fn || balloon_stats_fn ||
+        balloon_opaque) {
         /* We're already registered one balloon handler.  How many can
          * a guest really have?
          */
@@ -47,6 +50,7 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
     }
     balloon_event_fn = event_func;
     balloon_info_fn = info_func;
+    balloon_stats_fn = stats_func;
     balloon_opaque = opaque;
     return 0;
 }
@@ -58,6 +62,7 @@ void qemu_remove_balloon_handler(void *opaque)
     }
     balloon_event_fn = NULL;
     balloon_info_fn = NULL;
+    balloon_stats_fn = NULL;
     balloon_opaque = NULL;
 }
 
@@ -80,6 +85,15 @@ static int qemu_balloon_info(BalloonInfo *info)
     return 1;
 }
 
+static int qemu_balloon_stats(void)
+{
+    if (!balloon_stats_fn) {
+        return 0;
+    }
+    balloon_stats_fn(balloon_opaque);
+    return 1;
+}
+
 static bool check_kvm_sync_mmu(Error **errp)
 {
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
@@ -89,6 +103,18 @@ static bool check_kvm_sync_mmu(Error **errp)
     return true;
 }
 
+void qmp_balloon_get_memory_stats(Error **errp)
+{
+    if (!check_kvm_sync_mmu(errp)) {
+        return;
+    }
+
+    if (qemu_balloon_stats() == 0) {
+        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
+        return;
+    }
+}
+
 BalloonInfo *qmp_query_balloon(Error **errp)
 {
     BalloonInfo *info;
diff --git a/balloon.h b/balloon.h
index a539354..509e477 100644
--- a/balloon.h
+++ b/balloon.h
@@ -18,9 +18,11 @@
 
 typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
 typedef void (QEMUBalloonInfo)(void *opaque, BalloonInfo *info);
+typedef void (QEMUBalloonStats)(void *opaque);
 
 int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
-			     QEMUBalloonInfo *info_func, void *opaque);
+			     QEMUBalloonInfo *info_func, QEMUBalloonStats *stats_func,
+                 void *opaque);
 void qemu_remove_balloon_handler(void *opaque);
 
 #endif
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 4307f4c..8bc842c 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -22,6 +22,8 @@
 #include "virtio-balloon.h"
 #include "kvm.h"
 #include "exec-memory.h"
+#include "monitor.h"
+#include "qemu-objects.h"
 
 #if defined(__linux__)
 #include <sys/mman.h>
@@ -34,6 +36,7 @@ typedef struct VirtIOBalloon
     uint32_t num_pages;
     uint32_t actual;
     uint64_t stats[VIRTIO_BALLOON_S_NR];
+    bool stats_requested;
     VirtQueueElement stats_vq_elem;
     size_t stats_vq_offset;
     DeviceState *qdev;
@@ -56,12 +59,10 @@ static void balloon_page(void *addr, int deflate)
 /*
  * reset_stats - Mark all items in the stats array as unset
  *
- * This function needs to be called at device intialization and before
- * before updating to a set of newly-generated stats.  This will ensure that no
- * stale values stick around in case the guest reports a subset of the supported
- * statistics.
+ * This function ensures that no stale values stick around in case the guest
+ * reports a subset of the supported statistics.
  */
-static inline void reset_stats(VirtIOBalloon *dev)
+static void reset_stats(VirtIOBalloon *dev)
 {
     int i;
     for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
@@ -101,6 +102,38 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
     }
 }
 
+static void emit_qmp_balloon_event(const VirtIOBalloon *dev)
+{
+    int i;
+    QDict *stats;
+    const struct stats_strings {
+        int stat_idx;
+        const char *stat_name;
+    } stats_strings[] = {
+        { VIRTIO_BALLOON_S_SWAP_IN, "memory-swapped-in" },
+        { VIRTIO_BALLOON_S_SWAP_OUT, "memory-swapped-out" },
+        { VIRTIO_BALLOON_S_MAJFLT, "major-page-faults" },
+        { VIRTIO_BALLOON_S_MINFLT, "minor-page-faults" },
+        { VIRTIO_BALLOON_S_MEMFREE, "memory-free" },
+        { VIRTIO_BALLOON_S_MEMTOT, "memory-total" },
+        { 0, NULL },
+    };
+
+    stats = qdict_new();
+    for (i = 0; stats_strings[i].stat_name != NULL; i++) {
+        if (dev->stats[i] != -1) {
+            qdict_put(stats, stats_strings[i].stat_name,
+                      qint_from_int(dev->stats[i]));
+        }
+    }
+
+    if (qdict_size(stats) > 0) {
+        monitor_protocol_event(QEVENT_BALLOON_STATS, QOBJECT(stats));
+    }
+
+    QDECREF(stats);
+}
+
 static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
@@ -108,7 +141,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOBalloonStat stat;
     size_t offset = 0;
 
-    if (!virtqueue_pop(vq, elem)) {
+    if (!virtqueue_pop(vq, elem) || !s->stats_requested) {
         return;
     }
 
@@ -128,6 +161,9 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
             s->stats[tag] = val;
     }
     s->stats_vq_offset = offset;
+    s->stats_requested = false;
+
+    emit_qmp_balloon_event(s);
 }
 
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
@@ -156,31 +192,21 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
     return f;
 }
 
-static void virtio_balloon_info(void *opaque, BalloonInfo *info)
+static void virtio_balloon_stats(void *opaque)
 {
     VirtIOBalloon *dev = opaque;
 
-#if 0
-    /* Disable guest-provided stats for now. For more details please check:
-     * https://bugzilla.redhat.com/show_bug.cgi?id=623903
-     *
-     * If you do enable it (which is probably not going to happen as we
-     * need a new command for it), remember that you also need to fill the
-     * appropriate members of the BalloonInfo structure so that the stats
-     * are returned to the client.
-     */
     if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
+        dev->stats_requested = true;
         virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
         virtio_notify(&dev->vdev, dev->svq);
         return;
     }
-#endif
-
-    /* Stats are not supported.  Clear out any stale values that might
-     * have been set by a more featureful guest kernel.
-     */
-    reset_stats(dev);
+}
 
+static void virtio_balloon_info(void *opaque, BalloonInfo *info)
+{
+    VirtIOBalloon *dev = opaque;
     info->actual = ram_size - ((uint64_t) dev->actual <<
                                VIRTIO_BALLOON_PFN_SHIFT);
 }
@@ -236,18 +262,18 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
     s->vdev.get_features = virtio_balloon_get_features;
 
     ret = qemu_add_balloon_handler(virtio_balloon_to_target,
-                                   virtio_balloon_info, s);
+                                   virtio_balloon_info,
+                                   virtio_balloon_stats, s);
     if (ret < 0) {
         virtio_cleanup(&s->vdev);
         return NULL;
     }
 
+    s->stats_requested = false;
     s->ivq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
     s->dvq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
 
-    reset_stats(s);
-
     s->qdev = dev;
     register_savevm(dev, "virtio-balloon", -1, 1,
                     virtio_balloon_save, virtio_balloon_load, s);
diff --git a/monitor.c b/monitor.c
index aadbdcb..868f2a0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -484,6 +484,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
             break;
         case QEVENT_BLOCK_JOB_CANCELLED:
             event_name = "BLOCK_JOB_CANCELLED";
+        case QEVENT_BALLOON_STATS:
+            event_name = "BALLOON_MEMORY_STATS";
             break;
         default:
             abort();
diff --git a/monitor.h b/monitor.h
index b72ea07..76e26c7 100644
--- a/monitor.h
+++ b/monitor.h
@@ -38,6 +38,7 @@ typedef enum MonitorEvent {
     QEVENT_SPICE_DISCONNECTED,
     QEVENT_BLOCK_JOB_COMPLETED,
     QEVENT_BLOCK_JOB_CANCELLED,
+    QEVENT_BALLOON_STATS,
     QEVENT_MAX,
 } MonitorEvent;
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 24a42e3..c4d8d0c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1563,3 +1563,24 @@
 { 'command': 'qom-list-types',
   'data': { '*implements': 'str', '*abstract': 'bool' },
   'returns': [ 'ObjectTypeInfo' ] }
+
+##
+# @balloon-get-memory-stats
+#
+# Ask the guest's balloon driver for guest memory statistics.
+#
+# This command will only get the process started and will return immediately.
+# The BALLOON_MEMORY_STATS event will be emitted when the statistics
+# information is returned by the guest.
+#
+# Returns: nothing on success
+#          If the balloon driver is enabled but not functional because the KVM
+#          kernel module cannot support it, KvmMissingCap
+#          If no balloon device is present, DeviceNotActive
+#
+# Notes: There's no guarantees the guest will ever respond, thus the
+#        BALLOON_MEMORY_STATS event may never be emitted.
+#
+# Since: 1.1
+##
+{ 'command': 'balloon-get-memory-stats' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b5e2ab8..52c1fc3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2047,3 +2047,8 @@ EQMP
         .args_type  = "implements:s?,abstract:b?",
         .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
     },
+    {
+        .name       = "balloon-get-memory-stats",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_balloon_get_memory_stats,
+    },
-- 
1.7.9.111.gf3fb0.dirty

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Adam Litke]

On Wed, Feb 08, 2012 at 06:30:40PM -0200, Luiz Capitulino wrote:
> This commit adds a QMP API for the guest provided memory statistics
> (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> 
> The approach taken by the original commit
> (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> query-balloon command. It introduced a severe bug though: query-balloon
> would hang if the guest didn't respond.
> 
> The approach taken by this commit is asynchronous and thus avoids
> any QMP hangs.
> 
> First, a client has to issue the balloon-get-memory-stats command.
> That command gets the process started by only sending a request to
> the guest, it doesn't block. When the memory stats are made available
> by the guest, they are returned to the client as an QMP event.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  QMP/qmp-events.txt  |   36 ++++++++++++++++++++++++
>  balloon.c           |   30 +++++++++++++++++++-
>  balloon.h           |    4 ++-
>  hw/virtio-balloon.c |   76 ++++++++++++++++++++++++++++++++++-----------------
>  monitor.c           |    2 +
>  monitor.h           |    1 +
>  qapi-schema.json    |   21 ++++++++++++++
>  qmp-commands.hx     |    5 +++
>  8 files changed, 147 insertions(+), 28 deletions(-)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 06cb404..b34b289 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -1,6 +1,42 @@
>                     QEMU Monitor Protocol Events
>                     ============================
> 
> +BALLOON_MEMORY_STATS
> +--------------------
> +
> +Emitted when memory statistics information is made available by the guest.
> +
> +Data:
> +
> +- "memory-swapped-in": number of pages swapped in within the guest
> +  (json-int, optional)
> +- "memory-swapped-out": number of pages swapped out within the guest
> +  (json-int, optional)

These are in units of pages where memory-total and memory-free are in bytes.
Maybe it makes sense to name these "memory-pages-swapped-in/out" to avoid
confusion.  Yes it makes them longer, but I can imagine all of the questions in
the future when users don't expect the units to be in pages.

> +- "major-page-faults": number of major page faults within the guest
> +  (json-int, optional)
> +- "minor-page-faults": number of minor page faults within the guest
> +  (json-int, optional)
> +- "memory-free": amount of memory (in bytes) free in the guest
> +  (json-int, optional)
> +- "memory-total": amount of memory (in bytes) visible to the guest
> +  (json-int, optional)
> +
> +Example:
> +
> +{ "event": "BALLOON_MEMORY_STATS",
> +    "data": { "memory-free": 847941632,
> +              "major-page-faults": 225,
> +              "memory-swapped-in": 0,
> +              "minor-page-faults": 222317,
> +              "memory-total": 1045516288,
> +              "memory-swapped-out": 0 },
> +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +
> +Notes: o The balloon-get-memory-stats command must be issued first, to tell
> +         the guest to make the memory statistics available.
> +       o The event may not contain all data members when emitted
> +
> +
>  BLOCK_IO_ERROR
>  --------------
> 
> diff --git a/balloon.c b/balloon.c
> index d340ae3..1c1a3c1 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -33,12 +33,15 @@
> 
>  static QEMUBalloonEvent *balloon_event_fn;
>  static QEMUBalloonInfo *balloon_info_fn;
> +static QEMUBalloonStats *balloon_stats_fn;
>  static void *balloon_opaque;
> 
>  int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> -                             QEMUBalloonInfo *info_func, void *opaque)
> +                             QEMUBalloonInfo *info_func,
> +                             QEMUBalloonStats *stats_func, void *opaque)
>  {
> -    if (balloon_event_fn || balloon_info_fn || balloon_opaque) {
> +    if (balloon_event_fn || balloon_info_fn || balloon_stats_fn ||
> +        balloon_opaque) {
>          /* We're already registered one balloon handler.  How many can
>           * a guest really have?
>           */
> @@ -47,6 +50,7 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
>      }
>      balloon_event_fn = event_func;
>      balloon_info_fn = info_func;
> +    balloon_stats_fn = stats_func;
>      balloon_opaque = opaque;
>      return 0;
>  }
> @@ -58,6 +62,7 @@ void qemu_remove_balloon_handler(void *opaque)
>      }
>      balloon_event_fn = NULL;
>      balloon_info_fn = NULL;
> +    balloon_stats_fn = NULL;
>      balloon_opaque = NULL;
>  }
> 
> @@ -80,6 +85,15 @@ static int qemu_balloon_info(BalloonInfo *info)
>      return 1;
>  }
> 
> +static int qemu_balloon_stats(void)
> +{
> +    if (!balloon_stats_fn) {
> +        return 0;
> +    }
> +    balloon_stats_fn(balloon_opaque);
> +    return 1;
> +}
> +
>  static bool check_kvm_sync_mmu(Error **errp)
>  {
>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
> @@ -89,6 +103,18 @@ static bool check_kvm_sync_mmu(Error **errp)
>      return true;
>  }
> 
> +void qmp_balloon_get_memory_stats(Error **errp)
> +{
> +    if (!check_kvm_sync_mmu(errp)) {
> +        return;
> +    }
> +
> +    if (qemu_balloon_stats() == 0) {
> +        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
> +        return;
> +    }
> +}
> +
>  BalloonInfo *qmp_query_balloon(Error **errp)
>  {
>      BalloonInfo *info;
> diff --git a/balloon.h b/balloon.h
> index a539354..509e477 100644
> --- a/balloon.h
> +++ b/balloon.h
> @@ -18,9 +18,11 @@
> 
>  typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
>  typedef void (QEMUBalloonInfo)(void *opaque, BalloonInfo *info);
> +typedef void (QEMUBalloonStats)(void *opaque);
> 
>  int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> -			     QEMUBalloonInfo *info_func, void *opaque);
> +			     QEMUBalloonInfo *info_func, QEMUBalloonStats *stats_func,
> +                 void *opaque);
>  void qemu_remove_balloon_handler(void *opaque);
> 
>  #endif
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 4307f4c..8bc842c 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -22,6 +22,8 @@
>  #include "virtio-balloon.h"
>  #include "kvm.h"
>  #include "exec-memory.h"
> +#include "monitor.h"
> +#include "qemu-objects.h"
> 
>  #if defined(__linux__)
>  #include <sys/mman.h>
> @@ -34,6 +36,7 @@ typedef struct VirtIOBalloon
>      uint32_t num_pages;
>      uint32_t actual;
>      uint64_t stats[VIRTIO_BALLOON_S_NR];
> +    bool stats_requested;
>      VirtQueueElement stats_vq_elem;
>      size_t stats_vq_offset;
>      DeviceState *qdev;
> @@ -56,12 +59,10 @@ static void balloon_page(void *addr, int deflate)
>  /*
>   * reset_stats - Mark all items in the stats array as unset
>   *
> - * This function needs to be called at device intialization and before
> - * before updating to a set of newly-generated stats.  This will ensure that no
> - * stale values stick around in case the guest reports a subset of the supported
> - * statistics.
> + * This function ensures that no stale values stick around in case the guest
> + * reports a subset of the supported statistics.
>   */
> -static inline void reset_stats(VirtIOBalloon *dev)
> +static void reset_stats(VirtIOBalloon *dev)
>  {
>      int i;
>      for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
> @@ -101,6 +102,38 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>      }
>  }
> 
> +static void emit_qmp_balloon_event(const VirtIOBalloon *dev)
> +{
> +    int i;
> +    QDict *stats;
> +    const struct stats_strings {
> +        int stat_idx;
> +        const char *stat_name;
> +    } stats_strings[] = {
> +        { VIRTIO_BALLOON_S_SWAP_IN, "memory-swapped-in" },
> +        { VIRTIO_BALLOON_S_SWAP_OUT, "memory-swapped-out" },
> +        { VIRTIO_BALLOON_S_MAJFLT, "major-page-faults" },
> +        { VIRTIO_BALLOON_S_MINFLT, "minor-page-faults" },
> +        { VIRTIO_BALLOON_S_MEMFREE, "memory-free" },
> +        { VIRTIO_BALLOON_S_MEMTOT, "memory-total" },
> +        { 0, NULL },
> +    };
> +
> +    stats = qdict_new();
> +    for (i = 0; stats_strings[i].stat_name != NULL; i++) {
> +        if (dev->stats[i] != -1) {
> +            qdict_put(stats, stats_strings[i].stat_name,
> +                      qint_from_int(dev->stats[i]));
> +        }
> +    }
> +
> +    if (qdict_size(stats) > 0) {
> +        monitor_protocol_event(QEVENT_BALLOON_STATS, QOBJECT(stats));
> +    }
> +
> +    QDECREF(stats);
> +}
> +
>  static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
> @@ -108,7 +141,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
>      VirtIOBalloonStat stat;
>      size_t offset = 0;
> 
> -    if (!virtqueue_pop(vq, elem)) {
> +    if (!virtqueue_pop(vq, elem) || !s->stats_requested) {
>          return;
>      }
> 
> @@ -128,6 +161,9 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
>              s->stats[tag] = val;
>      }
>      s->stats_vq_offset = offset;
> +    s->stats_requested = false;
> +
> +    emit_qmp_balloon_event(s);
>  }
> 
>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> @@ -156,31 +192,21 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
>      return f;
>  }
> 
> -static void virtio_balloon_info(void *opaque, BalloonInfo *info)
> +static void virtio_balloon_stats(void *opaque)
>  {
>      VirtIOBalloon *dev = opaque;
> 
> -#if 0
> -    /* Disable guest-provided stats for now. For more details please check:
> -     * https://bugzilla.redhat.com/show_bug.cgi?id=623903
> -     *
> -     * If you do enable it (which is probably not going to happen as we
> -     * need a new command for it), remember that you also need to fill the
> -     * appropriate members of the BalloonInfo structure so that the stats
> -     * are returned to the client.
> -     */
>      if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
> +        dev->stats_requested = true;
>          virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
>          virtio_notify(&dev->vdev, dev->svq);
>          return;
>      }
> -#endif
> -
> -    /* Stats are not supported.  Clear out any stale values that might
> -     * have been set by a more featureful guest kernel.
> -     */
> -    reset_stats(dev);
> +}
> 
> +static void virtio_balloon_info(void *opaque, BalloonInfo *info)
> +{
> +    VirtIOBalloon *dev = opaque;
>      info->actual = ram_size - ((uint64_t) dev->actual <<
>                                 VIRTIO_BALLOON_PFN_SHIFT);
>  }
> @@ -236,18 +262,18 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
>      s->vdev.get_features = virtio_balloon_get_features;
> 
>      ret = qemu_add_balloon_handler(virtio_balloon_to_target,
> -                                   virtio_balloon_info, s);
> +                                   virtio_balloon_info,
> +                                   virtio_balloon_stats, s);
>      if (ret < 0) {
>          virtio_cleanup(&s->vdev);
>          return NULL;
>      }
> 
> +    s->stats_requested = false;
>      s->ivq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
>      s->dvq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
>      s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
> 
> -    reset_stats(s);
> -
>      s->qdev = dev;
>      register_savevm(dev, "virtio-balloon", -1, 1,
>                      virtio_balloon_save, virtio_balloon_load, s);
> diff --git a/monitor.c b/monitor.c
> index aadbdcb..868f2a0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -484,6 +484,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>              break;
>          case QEVENT_BLOCK_JOB_CANCELLED:
>              event_name = "BLOCK_JOB_CANCELLED";
> +        case QEVENT_BALLOON_STATS:
> +            event_name = "BALLOON_MEMORY_STATS";
>              break;
>          default:
>              abort();
> diff --git a/monitor.h b/monitor.h
> index b72ea07..76e26c7 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -38,6 +38,7 @@ typedef enum MonitorEvent {
>      QEVENT_SPICE_DISCONNECTED,
>      QEVENT_BLOCK_JOB_COMPLETED,
>      QEVENT_BLOCK_JOB_CANCELLED,
> +    QEVENT_BALLOON_STATS,
>      QEVENT_MAX,
>  } MonitorEvent;
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 24a42e3..c4d8d0c 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1563,3 +1563,24 @@
>  { 'command': 'qom-list-types',
>    'data': { '*implements': 'str', '*abstract': 'bool' },
>    'returns': [ 'ObjectTypeInfo' ] }
> +
> +##
> +# @balloon-get-memory-stats
> +#
> +# Ask the guest's balloon driver for guest memory statistics.
> +#
> +# This command will only get the process started and will return immediately.
> +# The BALLOON_MEMORY_STATS event will be emitted when the statistics
> +# information is returned by the guest.
> +#
> +# Returns: nothing on success
> +#          If the balloon driver is enabled but not functional because the KVM
> +#          kernel module cannot support it, KvmMissingCap
> +#          If no balloon device is present, DeviceNotActive
> +#
> +# Notes: There's no guarantees the guest will ever respond, thus the
> +#        BALLOON_MEMORY_STATS event may never be emitted.
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'balloon-get-memory-stats' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b5e2ab8..52c1fc3 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2047,3 +2047,8 @@ EQMP
>          .args_type  = "implements:s?,abstract:b?",
>          .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
>      },
> +    {
> +        .name       = "balloon-get-memory-stats",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_balloon_get_memory_stats,
> +    },
> -- 
> 1.7.9.111.gf3fb0.dirty
> 

-- 
Adam Litke <agl@us.ibm.com>
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Luiz Capitulino]

On Thu, 9 Feb 2012 13:26:40 -0600
Adam Litke <agl@us.ibm.com> wrote:

> On Wed, Feb 08, 2012 at 06:30:40PM -0200, Luiz Capitulino wrote:
> > This commit adds a QMP API for the guest provided memory statistics
> > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> > 
> > The approach taken by the original commit
> > (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> > query-balloon command. It introduced a severe bug though: query-balloon
> > would hang if the guest didn't respond.
> > 
> > The approach taken by this commit is asynchronous and thus avoids
> > any QMP hangs.
> > 
> > First, a client has to issue the balloon-get-memory-stats command.
> > That command gets the process started by only sending a request to
> > the guest, it doesn't block. When the memory stats are made available
> > by the guest, they are returned to the client as an QMP event.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  QMP/qmp-events.txt  |   36 ++++++++++++++++++++++++
> >  balloon.c           |   30 +++++++++++++++++++-
> >  balloon.h           |    4 ++-
> >  hw/virtio-balloon.c |   76 ++++++++++++++++++++++++++++++++++-----------------
> >  monitor.c           |    2 +
> >  monitor.h           |    1 +
> >  qapi-schema.json    |   21 ++++++++++++++
> >  qmp-commands.hx     |    5 +++
> >  8 files changed, 147 insertions(+), 28 deletions(-)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index 06cb404..b34b289 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -1,6 +1,42 @@
> >                     QEMU Monitor Protocol Events
> >                     ============================
> > 
> > +BALLOON_MEMORY_STATS
> > +--------------------
> > +
> > +Emitted when memory statistics information is made available by the guest.
> > +
> > +Data:
> > +
> > +- "memory-swapped-in": number of pages swapped in within the guest
> > +  (json-int, optional)
> > +- "memory-swapped-out": number of pages swapped out within the guest
> > +  (json-int, optional)
> 
> These are in units of pages where memory-total and memory-free are in bytes.
> Maybe it makes sense to name these "memory-pages-swapped-in/out" to avoid
> confusion.  Yes it makes them longer, but I can imagine all of the questions in
> the future when users don't expect the units to be in pages.

Seems like a good idea.

> 
> > +- "major-page-faults": number of major page faults within the guest
> > +  (json-int, optional)
> > +- "minor-page-faults": number of minor page faults within the guest
> > +  (json-int, optional)
> > +- "memory-free": amount of memory (in bytes) free in the guest
> > +  (json-int, optional)
> > +- "memory-total": amount of memory (in bytes) visible to the guest
> > +  (json-int, optional)
> > +
> > +Example:
> > +
> > +{ "event": "BALLOON_MEMORY_STATS",
> > +    "data": { "memory-free": 847941632,
> > +              "major-page-faults": 225,
> > +              "memory-swapped-in": 0,
> > +              "minor-page-faults": 222317,
> > +              "memory-total": 1045516288,
> > +              "memory-swapped-out": 0 },
> > +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> > +
> > +Notes: o The balloon-get-memory-stats command must be issued first, to tell
> > +         the guest to make the memory statistics available.
> > +       o The event may not contain all data members when emitted
> > +
> > +
> >  BLOCK_IO_ERROR
> >  --------------
> > 
> > diff --git a/balloon.c b/balloon.c
> > index d340ae3..1c1a3c1 100644
> > --- a/balloon.c
> > +++ b/balloon.c
> > @@ -33,12 +33,15 @@
> > 
> >  static QEMUBalloonEvent *balloon_event_fn;
> >  static QEMUBalloonInfo *balloon_info_fn;
> > +static QEMUBalloonStats *balloon_stats_fn;
> >  static void *balloon_opaque;
> > 
> >  int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> > -                             QEMUBalloonInfo *info_func, void *opaque)
> > +                             QEMUBalloonInfo *info_func,
> > +                             QEMUBalloonStats *stats_func, void *opaque)
> >  {
> > -    if (balloon_event_fn || balloon_info_fn || balloon_opaque) {
> > +    if (balloon_event_fn || balloon_info_fn || balloon_stats_fn ||
> > +        balloon_opaque) {
> >          /* We're already registered one balloon handler.  How many can
> >           * a guest really have?
> >           */
> > @@ -47,6 +50,7 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> >      }
> >      balloon_event_fn = event_func;
> >      balloon_info_fn = info_func;
> > +    balloon_stats_fn = stats_func;
> >      balloon_opaque = opaque;
> >      return 0;
> >  }
> > @@ -58,6 +62,7 @@ void qemu_remove_balloon_handler(void *opaque)
> >      }
> >      balloon_event_fn = NULL;
> >      balloon_info_fn = NULL;
> > +    balloon_stats_fn = NULL;
> >      balloon_opaque = NULL;
> >  }
> > 
> > @@ -80,6 +85,15 @@ static int qemu_balloon_info(BalloonInfo *info)
> >      return 1;
> >  }
> > 
> > +static int qemu_balloon_stats(void)
> > +{
> > +    if (!balloon_stats_fn) {
> > +        return 0;
> > +    }
> > +    balloon_stats_fn(balloon_opaque);
> > +    return 1;
> > +}
> > +
> >  static bool check_kvm_sync_mmu(Error **errp)
> >  {
> >      if (kvm_enabled() && !kvm_has_sync_mmu()) {
> > @@ -89,6 +103,18 @@ static bool check_kvm_sync_mmu(Error **errp)
> >      return true;
> >  }
> > 
> > +void qmp_balloon_get_memory_stats(Error **errp)
> > +{
> > +    if (!check_kvm_sync_mmu(errp)) {
> > +        return;
> > +    }
> > +
> > +    if (qemu_balloon_stats() == 0) {
> > +        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
> > +        return;
> > +    }
> > +}
> > +
> >  BalloonInfo *qmp_query_balloon(Error **errp)
> >  {
> >      BalloonInfo *info;
> > diff --git a/balloon.h b/balloon.h
> > index a539354..509e477 100644
> > --- a/balloon.h
> > +++ b/balloon.h
> > @@ -18,9 +18,11 @@
> > 
> >  typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
> >  typedef void (QEMUBalloonInfo)(void *opaque, BalloonInfo *info);
> > +typedef void (QEMUBalloonStats)(void *opaque);
> > 
> >  int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> > -			     QEMUBalloonInfo *info_func, void *opaque);
> > +			     QEMUBalloonInfo *info_func, QEMUBalloonStats *stats_func,
> > +                 void *opaque);
> >  void qemu_remove_balloon_handler(void *opaque);
> > 
> >  #endif
> > diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> > index 4307f4c..8bc842c 100644
> > --- a/hw/virtio-balloon.c
> > +++ b/hw/virtio-balloon.c
> > @@ -22,6 +22,8 @@
> >  #include "virtio-balloon.h"
> >  #include "kvm.h"
> >  #include "exec-memory.h"
> > +#include "monitor.h"
> > +#include "qemu-objects.h"
> > 
> >  #if defined(__linux__)
> >  #include <sys/mman.h>
> > @@ -34,6 +36,7 @@ typedef struct VirtIOBalloon
> >      uint32_t num_pages;
> >      uint32_t actual;
> >      uint64_t stats[VIRTIO_BALLOON_S_NR];
> > +    bool stats_requested;
> >      VirtQueueElement stats_vq_elem;
> >      size_t stats_vq_offset;
> >      DeviceState *qdev;
> > @@ -56,12 +59,10 @@ static void balloon_page(void *addr, int deflate)
> >  /*
> >   * reset_stats - Mark all items in the stats array as unset
> >   *
> > - * This function needs to be called at device intialization and before
> > - * before updating to a set of newly-generated stats.  This will ensure that no
> > - * stale values stick around in case the guest reports a subset of the supported
> > - * statistics.
> > + * This function ensures that no stale values stick around in case the guest
> > + * reports a subset of the supported statistics.
> >   */
> > -static inline void reset_stats(VirtIOBalloon *dev)
> > +static void reset_stats(VirtIOBalloon *dev)
> >  {
> >      int i;
> >      for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
> > @@ -101,6 +102,38 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >      }
> >  }
> > 
> > +static void emit_qmp_balloon_event(const VirtIOBalloon *dev)
> > +{
> > +    int i;
> > +    QDict *stats;
> > +    const struct stats_strings {
> > +        int stat_idx;
> > +        const char *stat_name;
> > +    } stats_strings[] = {
> > +        { VIRTIO_BALLOON_S_SWAP_IN, "memory-swapped-in" },
> > +        { VIRTIO_BALLOON_S_SWAP_OUT, "memory-swapped-out" },
> > +        { VIRTIO_BALLOON_S_MAJFLT, "major-page-faults" },
> > +        { VIRTIO_BALLOON_S_MINFLT, "minor-page-faults" },
> > +        { VIRTIO_BALLOON_S_MEMFREE, "memory-free" },
> > +        { VIRTIO_BALLOON_S_MEMTOT, "memory-total" },
> > +        { 0, NULL },
> > +    };
> > +
> > +    stats = qdict_new();
> > +    for (i = 0; stats_strings[i].stat_name != NULL; i++) {
> > +        if (dev->stats[i] != -1) {
> > +            qdict_put(stats, stats_strings[i].stat_name,
> > +                      qint_from_int(dev->stats[i]));
> > +        }
> > +    }
> > +
> > +    if (qdict_size(stats) > 0) {
> > +        monitor_protocol_event(QEVENT_BALLOON_STATS, QOBJECT(stats));
> > +    }
> > +
> > +    QDECREF(stats);
> > +}
> > +
> >  static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> >      VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
> > @@ -108,7 +141,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> >      VirtIOBalloonStat stat;
> >      size_t offset = 0;
> > 
> > -    if (!virtqueue_pop(vq, elem)) {
> > +    if (!virtqueue_pop(vq, elem) || !s->stats_requested) {
> >          return;
> >      }
> > 
> > @@ -128,6 +161,9 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> >              s->stats[tag] = val;
> >      }
> >      s->stats_vq_offset = offset;
> > +    s->stats_requested = false;
> > +
> > +    emit_qmp_balloon_event(s);
> >  }
> > 
> >  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> > @@ -156,31 +192,21 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
> >      return f;
> >  }
> > 
> > -static void virtio_balloon_info(void *opaque, BalloonInfo *info)
> > +static void virtio_balloon_stats(void *opaque)
> >  {
> >      VirtIOBalloon *dev = opaque;
> > 
> > -#if 0
> > -    /* Disable guest-provided stats for now. For more details please check:
> > -     * https://bugzilla.redhat.com/show_bug.cgi?id=623903
> > -     *
> > -     * If you do enable it (which is probably not going to happen as we
> > -     * need a new command for it), remember that you also need to fill the
> > -     * appropriate members of the BalloonInfo structure so that the stats
> > -     * are returned to the client.
> > -     */
> >      if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
> > +        dev->stats_requested = true;
> >          virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
> >          virtio_notify(&dev->vdev, dev->svq);
> >          return;
> >      }
> > -#endif
> > -
> > -    /* Stats are not supported.  Clear out any stale values that might
> > -     * have been set by a more featureful guest kernel.
> > -     */
> > -    reset_stats(dev);
> > +}
> > 
> > +static void virtio_balloon_info(void *opaque, BalloonInfo *info)
> > +{
> > +    VirtIOBalloon *dev = opaque;
> >      info->actual = ram_size - ((uint64_t) dev->actual <<
> >                                 VIRTIO_BALLOON_PFN_SHIFT);
> >  }
> > @@ -236,18 +262,18 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
> >      s->vdev.get_features = virtio_balloon_get_features;
> > 
> >      ret = qemu_add_balloon_handler(virtio_balloon_to_target,
> > -                                   virtio_balloon_info, s);
> > +                                   virtio_balloon_info,
> > +                                   virtio_balloon_stats, s);
> >      if (ret < 0) {
> >          virtio_cleanup(&s->vdev);
> >          return NULL;
> >      }
> > 
> > +    s->stats_requested = false;
> >      s->ivq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
> >      s->dvq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
> >      s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
> > 
> > -    reset_stats(s);
> > -
> >      s->qdev = dev;
> >      register_savevm(dev, "virtio-balloon", -1, 1,
> >                      virtio_balloon_save, virtio_balloon_load, s);
> > diff --git a/monitor.c b/monitor.c
> > index aadbdcb..868f2a0 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -484,6 +484,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
> >              break;
> >          case QEVENT_BLOCK_JOB_CANCELLED:
> >              event_name = "BLOCK_JOB_CANCELLED";
> > +        case QEVENT_BALLOON_STATS:
> > +            event_name = "BALLOON_MEMORY_STATS";
> >              break;
> >          default:
> >              abort();
> > diff --git a/monitor.h b/monitor.h
> > index b72ea07..76e26c7 100644
> > --- a/monitor.h
> > +++ b/monitor.h
> > @@ -38,6 +38,7 @@ typedef enum MonitorEvent {
> >      QEVENT_SPICE_DISCONNECTED,
> >      QEVENT_BLOCK_JOB_COMPLETED,
> >      QEVENT_BLOCK_JOB_CANCELLED,
> > +    QEVENT_BALLOON_STATS,
> >      QEVENT_MAX,
> >  } MonitorEvent;
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 24a42e3..c4d8d0c 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1563,3 +1563,24 @@
> >  { 'command': 'qom-list-types',
> >    'data': { '*implements': 'str', '*abstract': 'bool' },
> >    'returns': [ 'ObjectTypeInfo' ] }
> > +
> > +##
> > +# @balloon-get-memory-stats
> > +#
> > +# Ask the guest's balloon driver for guest memory statistics.
> > +#
> > +# This command will only get the process started and will return immediately.
> > +# The BALLOON_MEMORY_STATS event will be emitted when the statistics
> > +# information is returned by the guest.
> > +#
> > +# Returns: nothing on success
> > +#          If the balloon driver is enabled but not functional because the KVM
> > +#          kernel module cannot support it, KvmMissingCap
> > +#          If no balloon device is present, DeviceNotActive
> > +#
> > +# Notes: There's no guarantees the guest will ever respond, thus the
> > +#        BALLOON_MEMORY_STATS event may never be emitted.
> > +#
> > +# Since: 1.1
> > +##
> > +{ 'command': 'balloon-get-memory-stats' }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index b5e2ab8..52c1fc3 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -2047,3 +2047,8 @@ EQMP
> >          .args_type  = "implements:s?,abstract:b?",
> >          .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
> >      },
> > +    {
> > +        .name       = "balloon-get-memory-stats",
> > +        .args_type  = "",
> > +        .mhandler.cmd_new = qmp_marshal_input_balloon_get_memory_stats,
> > +    },
> > -- 
> > 1.7.9.111.gf3fb0.dirty
> > 
> 

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Anthony Liguori]

On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> This commit adds a QMP API for the guest provided memory statistics
> (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
>
> The approach taken by the original commit
> (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> query-balloon command. It introduced a severe bug though: query-balloon
> would hang if the guest didn't respond.
>
> The approach taken by this commit is asynchronous and thus avoids
> any QMP hangs.
>
> First, a client has to issue the balloon-get-memory-stats command.
> That command gets the process started by only sending a request to
> the guest, it doesn't block. When the memory stats are made available
> by the guest, they are returned to the client as an QMP event.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>

Do we need this to be stable in 1.1?

We can do this pretty nicely through QOM.  We can have a polling property in the 
virtio-balloon driver, that when set, will enable the virtio-balloon device to 
poll the guest for statistics.

We can also have properties for each of the memory statistics and a timestamp 
for when the last update was.

I think this is a friendlier approach for clients, and a cleaner approach from a 
QEMU perspective.

There's nothing generic about this functionality.  It's extremely specific to 
virtio-balloon.  We just lacked ways to expose device specific function pre-QOM.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Michael Roth]

On Fri, Feb 17, 2012 at 10:55:41AM -0600, Anthony Liguori wrote:
> On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> >This commit adds a QMP API for the guest provided memory statistics
> >(long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> >
> >The approach taken by the original commit
> >(625a5befc2e3200b396594f002218d235e375da5) was to extend the
> >query-balloon command. It introduced a severe bug though: query-balloon
> >would hang if the guest didn't respond.
> >
> >The approach taken by this commit is asynchronous and thus avoids
> >any QMP hangs.
> >
> >First, a client has to issue the balloon-get-memory-stats command.
> >That command gets the process started by only sending a request to
> >the guest, it doesn't block. When the memory stats are made available
> >by the guest, they are returned to the client as an QMP event.
> >
> >Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> 
> Do we need this to be stable in 1.1?
> 
> We can do this pretty nicely through QOM.  We can have a polling
> property in the virtio-balloon driver, that when set, will enable
> the virtio-balloon device to poll the guest for statistics.
> 
> We can also have properties for each of the memory statistics and a
> timestamp for when the last update was.
> 
> I think this is a friendlier approach for clients, and a cleaner
> approach from a QEMU perspective.
> 
> There's nothing generic about this functionality.  It's extremely
> specific to virtio-balloon.  We just lacked ways to expose device
> specific function pre-QOM.

I'm not so sure, I think proxying guest agent commands through QMP
would hit very similar snags, for instance.

> 
> Regards,
> 
> Anthony Liguori
> 
> 

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Luiz Capitulino]

On Fri, 17 Feb 2012 10:55:41 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> > This commit adds a QMP API for the guest provided memory statistics
> > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> >
> > The approach taken by the original commit
> > (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> > query-balloon command. It introduced a severe bug though: query-balloon
> > would hang if the guest didn't respond.
> >
> > The approach taken by this commit is asynchronous and thus avoids
> > any QMP hangs.
> >
> > First, a client has to issue the balloon-get-memory-stats command.
> > That command gets the process started by only sending a request to
> > the guest, it doesn't block. When the memory stats are made available
> > by the guest, they are returned to the client as an QMP event.
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> 
> Do we need this to be stable in 1.1?

Well, this is disabled for a long time already and libvirt needs it, so I'd
say asap, but isn't it possible to implement this with current QOM?

> We can do this pretty nicely through QOM.  We can have a polling property in the 
> virtio-balloon driver, that when set, will enable the virtio-balloon device to 
> poll the guest for statistics.
>
> 
> We can also have properties for each of the memory statistics and a timestamp 
> for when the last update was.
> 
> I think this is a friendlier approach for clients, and a cleaner approach from a 
> QEMU perspective.

I agree it's friendlier, but is it a good idea to keep polling the guest for
something that may never be needed by a mngt app (real question)?

We could allow the mngt app to do the polling by adding a query-balloon-stats
command (instead of balloon-get-memory-stats & event). This command could
return the latest available stats if any (with a timestamp) and query the
guest for new stats.

> 
> There's nothing generic about this functionality.  It's extremely specific to 
> virtio-balloon.  We just lacked ways to expose device specific function pre-QOM.
> 
> Regards,
> 
> Anthony Liguori
> 

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Anthony Liguori]

On 02/17/2012 11:09 AM, Michael Roth wrote:
> On Fri, Feb 17, 2012 at 10:55:41AM -0600, Anthony Liguori wrote:
>> On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
>>> This commit adds a QMP API for the guest provided memory statistics
>>> (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
>>>
>>> The approach taken by the original commit
>>> (625a5befc2e3200b396594f002218d235e375da5) was to extend the
>>> query-balloon command. It introduced a severe bug though: query-balloon
>>> would hang if the guest didn't respond.
>>>
>>> The approach taken by this commit is asynchronous and thus avoids
>>> any QMP hangs.
>>>
>>> First, a client has to issue the balloon-get-memory-stats command.
>>> That command gets the process started by only sending a request to
>>> the guest, it doesn't block. When the memory stats are made available
>>> by the guest, they are returned to the client as an QMP event.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>
>> Do we need this to be stable in 1.1?
>>
>> We can do this pretty nicely through QOM.  We can have a polling
>> property in the virtio-balloon driver, that when set, will enable
>> the virtio-balloon device to poll the guest for statistics.
>>
>> We can also have properties for each of the memory statistics and a
>> timestamp for when the last update was.
>>
>> I think this is a friendlier approach for clients, and a cleaner
>> approach from a QEMU perspective.
>>
>> There's nothing generic about this functionality.  It's extremely
>> specific to virtio-balloon.  We just lacked ways to expose device
>> specific function pre-QOM.
>
> I'm not so sure, I think proxying guest agent commands through QMP
> would hit very similar snags, for instance.

We would proxy guest agent commands as asynchronous QMP commands, no?

Regards,

Anthony Liguori

>
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>
>
>

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Michael Roth]

On Fri, Feb 17, 2012 at 02:07:47PM -0600, Anthony Liguori wrote:
> On 02/17/2012 11:09 AM, Michael Roth wrote:
> >On Fri, Feb 17, 2012 at 10:55:41AM -0600, Anthony Liguori wrote:
> >>On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> >>>This commit adds a QMP API for the guest provided memory statistics
> >>>(long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> >>>
> >>>The approach taken by the original commit
> >>>(625a5befc2e3200b396594f002218d235e375da5) was to extend the
> >>>query-balloon command. It introduced a severe bug though: query-balloon
> >>>would hang if the guest didn't respond.
> >>>
> >>>The approach taken by this commit is asynchronous and thus avoids
> >>>any QMP hangs.
> >>>
> >>>First, a client has to issue the balloon-get-memory-stats command.
> >>>That command gets the process started by only sending a request to
> >>>the guest, it doesn't block. When the memory stats are made available
> >>>by the guest, they are returned to the client as an QMP event.
> >>>
> >>>Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >>
> >>Do we need this to be stable in 1.1?
> >>
> >>We can do this pretty nicely through QOM.  We can have a polling
> >>property in the virtio-balloon driver, that when set, will enable
> >>the virtio-balloon device to poll the guest for statistics.
> >>
> >>We can also have properties for each of the memory statistics and a
> >>timestamp for when the last update was.
> >>
> >>I think this is a friendlier approach for clients, and a cleaner
> >>approach from a QEMU perspective.
> >>
> >>There's nothing generic about this functionality.  It's extremely
> >>specific to virtio-balloon.  We just lacked ways to expose device
> >>specific function pre-QOM.
> >
> >I'm not so sure, I think proxying guest agent commands through QMP
> >would hit very similar snags, for instance.
> 
> We would proxy guest agent commands as asynchronous QMP commands, no?

Yup, that's the plan. Just mean that the balloon stats fall in the same
bucket and could (eventually) benefit from a more general solution. In
particular, with async QMP in place we could deprecate the polling approach
by having the request do basically what Luiz's patch does and calling
issuing the qmp completion instead of the event.

But I guess the more important question is getting the API right.
Something that allows us to basically turn query-balloon back on would
certainly be optimal... otherwise we'll add new QMP commands and deprecate
them fairly quickly. If avoiding that requires a polling mechanism then it
may be worth it in that sense.

I don't think that requires QOM though, just a stats_pending flags in
virtio-balloon and a periodic timer cb that sends a new request if it's
cleared. QOM might be nicer though.

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >>
> >>Regards,
> >>
> >>Anthony Liguori
> >>
> >>
> >
> >
> 

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Michael Roth]

On Fri, Feb 17, 2012 at 03:16:22PM -0200, Luiz Capitulino wrote:
> On Fri, 17 Feb 2012 10:55:41 -0600
> Anthony Liguori <aliguori@us.ibm.com> wrote:
> 
> > On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> > > This commit adds a QMP API for the guest provided memory statistics
> > > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> > >
> > > The approach taken by the original commit
> > > (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> > > query-balloon command. It introduced a severe bug though: query-balloon
> > > would hang if the guest didn't respond.
> > >
> > > The approach taken by this commit is asynchronous and thus avoids
> > > any QMP hangs.
> > >
> > > First, a client has to issue the balloon-get-memory-stats command.
> > > That command gets the process started by only sending a request to
> > > the guest, it doesn't block. When the memory stats are made available
> > > by the guest, they are returned to the client as an QMP event.
> > >
> > > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > 
> > Do we need this to be stable in 1.1?
> 
> Well, this is disabled for a long time already and libvirt needs it, so I'd
> say asap, but isn't it possible to implement this with current QOM?
> 
> > We can do this pretty nicely through QOM.  We can have a polling property in the 
> > virtio-balloon driver, that when set, will enable the virtio-balloon device to 
> > poll the guest for statistics.
> >
> > 
> > We can also have properties for each of the memory statistics and a timestamp 
> > for when the last update was.
> > 
> > I think this is a friendlier approach for clients, and a cleaner approach from a 
> > QEMU perspective.
> 
> I agree it's friendlier, but is it a good idea to keep polling the guest for
> something that may never be needed by a mngt app (real question)?

Probably not, but then again you'd only need like 1-second granularity.
Also, I think we can do away with the polling once async QMP is in
place, so we wouldn't be stuck with it necessarilly.

> 
> We could allow the mngt app to do the polling by adding a query-balloon-stats
> command (instead of balloon-get-memory-stats & event). This command could
> return the latest available stats if any (with a timestamp) and query the
> guest for new stats.

The downside there is you could read some really stale data that way,
to the point where any app that really cared would likely throw out the first
result.

> 
> > 
> > There's nothing generic about this functionality.  It's extremely specific to 
> > virtio-balloon.  We just lacked ways to expose device specific function pre-QOM.
> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> 
> 

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Luiz Capitulino]

On Fri, 17 Feb 2012 15:51:33 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Fri, Feb 17, 2012 at 03:16:22PM -0200, Luiz Capitulino wrote:
> > On Fri, 17 Feb 2012 10:55:41 -0600
> > Anthony Liguori <aliguori@us.ibm.com> wrote:
> > 
> > > On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> > > > This commit adds a QMP API for the guest provided memory statistics
> > > > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> > > >
> > > > The approach taken by the original commit
> > > > (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> > > > query-balloon command. It introduced a severe bug though: query-balloon
> > > > would hang if the guest didn't respond.
> > > >
> > > > The approach taken by this commit is asynchronous and thus avoids
> > > > any QMP hangs.
> > > >
> > > > First, a client has to issue the balloon-get-memory-stats command.
> > > > That command gets the process started by only sending a request to
> > > > the guest, it doesn't block. When the memory stats are made available
> > > > by the guest, they are returned to the client as an QMP event.
> > > >
> > > > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > > 
> > > Do we need this to be stable in 1.1?
> > 
> > Well, this is disabled for a long time already and libvirt needs it, so I'd
> > say asap, but isn't it possible to implement this with current QOM?
> > 
> > > We can do this pretty nicely through QOM.  We can have a polling property in the 
> > > virtio-balloon driver, that when set, will enable the virtio-balloon device to 
> > > poll the guest for statistics.
> > >
> > > 
> > > We can also have properties for each of the memory statistics and a timestamp 
> > > for when the last update was.
> > > 
> > > I think this is a friendlier approach for clients, and a cleaner approach from a 
> > > QEMU perspective.
> > 
> > I agree it's friendlier, but is it a good idea to keep polling the guest for
> > something that may never be needed by a mngt app (real question)?
> 
> Probably not, but then again you'd only need like 1-second granularity.

I've talked with Anthony by irc about the implementation details of this and
it will be possible to enable/disable the polling, so this is not an issue
anymore.

> Also, I think we can do away with the polling once async QMP is in
> place, so we wouldn't be stuck with it necessarilly.

This is what this series does :) I don't think it's necessary to wait for
async support, we're accepting ad-hoc async mechanisms for other commands and
could use one here too if needed.

> > We could allow the mngt app to do the polling by adding a query-balloon-stats
> > command (instead of balloon-get-memory-stats & event). This command could
> > return the latest available stats if any (with a timestamp) and query the
> > guest for new stats.
> 
> The downside there is you could read some really stale data that way,
> to the point where any app that really cared would likely throw out the first
> result.

Having stale data will be possible with any timer based polling...

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Michael Roth]

On Wed, Feb 22, 2012 at 10:48:13AM -0200, Luiz Capitulino wrote:
> On Fri, 17 Feb 2012 15:51:33 -0600
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Fri, Feb 17, 2012 at 03:16:22PM -0200, Luiz Capitulino wrote:
> > > On Fri, 17 Feb 2012 10:55:41 -0600
> > > Anthony Liguori <aliguori@us.ibm.com> wrote:
> > > 
> > > > On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> > > > > This commit adds a QMP API for the guest provided memory statistics
> > > > > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> > > > >
> > > > > The approach taken by the original commit
> > > > > (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> > > > > query-balloon command. It introduced a severe bug though: query-balloon
> > > > > would hang if the guest didn't respond.
> > > > >
> > > > > The approach taken by this commit is asynchronous and thus avoids
> > > > > any QMP hangs.
> > > > >
> > > > > First, a client has to issue the balloon-get-memory-stats command.
> > > > > That command gets the process started by only sending a request to
> > > > > the guest, it doesn't block. When the memory stats are made available
> > > > > by the guest, they are returned to the client as an QMP event.
> > > > >
> > > > > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > > > 
> > > > Do we need this to be stable in 1.1?
> > > 
> > > Well, this is disabled for a long time already and libvirt needs it, so I'd
> > > say asap, but isn't it possible to implement this with current QOM?
> > > 
> > > > We can do this pretty nicely through QOM.  We can have a polling property in the 
> > > > virtio-balloon driver, that when set, will enable the virtio-balloon device to 
> > > > poll the guest for statistics.
> > > >
> > > > 
> > > > We can also have properties for each of the memory statistics and a timestamp 
> > > > for when the last update was.
> > > > 
> > > > I think this is a friendlier approach for clients, and a cleaner approach from a 
> > > > QEMU perspective.
> > > 
> > > I agree it's friendlier, but is it a good idea to keep polling the guest for
> > > something that may never be needed by a mngt app (real question)?
> > 
> > Probably not, but then again you'd only need like 1-second granularity.
> 
> I've talked with Anthony by irc about the implementation details of this and
> it will be possible to enable/disable the polling, so this is not an issue
> anymore.
> 
> > Also, I think we can do away with the polling once async QMP is in
> > place, so we wouldn't be stuck with it necessarilly.
> 
> This is what this series does :) I don't think it's necessary to wait for
> async support, we're accepting ad-hoc async mechanisms for other commands and
> could use one here too if needed.

I don't mind the ad-hoc implementation details, I just think in this
case it's leaking out into our APIs. With proper async QMP in place we
just re-enable query-balloon and existing synchronous QMP clients and
libvirt interfaces Just Work (albeit with potential for a "timed-out"
response). There's no need for a specialized balloon-get-memory-stats command
at all.

And if they want stats asynchronously, proper async QMP does that as well:
they just need to make their QMP client async-aware (basically, don't wait
around for a response, and tag the query-balloon request so you can
match the response to the query).

So balloon-get-memory-stats is already on the path to being deprecated.

We should instead focus on just re-enabling query-balloon, and if that can be
achieved with this approach, then I'm all for it, but I think we all seem to
agree that a timer-based mechanism would be needed instead.

> 
> > > We could allow the mngt app to do the polling by adding a query-balloon-stats
> > > command (instead of balloon-get-memory-stats & event). This command could
> > > return the latest available stats if any (with a timestamp) and query the
> > > guest for new stats.
> > 
> > The downside there is you could read some really stale data that way,
> > to the point where any app that really cared would likely throw out the first
> > result.
> 
> Having stale data will be possible with any timer based polling...
> 

Sorry, thought you were suggesting we do "lazy" polling where we only
queue up a balloon request after a query-balloon has been issued, in
which case the age of the response is unbounded... well, from a QMP standpoint,
I guess that *is* what we'd be doing, and we'd just be telling management tools
to add a wrapper around the interface as a work-around, which seems
suggests it's not the right interface.

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Luiz Capitulino]

On Wed, 22 Feb 2012 09:05:22 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Wed, Feb 22, 2012 at 10:48:13AM -0200, Luiz Capitulino wrote:
> > On Fri, 17 Feb 2012 15:51:33 -0600
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > On Fri, Feb 17, 2012 at 03:16:22PM -0200, Luiz Capitulino wrote:
> > > > On Fri, 17 Feb 2012 10:55:41 -0600
> > > > Anthony Liguori <aliguori@us.ibm.com> wrote:
> > > > 
> > > > > On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> > > > > > This commit adds a QMP API for the guest provided memory statistics
> > > > > > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> > > > > >
> > > > > > The approach taken by the original commit
> > > > > > (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> > > > > > query-balloon command. It introduced a severe bug though: query-balloon
> > > > > > would hang if the guest didn't respond.
> > > > > >
> > > > > > The approach taken by this commit is asynchronous and thus avoids
> > > > > > any QMP hangs.
> > > > > >
> > > > > > First, a client has to issue the balloon-get-memory-stats command.
> > > > > > That command gets the process started by only sending a request to
> > > > > > the guest, it doesn't block. When the memory stats are made available
> > > > > > by the guest, they are returned to the client as an QMP event.
> > > > > >
> > > > > > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > > > > 
> > > > > Do we need this to be stable in 1.1?
> > > > 
> > > > Well, this is disabled for a long time already and libvirt needs it, so I'd
> > > > say asap, but isn't it possible to implement this with current QOM?
> > > > 
> > > > > We can do this pretty nicely through QOM.  We can have a polling property in the 
> > > > > virtio-balloon driver, that when set, will enable the virtio-balloon device to 
> > > > > poll the guest for statistics.
> > > > >
> > > > > 
> > > > > We can also have properties for each of the memory statistics and a timestamp 
> > > > > for when the last update was.
> > > > > 
> > > > > I think this is a friendlier approach for clients, and a cleaner approach from a 
> > > > > QEMU perspective.
> > > > 
> > > > I agree it's friendlier, but is it a good idea to keep polling the guest for
> > > > something that may never be needed by a mngt app (real question)?
> > > 
> > > Probably not, but then again you'd only need like 1-second granularity.
> > 
> > I've talked with Anthony by irc about the implementation details of this and
> > it will be possible to enable/disable the polling, so this is not an issue
> > anymore.
> > 
> > > Also, I think we can do away with the polling once async QMP is in
> > > place, so we wouldn't be stuck with it necessarilly.
> > 
> > This is what this series does :) I don't think it's necessary to wait for
> > async support, we're accepting ad-hoc async mechanisms for other commands and
> > could use one here too if needed.
> 
> I don't mind the ad-hoc implementation details, I just think in this
> case it's leaking out into our APIs. With proper async QMP in place we
> just re-enable query-balloon and existing synchronous QMP clients and
> libvirt interfaces Just Work (albeit with potential for a "timed-out"
> response). There's no need for a specialized balloon-get-memory-stats command
> at all.

It's not possible to re-use query-balloon for this, making a synchronous
command asynchronous is an incompatible change. That was exactly the problem
we had that forced us to disable this feature (and that's why a new command
is required, be it balloon-get-memory-stats or a QOM device property).

> And if they want stats asynchronously, proper async QMP does that as well:
> they just need to make their QMP client async-aware (basically, don't wait
> around for a response, and tag the query-balloon request so you can
> match the response to the query).
> 
> So balloon-get-memory-stats is already on the path to being deprecated.

Any command implementing its async schema is going to be deprecated when we
get proper async support. That's not a problem per se. I mean, a much worse
problem is to delay features & functionality because we don't have the perfect
means to implement them.

But that's a dead discussion I guess, as I already agreed on implementing
this as a device property.

> We should instead focus on just re-enabling query-balloon, and if that can be
> achieved with this approach, then I'm all for it, but I think we all seem to
> agree that a timer-based mechanism would be needed instead.
> 
> > 
> > > > We could allow the mngt app to do the polling by adding a query-balloon-stats
> > > > command (instead of balloon-get-memory-stats & event). This command could
> > > > return the latest available stats if any (with a timestamp) and query the
> > > > guest for new stats.
> > > 
> > > The downside there is you could read some really stale data that way,
> > > to the point where any app that really cared would likely throw out the first
> > > result.
> > 
> > Having stale data will be possible with any timer based polling...
> > 
> 
> Sorry, thought you were suggesting we do "lazy" polling where we only
> queue up a balloon request after a query-balloon has been issued, in
> which case the age of the response is unbounded... well, from a QMP standpoint,
> I guess that *is* what we'd be doing, and we'd just be telling management tools
> to add a wrapper around the interface as a work-around, which seems
> suggests it's not the right interface.
> 

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Michael Roth]

On Wed, Feb 22, 2012 at 02:09:35PM -0200, Luiz Capitulino wrote:
> On Wed, 22 Feb 2012 09:05:22 -0600
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Feb 22, 2012 at 10:48:13AM -0200, Luiz Capitulino wrote:
> > > On Fri, 17 Feb 2012 15:51:33 -0600
> > > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Fri, Feb 17, 2012 at 03:16:22PM -0200, Luiz Capitulino wrote:
> > > > > On Fri, 17 Feb 2012 10:55:41 -0600
> > > > > Anthony Liguori <aliguori@us.ibm.com> wrote:
> > > > > 
> > > > > > On 02/08/2012 02:30 PM, Luiz Capitulino wrote:
> > > > > > > This commit adds a QMP API for the guest provided memory statistics
> > > > > > > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a).
> > > > > > >
> > > > > > > The approach taken by the original commit
> > > > > > > (625a5befc2e3200b396594f002218d235e375da5) was to extend the
> > > > > > > query-balloon command. It introduced a severe bug though: query-balloon
> > > > > > > would hang if the guest didn't respond.
> > > > > > >
> > > > > > > The approach taken by this commit is asynchronous and thus avoids
> > > > > > > any QMP hangs.
> > > > > > >
> > > > > > > First, a client has to issue the balloon-get-memory-stats command.
> > > > > > > That command gets the process started by only sending a request to
> > > > > > > the guest, it doesn't block. When the memory stats are made available
> > > > > > > by the guest, they are returned to the client as an QMP event.
> > > > > > >
> > > > > > > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > > > > > 
> > > > > > Do we need this to be stable in 1.1?
> > > > > 
> > > > > Well, this is disabled for a long time already and libvirt needs it, so I'd
> > > > > say asap, but isn't it possible to implement this with current QOM?
> > > > > 
> > > > > > We can do this pretty nicely through QOM.  We can have a polling property in the 
> > > > > > virtio-balloon driver, that when set, will enable the virtio-balloon device to 
> > > > > > poll the guest for statistics.
> > > > > >
> > > > > > 
> > > > > > We can also have properties for each of the memory statistics and a timestamp 
> > > > > > for when the last update was.
> > > > > > 
> > > > > > I think this is a friendlier approach for clients, and a cleaner approach from a 
> > > > > > QEMU perspective.
> > > > > 
> > > > > I agree it's friendlier, but is it a good idea to keep polling the guest for
> > > > > something that may never be needed by a mngt app (real question)?
> > > > 
> > > > Probably not, but then again you'd only need like 1-second granularity.
> > > 
> > > I've talked with Anthony by irc about the implementation details of this and
> > > it will be possible to enable/disable the polling, so this is not an issue
> > > anymore.
> > > 
> > > > Also, I think we can do away with the polling once async QMP is in
> > > > place, so we wouldn't be stuck with it necessarilly.
> > > 
> > > This is what this series does :) I don't think it's necessary to wait for
> > > async support, we're accepting ad-hoc async mechanisms for other commands and
> > > could use one here too if needed.
> > 
> > I don't mind the ad-hoc implementation details, I just think in this
> > case it's leaking out into our APIs. With proper async QMP in place we
> > just re-enable query-balloon and existing synchronous QMP clients and
> > libvirt interfaces Just Work (albeit with potential for a "timed-out"
> > response). There's no need for a specialized balloon-get-memory-stats command
> > at all.
> 
> It's not possible to re-use query-balloon for this, making a synchronous
> command asynchronous is an incompatible change. That was exactly the problem
> we had that forced us to disable this feature (and that's why a new command
> is required, be it balloon-get-memory-stats or a QOM device property).

But I'm not suggesting we make query-balloon asynchronous.

I'm suggesting be re-enable it as a synchronous interface by having it
immediately return the latest-available results from a timer-driven
query mechanism that tucks away the query results, such as the one Anthony
suggested.

In the future we can drop the polling backend and switch the backend over to
using proper async QMP that queries on demand. The change to current users
would be transparent (and it would also expose an *optional* async
front-end that would also handle the "send me an event when it's ready"
use-case, so balloon-get-memory-stats would never be needed).

> 
> > And if they want stats asynchronously, proper async QMP does that as well:
> > they just need to make their QMP client async-aware (basically, don't wait
> > around for a response, and tag the query-balloon request so you can
> > match the response to the query).
> > 
> > So balloon-get-memory-stats is already on the path to being deprecated.
> 
> Any command implementing its async schema is going to be deprecated when we
> get proper async support. That's not a problem per se. I mean, a much worse
> problem is to delay features & functionality because we don't have the perfect
> means to implement them.
> 
> But that's a dead discussion I guess, as I already agreed on implementing
> this as a device property.

Along with timer-based refresh of the properties? If so I don't understand why we
can't just have query-balloon simply return those properties when it's called? I
thought that's what Anthony was driving at with the timer-based
approach.

> 
> > We should instead focus on just re-enabling query-balloon, and if that can be
> > achieved with this approach, then I'm all for it, but I think we all seem to
> > agree that a timer-based mechanism would be needed instead.
> > 
> > > 
> > > > > We could allow the mngt app to do the polling by adding a query-balloon-stats
> > > > > command (instead of balloon-get-memory-stats & event). This command could
> > > > > return the latest available stats if any (with a timestamp) and query the
> > > > > guest for new stats.
> > > > 
> > > > The downside there is you could read some really stale data that way,
> > > > to the point where any app that really cared would likely throw out the first
> > > > result.
> > > 
> > > Having stale data will be possible with any timer based polling...
> > > 
> > 
> > Sorry, thought you were suggesting we do "lazy" polling where we only
> > queue up a balloon request after a query-balloon has been issued, in
> > which case the age of the response is unbounded... well, from a QMP standpoint,
> > I guess that *is* what we'd be doing, and we'd just be telling management tools
> > to add a wrapper around the interface as a work-around, which seems
> > suggests it's not the right interface.
> > 
> 

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Luiz Capitulino]

On Wed, 22 Feb 2012 10:54:45 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> But I'm not suggesting we make query-balloon asynchronous.
> 
> I'm suggesting be re-enable it as a synchronous interface by having it
> immediately return the latest-available results from a timer-driven
> query mechanism that tucks away the query results, such as the one Anthony
> suggested.

I'm not sure I like the semantics, as query-balloon would have some fields
that actually depends on a timer based polling being enabled somewhere else,
while others fields ('actual') would always be there.

> > But that's a dead discussion I guess, as I already agreed on implementing
> > this as a device property.
> 
> Along with timer-based refresh of the properties? If so I don't understand why we
> can't just have query-balloon simply return those properties when it's called? I
> thought that's what Anthony was driving at with the timer-based
> approach.

What I had understood is to make each stats a dynamic device property, then
mngt apps would use qom-set/get on them.

Anthony, can you detail your suggestion please?

Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Michael Roth]

On Wed, Feb 22, 2012 at 05:44:37PM -0200, Luiz Capitulino wrote:
> On Wed, 22 Feb 2012 10:54:45 -0600
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > But I'm not suggesting we make query-balloon asynchronous.
> > 
> > I'm suggesting be re-enable it as a synchronous interface by having it
> > immediately return the latest-available results from a timer-driven
> > query mechanism that tucks away the query results, such as the one Anthony
> > suggested.
> 
> I'm not sure I like the semantics, as query-balloon would have some fields
> that actually depends on a timer based polling being enabled somewhere else,
> while others fields ('actual') would always be there.

I think that's more a side-effect of sticking stats unrelated to
ballooning in query-balloon in the first place... but yah, maybe it
makes the interfaces not worth reviving.

> 
> > > But that's a dead discussion I guess, as I already agreed on implementing
> > > this as a device property.
> > 
> > Along with timer-based refresh of the properties? If so I don't understand why we
> > can't just have query-balloon simply return those properties when it's called? I
> > thought that's what Anthony was driving at with the timer-based
> > approach.
> 
> What I had understood is to make each stats a dynamic device property, then
> mngt apps would use qom-set/get on them.

Ahhhh, yah... you're probably right. And we get that for free by tieing
the data to properties, which I guess is where the significance of QOM
was wrt to the polling approach (since we could actually stick the data
anywhere if we did something like query-balloon).

I don't really have any objection there, my main concern was that we
were gonna be adding something *beyond* qom-get or query-balloon, but if
that's not the case, carry on :)

> 
> Anthony, can you detail your suggestion please?
>