[Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command

[Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command

[Luiz Capitulino]

Long ago, commit 625a5be added the guest provided memory statistics to
the query-balloon command. Unfortunately, it also introduced a severe
bug: query-balloon would hang if the guest didn't respond. This, in turn,
would also cause a hang in libvirt.

Because of that, we decided to disable the guest memory stats feature
(commit 11724ff).

As we decided to let commands implement ad-hoc async mechanisms until we
get a proper way to do it, I decided to try to re-enable that feature.

My idea is to have 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 05/05 for full details).

I'm not sure if that approach is good for libvirt though, so it would be
very helpful to get their input (Eric, I'm CC'ing you here, but feel free
to route this to someone else).

Another interesting point is that, there's another way of doing this and
it's using qemu-ga instead. That's, qemu-ga could read that information
from proc and return it. This is easier & simpler, as it doesn't involve
guest communication. We also could return a lot more information if needed.
The only disadvantage I can see is the dependency on qemu-ga...

 QMP/qmp-events.txt  |   28 ++++++++++++++++++++++++++++
 balloon.c           |   47 +++++++++++++++++++++++++++++++++++++----------
 balloon.h           |    7 ++++---
 hmp.c               |   25 +------------------------
 hw/virtio-balloon.c |   39 +++++++++++++++++++++++++++------------
 monitor.c           |    3 +++
 monitor.h           |    1 +
 qapi-schema.json    |   42 ++++++++++++++++++++++--------------------
 qmp-commands.hx     |    6 ++++++
 9 files changed, 129 insertions(+), 69 deletions(-)

[Qemu-devel] [PATCH 1/5] 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.rc0.dirty

[Qemu-devel] [PATCH 2/5] 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.rc0.dirty

[Qemu-devel] [PATCH 3/5] 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 8a77780..42f719a 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 44cf764..d9b1965 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.rc0.dirty

[Qemu-devel] [PATCH 4/5] balloon: Rename QEMUBalloonStatus to QEMUBalloonInfo

[Luiz Capitulino]

Next commit will introduce the QEMUBalloonStats type, which can
cause confusion with QEMUBalloonStatus. Also, QEMUBalloonInfo
matches better with the BalloonInfo type, where the current
balloon information is returned.

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.rc0.dirty

[Qemu-devel] [PATCH 5/5] 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. That approach introduced a severe bug though,
because 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. balloon-get-memory-stats doesn't block. When the memory
stats is made available by the guest, it's returned to the client
as an QMP event.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt  |   28 ++++++++++++++++++++++++++++
 balloon.c           |   31 +++++++++++++++++++++++++++++--
 balloon.h           |    4 +++-
 hw/virtio-balloon.c |   39 +++++++++++++++++++++++++++------------
 monitor.c           |    3 +++
 monitor.h           |    1 +
 qapi-schema.json    |   21 +++++++++++++++++++++
 qmp-commands.hx     |    6 ++++++
 8 files changed, 118 insertions(+), 15 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index af586ec..59a5334 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -1,6 +1,34 @@
                    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)
+- "memory-swapped-out": number of pages swapped out within the guest (json-int)
+- "major-page-faults": number of major page faults within the guest (json-int)
+- "minor-page-faults": number of minor page faults within the guest (json-int)
+- "memory-free": amount of memory (in bytes) free in the guest (json-int)
+- "memory-total": amount of memory (in bytes) visible to the guest (json-int)
+
+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 } }
+
+Note: The balloon-get-memory-stats command must be issued to tell the guest
+      to make memory statistics available.
+
 BLOCK_IO_ERROR
 --------------
 
diff --git a/balloon.c b/balloon.c
index b32b487..0f54d0c 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,28 @@ 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;
+}
+
+void qmp_balloon_get_memory_stats(Error **errp)
+{
+    if (kvm_enabled() && !kvm_has_sync_mmu()) {
+        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+        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..883d432 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>
@@ -104,8 +106,10 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
+    VirtIOBalloon *dev = s;
     VirtQueueElement *elem = &s->stats_vq_elem;
     VirtIOBalloonStat stat;
+    QObject *stats_obj;
     size_t offset = 0;
 
     if (!virtqueue_pop(vq, elem)) {
@@ -128,6 +132,22 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
             s->stats[tag] = val;
     }
     s->stats_vq_offset = offset;
+
+    stats_obj = qobject_from_jsonf("{ 'memory-swapped-in': %" PRId64 ", "
+                                   "'memory-swapped-out': %" PRId64 ", "
+                                   "'memory-free': %" PRId64 ", "
+                                   "'memory-total': %" PRId64 ", "
+                                   "'major-page-faults': %" PRId64 ", "
+                                   "'minor-page-faults': %" PRId64 " }",
+                                   dev->stats[VIRTIO_BALLOON_S_SWAP_IN],
+                                   dev->stats[VIRTIO_BALLOON_S_SWAP_OUT],
+                                   dev->stats[VIRTIO_BALLOON_S_MEMFREE],
+                                   dev->stats[VIRTIO_BALLOON_S_MEMTOT],
+                                   dev->stats[VIRTIO_BALLOON_S_MAJFLT],
+                                   dev->stats[VIRTIO_BALLOON_S_MINFLT]);
+
+    monitor_protocol_event(QEVENT_BALLOON_STATS, stats_obj);
+    qobject_decref(stats_obj);
 }
 
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
@@ -156,31 +176,25 @@ 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)) {
         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,7 +250,8 @@ 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;
diff --git a/monitor.c b/monitor.c
index 7334401..b3d5036 100644
--- a/monitor.c
+++ b/monitor.c
@@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_SPICE_DISCONNECTED:
             event_name = "SPICE_DISCONNECTED";
             break;
+        case QEVENT_BALLOON_STATS:
+            event_name = "BALLOON_MEMORY_STATS";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index cfa2f67..a9e7718 100644
--- a/monitor.h
+++ b/monitor.h
@@ -35,6 +35,7 @@ typedef enum MonitorEvent {
     QEVENT_SPICE_CONNECTED,
     QEVENT_SPICE_INITIALIZED,
     QEVENT_SPICE_DISCONNECTED,
+    QEVENT_BALLOON_STATS,
     QEVENT_MAX,
 } MonitorEvent;
 
diff --git a/qapi-schema.json b/qapi-schema.json
index d9b1965..e3b443b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1256,3 +1256,24 @@
 { 'command': 'qom-set',
   'data': { 'path': 'str', 'property': 'str', 'value': 'visitor' },
   'gen': 'no' }
+
+##
+# @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 7e3f4b9..7a80cda 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2027,3 +2027,9 @@ EQMP
 	.args_type  = "path:s,property:s",
 	.mhandler.cmd_new = qmp_qom_get,
     },
+
+    {
+        .name       = "balloon-get-memory-stats",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_balloon_get_memory_stats,
+    },
-- 
1.7.9.rc0.dirty

Re: [Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command

[Michael Roth]

On 01/19/2012 09:56 AM, Luiz Capitulino wrote:
> Long ago, commit 625a5be added the guest provided memory statistics to
> the query-balloon command. Unfortunately, it also introduced a severe
> bug: query-balloon would hang if the guest didn't respond. This, in turn,
> would also cause a hang in libvirt.
>
> Because of that, we decided to disable the guest memory stats feature
> (commit 11724ff).
>
> As we decided to let commands implement ad-hoc async mechanisms until we
> get a proper way to do it, I decided to try to re-enable that feature.
>
> My idea is to have 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 05/05 for full details).
>
> I'm not sure if that approach is good for libvirt though, so it would be
> very helpful to get their input (Eric, I'm CC'ing you here, but feel free
> to route this to someone else).
>
> Another interesting point is that, there's another way of doing this and
> it's using qemu-ga instead. That's, qemu-ga could read that information
> from proc and return it. This is easier&  simpler, as it doesn't involve
> guest communication. We also could return a lot more information if needed.
> The only disadvantage I can see is the dependency on qemu-ga...

A nice plus with this is we retroactively get support for a wide range 
of existing guest types that we wouldn't realistically expect to have a 
qemu-ga package, and I think there's even balloon stats for Windows 
guests as well. Event-based updates also lend themselves nicely to 
things like qemu-centric UI integration.

Memory stats is one of the proposed use-cases of qemu-ga, but I don't 
see these as necessarily being in conflict. We can extend the statistics 
collection quite a bit more with an agent approach, but it's nice to 
have some assurance that if you can balloon a guest, you can at the very 
least get at the bare minimum information you need to make reasonable 
decisions about ballooning.

>
>   QMP/qmp-events.txt  |   28 ++++++++++++++++++++++++++++
>   balloon.c           |   47 +++++++++++++++++++++++++++++++++++++----------
>   balloon.h           |    7 ++++---
>   hmp.c               |   25 +------------------------
>   hw/virtio-balloon.c |   39 +++++++++++++++++++++++++++------------
>   monitor.c           |    3 +++
>   monitor.h           |    1 +
>   qapi-schema.json    |   42 ++++++++++++++++++++++--------------------
>   qmp-commands.hx     |    6 ++++++
>   9 files changed, 129 insertions(+), 69 deletions(-)
>

Re: [Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command

[Luiz Capitulino]

On Thu, 19 Jan 2012 10:43:35 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 01/19/2012 09:56 AM, Luiz Capitulino wrote:
> > Long ago, commit 625a5be added the guest provided memory statistics to
> > the query-balloon command. Unfortunately, it also introduced a severe
> > bug: query-balloon would hang if the guest didn't respond. This, in turn,
> > would also cause a hang in libvirt.
> >
> > Because of that, we decided to disable the guest memory stats feature
> > (commit 11724ff).
> >
> > As we decided to let commands implement ad-hoc async mechanisms until we
> > get a proper way to do it, I decided to try to re-enable that feature.
> >
> > My idea is to have 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 05/05 for full details).
> >
> > I'm not sure if that approach is good for libvirt though, so it would be
> > very helpful to get their input (Eric, I'm CC'ing you here, but feel free
> > to route this to someone else).
> >
> > Another interesting point is that, there's another way of doing this and
> > it's using qemu-ga instead. That's, qemu-ga could read that information
> > from proc and return it. This is easier&  simpler, as it doesn't involve
> > guest communication. We also could return a lot more information if needed.
> > The only disadvantage I can see is the dependency on qemu-ga...
> 
> A nice plus with this is we retroactively get support for a wide range 
> of existing guest types that we wouldn't realistically expect to have a 
> qemu-ga package, and I think there's even balloon stats for Windows 
> guests as well. Event-based updates also lend themselves nicely to 
> things like qemu-centric UI integration.
> 
> Memory stats is one of the proposed use-cases of qemu-ga, but I don't 
> see these as necessarily being in conflict. We can extend the statistics 
> collection quite a bit more with an agent approach, but it's nice to 
> have some assurance that if you can balloon a guest, you can at the very 
> least get at the bare minimum information you need to make reasonable 
> decisions about ballooning.

That makes sense, we could have both.

> 
> >
> >   QMP/qmp-events.txt  |   28 ++++++++++++++++++++++++++++
> >   balloon.c           |   47 +++++++++++++++++++++++++++++++++++++----------
> >   balloon.h           |    7 ++++---
> >   hmp.c               |   25 +------------------------
> >   hw/virtio-balloon.c |   39 +++++++++++++++++++++++++++------------
> >   monitor.c           |    3 +++
> >   monitor.h           |    1 +
> >   qapi-schema.json    |   42 ++++++++++++++++++++++--------------------
> >   qmp-commands.hx     |    6 ++++++
> >   9 files changed, 129 insertions(+), 69 deletions(-)
> >
> 

Re: [Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3424 bytes --]

On 01/19/2012 08:56 AM, Luiz Capitulino wrote:
> Long ago, commit 625a5be added the guest provided memory statistics to
> the query-balloon command. Unfortunately, it also introduced a severe
> bug: query-balloon would hang if the guest didn't respond. This, in turn,
> would also cause a hang in libvirt.
> 
> Because of that, we decided to disable the guest memory stats feature
> (commit 11724ff).
> 
> As we decided to let commands implement ad-hoc async mechanisms until we
> get a proper way to do it, I decided to try to re-enable that feature.
> 
> My idea is to have 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 05/05 for full details).
> 
> I'm not sure if that approach is good for libvirt though, so it would be
> very helpful to get their input (Eric, I'm CC'ing you here, but feel free
> to route this to someone else).

[I went ahead and cc'd the libvirt list]

Yes, libvirt can live with this approach.  And having this in parallel
to a qemu-ga verb is nice, since, as it was pointed out, this would
allow interaction with guests that have a balloon device but not a guest
agent.

You may want to read this thread [1], for thoughts on the impact of
making another existing blocking command be extended into one that
starts an async event and ends when an event is raised; libvirt can
expose both a blocking and an asynchronous implementation to the user on
top of the qemu model being just asynchronous.
[1] https://www.redhat.com/archives/libvir-list/2012-January/msg00562.html

Thinking aloud - do we need a means to poll the state of the
balloon-stat query?  On the one hand, if libvirtd issues the start
command, then gets stopped, then the event occurs, then libvirtd is
restarted, then libvirt won't know that the event was missed.  On the
other hand, since this involves guest interaction, libvirt already has
to assume that the guest may be malicious and refuse to report stats
and/or report invalid stats, so libvirt would already have to be
prepared to give up if no event has arrived in a fixed amount of time,
and that also means that restarting libvirtd can just ignore any balloon
query that was in flight before the restart.

So I guess I'm okay with just a start and an event, with no poll of the
last-known guest response.  But it does mean that qemu has to gracefully
handle if libvirt makes two start requests in a row without any
intervening events, and conversely that libvirt has to be prepared for
an event that happens even when libvirt doesn't remember triggering the
start command.

> Another interesting point is that, there's another way of doing this and
> it's using qemu-ga instead. That's, qemu-ga could read that information
> from proc and return it. This is easier & simpler, as it doesn't involve
> guest communication. We also could return a lot more information if needed.
> The only disadvantage I can see is the dependency on qemu-ga...

Most likely, we would want to teach libvirt to use both methods, and
give the choice to the user on which approach to use when the guest
supports both.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command

[Luiz Capitulino]

On Thu, 19 Jan 2012 10:15:55 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 01/19/2012 08:56 AM, Luiz Capitulino wrote:
> > Long ago, commit 625a5be added the guest provided memory statistics to
> > the query-balloon command. Unfortunately, it also introduced a severe
> > bug: query-balloon would hang if the guest didn't respond. This, in turn,
> > would also cause a hang in libvirt.
> > 
> > Because of that, we decided to disable the guest memory stats feature
> > (commit 11724ff).
> > 
> > As we decided to let commands implement ad-hoc async mechanisms until we
> > get a proper way to do it, I decided to try to re-enable that feature.
> > 
> > My idea is to have 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 05/05 for full details).
> > 
> > I'm not sure if that approach is good for libvirt though, so it would be
> > very helpful to get their input (Eric, I'm CC'ing you here, but feel free
> > to route this to someone else).
> 
> [I went ahead and cc'd the libvirt list]
> 
> Yes, libvirt can live with this approach.  And having this in parallel
> to a qemu-ga verb is nice, since, as it was pointed out, this would
> allow interaction with guests that have a balloon device but not a guest
> agent.
> 
> You may want to read this thread [1], for thoughts on the impact of
> making another existing blocking command be extended into one that
> starts an async event and ends when an event is raised; libvirt can
> expose both a blocking and an asynchronous implementation to the user on
> top of the qemu model being just asynchronous.
> [1] https://www.redhat.com/archives/libvir-list/2012-January/msg00562.html
> 
> Thinking aloud - do we need a means to poll the state of the
> balloon-stat query?

We could have a query-balloon-memory-stats command that returns the last
available stats (or none, if ballon-get-memory-stats wasn't issued), and
I think that it would be better to move the stats info from the event to
the query command too, this way the event would just signal that the stats
info are available.

I find that approach a bit more complicated though.

>  On the one hand, if libvirtd issues the start
> command, then gets stopped, then the event occurs, then libvirtd is
> restarted, then libvirt won't know that the event was missed.  On the
> other hand, since this involves guest interaction, libvirt already has
> to assume that the guest may be malicious and refuse to report stats
> and/or report invalid stats, so libvirt would already have to be
> prepared to give up if no event has arrived in a fixed amount of time,
> and that also means that restarting libvirtd can just ignore any balloon
> query that was in flight before the restart.

Yes, there's no guarantee the event will be ever sent. If it doesn't
arrive after a fixed amount of time, the best thing to do is to issue
the start command again.

> So I guess I'm okay with just a start and an event, with no poll of the
> last-known guest response.  But it does mean that qemu has to gracefully
> handle if libvirt makes two start requests in a row without any
> intervening events, and conversely that libvirt has to be prepared for
> an event that happens even when libvirt doesn't remember triggering the
> start command.

There could be intervening events. Everything can happen between the
start command and the event (I/O Error, VM stop, etc). Libvirt has to be
prepared for that.

> 
> > Another interesting point is that, there's another way of doing this and
> > it's using qemu-ga instead. That's, qemu-ga could read that information
> > from proc and return it. This is easier & simpler, as it doesn't involve
> > guest communication. We also could return a lot more information if needed.
> > The only disadvantage I can see is the dependency on qemu-ga...
> 
> Most likely, we would want to teach libvirt to use both methods, and
> give the choice to the user on which approach to use when the guest
> supports both.
> 

Re: [Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command

[Adam Litke]

On Thu, Jan 19, 2012 at 01:56:26PM -0200, Luiz Capitulino wrote:
> Long ago, commit 625a5be added the guest provided memory statistics to
> the query-balloon command. Unfortunately, it also introduced a severe
> bug: query-balloon would hang if the guest didn't respond. This, in turn,
> would also cause a hang in libvirt.
> 
> Because of that, we decided to disable the guest memory stats feature
> (commit 11724ff).
> 
> As we decided to let commands implement ad-hoc async mechanisms until we
> get a proper way to do it, I decided to try to re-enable that feature.
> 
> My idea is to have 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 05/05 for full details).
> 
> I'm not sure if that approach is good for libvirt though, so it would be
> very helpful to get their input (Eric, I'm CC'ing you here, but feel free
> to route this to someone else).
> 
> Another interesting point is that, there's another way of doing this and
> it's using qemu-ga instead. That's, qemu-ga could read that information
> from proc and return it. This is easier & simpler, as it doesn't involve
> guest communication. We also could return a lot more information if needed.
> The only disadvantage I can see is the dependency on qemu-ga...
> 
>  QMP/qmp-events.txt  |   28 ++++++++++++++++++++++++++++
>  balloon.c           |   47 +++++++++++++++++++++++++++++++++++++----------
>  balloon.h           |    7 ++++---
>  hmp.c               |   25 +------------------------
>  hw/virtio-balloon.c |   39 +++++++++++++++++++++++++++------------
>  monitor.c           |    3 +++
>  monitor.h           |    1 +
>  qapi-schema.json    |   42 ++++++++++++++++++++++--------------------
>  qmp-commands.hx     |    6 ++++++
>  9 files changed, 129 insertions(+), 69 deletions(-)
> 

The patch series looks good.  Thanks for making the improvements.  Once it is
upstream I can help out with the libvirt pieces.  I tested it with a Fedora-15
VM and it worked out of the box :)  I just have one small nit.  Due to the way
that the virtio stats queue works, the guest emits a stats event with bogus
values when the balloon driver initializes (which gives the host control of the
channel).  Your patches emit this initial event (which contains undefined
values).  In my old code, we kept a boolean flag in the ballon device to record
if stats have been requested and only if that was set would we raise the event.
Without this, the guest can spam the host with an unlimited number of bogus
events.

Tested-by: Adam Litke <agl@us.ibm.com>

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

Re: [Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command

[Luiz Capitulino]

On Thu, 19 Jan 2012 15:50:06 -0600
Adam Litke <agl@us.ibm.com> wrote:

> On Thu, Jan 19, 2012 at 01:56:26PM -0200, Luiz Capitulino wrote:
> > Long ago, commit 625a5be added the guest provided memory statistics to
> > the query-balloon command. Unfortunately, it also introduced a severe
> > bug: query-balloon would hang if the guest didn't respond. This, in turn,
> > would also cause a hang in libvirt.
> > 
> > Because of that, we decided to disable the guest memory stats feature
> > (commit 11724ff).
> > 
> > As we decided to let commands implement ad-hoc async mechanisms until we
> > get a proper way to do it, I decided to try to re-enable that feature.
> > 
> > My idea is to have 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 05/05 for full details).
> > 
> > I'm not sure if that approach is good for libvirt though, so it would be
> > very helpful to get their input (Eric, I'm CC'ing you here, but feel free
> > to route this to someone else).
> > 
> > Another interesting point is that, there's another way of doing this and
> > it's using qemu-ga instead. That's, qemu-ga could read that information
> > from proc and return it. This is easier & simpler, as it doesn't involve
> > guest communication. We also could return a lot more information if needed.
> > The only disadvantage I can see is the dependency on qemu-ga...
> > 
> >  QMP/qmp-events.txt  |   28 ++++++++++++++++++++++++++++
> >  balloon.c           |   47 +++++++++++++++++++++++++++++++++++++----------
> >  balloon.h           |    7 ++++---
> >  hmp.c               |   25 +------------------------
> >  hw/virtio-balloon.c |   39 +++++++++++++++++++++++++++------------
> >  monitor.c           |    3 +++
> >  monitor.h           |    1 +
> >  qapi-schema.json    |   42 ++++++++++++++++++++++--------------------
> >  qmp-commands.hx     |    6 ++++++
> >  9 files changed, 129 insertions(+), 69 deletions(-)
> > 
> 
> The patch series looks good.  Thanks for making the improvements.  Once it is
> upstream I can help out with the libvirt pieces.  I tested it with a Fedora-15
> VM and it worked out of the box :)  I just have one small nit.  Due to the way
> that the virtio stats queue works, the guest emits a stats event with bogus
> values when the balloon driver initializes (which gives the host control of the
> channel).  Your patches emit this initial event (which contains undefined
> values).  In my old code, we kept a boolean flag in the ballon device to record
> if stats have been requested and only if that was set would we raise the event.
> Without this, the guest can spam the host with an unlimited number of bogus
> events.

Yeah, I've seen this too. I'll fix it. We also need migration and HMP
support. Although the latter seems difficult as we don't have a way for
qemu subsystems to wait for events yet.

> Tested-by: Adam Litke <agl@us.ibm.com>

Thanks!

>