[Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

This new try to re-enable the virtio-balloon driver stats uses QOM properties
via a polling mechanism as suggested by Anthony here:

 http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02390.html

o Changes from the rfc

 - avoid balloon_stats_poll_cb() and virtio_balloon_handle_output()
   running in parallel
 - small renames and re-writes for better readability
 - update documentation

Luiz Capitulino (3):
  virtio-balloon: drop old stats code
  virtio-balloon: re-enable balloon stats
  docs: document virtio-balloon stats

 docs/virtio-balloon-stats.txt |  87 +++++++++++++++++++
 hw/virtio-balloon.c           | 189 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 252 insertions(+), 24 deletions(-)
 create mode 100644 docs/virtio-balloon-stats.txt

-- 
1.8.0

[Qemu-devel] [PATCH 1/3] virtio-balloon: drop old stats code

[Luiz Capitulino]

Next commit will re-enable balloon stats with a different interface, but
this old code conflicts with it. Drop it.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/virtio-balloon.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index dd1a650..4398025 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -164,28 +164,6 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
 static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
 {
     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);
-
     info->actual = ram_size - ((uint64_t) dev->actual <<
                                VIRTIO_BALLOON_PFN_SHIFT);
 }
-- 
1.8.0

[Qemu-devel] [PATCH 2/3] virtio-balloon: re-enable balloon stats

[Luiz Capitulino]

The statistics are now available through device properties via a
polling mechanism. First a client has to enable polling, then it
can query each stat individually.

The following control properties are introduced:

 o stats-polling-interval: a value greater than zero enables polling
   in the specified interval (in seconds). When value equals zero,
   polling is disabled. If polling is already enabled and a value
   greater than zero is written, the polling interval time is changed

 o stats-last-update: last stats update timestamp, in seconds.

The following stats properties are introduced, all values are in bytes:

 o stat-swap-in
 o stat-swap-out
 o stat-major-faults
 o stat-minor-faults
 o stat-free-memory
 o stat-total-memory

Please, refer to the documentation introduced by the next commit for
more information and examples.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/virtio-balloon.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 165 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 4398025..4bab2ae 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 "qemu-timer.h"
+#include "qapi/qapi-visit-core.h"
 
 #if defined(__linux__)
 #include <sys/mman.h>
@@ -36,6 +38,9 @@ typedef struct VirtIOBalloon
     uint64_t stats[VIRTIO_BALLOON_S_NR];
     VirtQueueElement stats_vq_elem;
     size_t stats_vq_offset;
+    QEMUTimer *stats_timer;
+    int64_t stats_last_update;
+    int64_t stats_poll_interval;
     DeviceState *qdev;
 } VirtIOBalloon;
 
@@ -53,6 +58,16 @@ static void balloon_page(void *addr, int deflate)
 #endif
 }
 
+static const char *balloon_stat_names[] = {
+   [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in", 
+   [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out",
+   [VIRTIO_BALLOON_S_MAJFLT] = "stat-major-faults",
+   [VIRTIO_BALLOON_S_MINFLT] = "stat-minor-faults",
+   [VIRTIO_BALLOON_S_MEMFREE] = "stat-free-memory",
+   [VIRTIO_BALLOON_S_MEMTOT] = "stat-total-memory",
+   [VIRTIO_BALLOON_S_NR] = NULL
+};
+
 /*
  * reset_stats - Mark all items in the stats array as unset
  *
@@ -67,6 +82,127 @@ static inline void reset_stats(VirtIOBalloon *dev)
     for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
 }
 
+static bool balloon_stats_supported(const VirtIOBalloon *s)
+{
+    return s->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ);
+}
+
+static bool balloon_stats_enabled(const VirtIOBalloon *s)
+{
+    return s->stats_poll_interval > 0;
+}
+
+static void balloon_stats_destroy_timer(VirtIOBalloon *s)
+{
+    if (balloon_stats_enabled(s)) {
+        qemu_del_timer(s->stats_timer);
+        qemu_free_timer(s->stats_timer);
+        s->stats_timer = NULL;
+        s->stats_poll_interval = 0;
+    }
+}
+
+static void balloon_stats_change_timer(VirtIOBalloon *s, int secs)
+{
+    qemu_mod_timer(s->stats_timer, qemu_get_clock_ms(vm_clock) + secs * 1000);
+}
+
+static void balloon_stats_poll_cb(void *opaque)
+{
+    VirtIOBalloon *s = opaque;
+
+    virtqueue_push(s->svq, &s->stats_vq_elem, s->stats_vq_offset);
+    virtio_notify(&s->vdev, s->svq);
+}
+
+static void balloon_stats_get_last_update(Object *obj, struct Visitor *v,
+                                          void *opaque, const char *name,
+                                          Error **errp)
+{
+    VirtIOBalloon *s = opaque;
+    visit_type_int(v, &s->stats_last_update, name, errp);
+}
+
+static void balloon_stats_get_stat(Object *obj, struct Visitor *v,
+                                   void *opaque, const char *name, Error **errp)
+{
+    VirtIOBalloon *s = opaque;
+    int i;
+
+    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
+        if (!strcmp(balloon_stat_names[i], name)) {
+            break;
+        }
+    }
+
+    if (i == VIRTIO_BALLOON_S_NR) {
+        error_setg(errp, "invalid stat name '%s'", name);
+        return;
+    }
+
+    if (s->stats[i] == -1) {
+        error_setg(errp,
+            "timer hasn't been enabled or guest doesn't support '%s'", name);
+        return;
+    }
+
+    visit_type_int(v, (int64_t *) &s->stats[i], name, errp);
+}
+
+static void balloon_stats_get_poll_interval(Object *obj, struct Visitor *v,
+                                            void *opaque, const char *name,
+                                            Error **errp)
+{
+    VirtIOBalloon *s = opaque;
+    visit_type_int(v, &s->stats_poll_interval, name, errp);
+}
+
+static void balloon_stats_set_poll_interval(Object *obj, struct Visitor *v,
+                                            void *opaque, const char *name,
+                                            Error **errp)
+{
+    VirtIOBalloon *s = opaque;
+    int64_t value;
+
+    if (!balloon_stats_supported(s)) {
+        error_setg(errp, "guest doesn\'t support balloon stats");
+        return;
+    }
+
+    visit_type_int(v, &value, name, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    if (value < 0) {
+        error_setg(errp, "timer value must be positive");
+        return;
+    }
+
+    if (value == s->stats_poll_interval) {
+        return;
+    }
+
+    if (value == 0) {
+        /* timer=0 disables the timer */
+        balloon_stats_destroy_timer(s);
+        return;
+    }
+
+    if (balloon_stats_enabled(s)) {
+        /* timer interval change */
+        s->stats_poll_interval = value;
+        balloon_stats_change_timer(s, value);
+        return;
+    }
+
+    /* create a new timer */
+    g_assert(s->stats_timer == NULL);
+    s->stats_timer = qemu_new_timer_ms(vm_clock, balloon_stats_poll_cb, s);
+    s->stats_poll_interval = value;
+    balloon_stats_change_timer(s, 0);
+}
+
 static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBalloon *s = to_virtio_balloon(vdev);
@@ -107,9 +243,10 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
     VirtQueueElement *elem = &s->stats_vq_elem;
     VirtIOBalloonStat stat;
     size_t offset = 0;
+    qemu_timeval tv;
 
     if (!virtqueue_pop(vq, elem)) {
-        return;
+        goto out;
     }
 
     /* Initialize the stats to get rid of any stale values.  This is only
@@ -128,6 +265,18 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
             s->stats[tag] = val;
     }
     s->stats_vq_offset = offset;
+
+    if (qemu_gettimeofday(&tv) < 0) {
+        fprintf(stderr, "warning: %s: failed to get time of day\n", __func__);
+        goto out;
+    }
+
+    s->stats_last_update = tv.tv_sec;
+
+out:
+    if (balloon_stats_enabled(s)) {
+        balloon_stats_change_timer(s, s->stats_poll_interval);
+    }
 }
 
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
@@ -212,7 +361,7 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id)
 VirtIODevice *virtio_balloon_init(DeviceState *dev)
 {
     VirtIOBalloon *s;
-    int ret;
+    int i, ret;
 
     s = (VirtIOBalloon *)virtio_common_init("virtio-balloon",
                                             VIRTIO_ID_BALLOON,
@@ -239,6 +388,19 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
     register_savevm(dev, "virtio-balloon", -1, 1,
                     virtio_balloon_save, virtio_balloon_load, s);
 
+    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
+        object_property_add(OBJECT(dev), balloon_stat_names[i], "int",
+                            balloon_stats_get_stat, NULL, NULL, s, NULL);
+    }
+
+    object_property_add(OBJECT(dev), "stats-last-update", "int",
+                        balloon_stats_get_last_update, NULL, NULL, s, NULL);
+
+    object_property_add(OBJECT(dev), "stats-polling-interval", "int",
+                        balloon_stats_get_poll_interval,
+                        balloon_stats_set_poll_interval,
+                        NULL, s, NULL);
+
     return &s->vdev;
 }
 
@@ -246,6 +408,7 @@ void virtio_balloon_exit(VirtIODevice *vdev)
 {
     VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
 
+    balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
     unregister_savevm(s->qdev, "virtio-balloon", s);
     virtio_cleanup(vdev);
-- 
1.8.0

[Qemu-devel] [PATCH 3/3] docs: document virtio-balloon stats

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 docs/virtio-balloon-stats.txt | 87 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 docs/virtio-balloon-stats.txt

diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
new file mode 100644
index 0000000..139e74d
--- /dev/null
+++ b/docs/virtio-balloon-stats.txt
@@ -0,0 +1,87 @@
+virtio balloon memory statistics
+================================
+
+The virtio balloon driver supports guest memory statistics reporting. These
+statistics are available to QEMU users as QOM (QEMU Object Model) device
+properties via a polling mechanism.
+
+Basically, clients first have to enable polling, then they can query the
+available statistics.
+
+There are two control properties and six memory statistics properties.
+
+The control properties are:
+
+ o stats-polling-interval: polling time interval in seconds, it can be:
+
+   > 0  enables polling in the specified interval. If polling is already
+        enabled, the polling time interval will be changed to the new value
+
+   0    disables polling. Previous polled statistics are still valid and
+        can be queried.
+
+ o stats-last-update: last stats update timestamp, in seconds
+
+The following statistics are available, all values are in bytes:
+
+ o stat-swap-in
+ o stat-swap-out
+ o stat-major-faults
+ o stat-minor-faults
+ o stat-free-memory
+ o stat-total-memory
+
+Also, please note the following:
+
+ - If a statistic is queried before the timer is enabled or if the guest
+   doesn't support a particular statistic, an error will be returned
+
+ - Previously polled statistics remain available even if the timer is
+   later disabled
+
+ - The polling timer is only re-armed when the guest answers to the
+   statistics request. This means that if the guest takes too long to
+   respond (say, a few seconds) this delay will end up being added to the
+   poll interval. Also, if a (buggy) guest doesn't ever respond, the
+   statistics won't get updated and the timer disarmed
+
+Here are a few examples. The virtio-balloon device is assumed to be in the
+'/machine/peripheral-anon/device[1]' QOM path.
+
+Enable polling with 2 seconds interval:
+
+{ "execute": "qom-set",
+             "arguments": { "path": "/machine/peripheral-anon/device[1]",
+			 "property": "stats-polling-interval", "value": 2 } }
+
+{ "return": {} }
+
+Change polling to 10 seconds:
+
+{ "execute": "qom-set",
+             "arguments": { "path": "/machine/peripheral-anon/device[1]",
+			 "property": "stats-polling-interval", "value": 10 } }
+
+{ "return": {} }
+
+Get last update timestamp and free memory stat:
+
+{ "execute": "qom-get",
+  "arguments": { "path": "/machine/peripheral-anon/device[1]",
+  "property": "stats-last-update" } }
+
+{ "return": 1354629634 }
+
+{ "execute": "qom-get",
+  "arguments": { "path": "/machine/peripheral-anon/device[1]",
+  "property": "stat-free-memory" } }
+
+{ "return": 845115392 }
+
+Disable polling:
+
+{ "execute": "qom-set",
+             "arguments": { "path": "/machine/peripheral-anon/device[1]",
+			 "property": "stats-polling-interval", "value": 0 } }
+
+{ "return": {} }
-- 
1.8.0

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

Can't we enable stat queries by default (10s interval), and simply return all stats
with one API call (query-ballon)? 

That qom-get interface forces me to do 6 API calls to get all information!

> -----Original Message-----
> From: qemu-devel-bounces+dietmar=proxmox.com@nongnu.org
> [mailto:qemu-devel-bounces+dietmar=proxmox.com@nongnu.org] On
> Behalf Of Luiz Capitulino
> Sent: Montag, 10. Dezember 2012 20:36
> To: qemu-devel@nongnu.org
> Cc: aliguori@us.ibm.com; mdroth@linux.vnet.ibm.com; agl@us.ibm.com
> Subject: [Qemu-devel] [PATCH 0/3] re-enable balloon stats
> 
> This new try to re-enable the virtio-balloon driver stats uses QOM properties
> via a polling mechanism as suggested by Anthony here:
> 
>  http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02390.html
> 
> o Changes from the rfc
> 
>  - avoid balloon_stats_poll_cb() and virtio_balloon_handle_output()
>    running in parallel
>  - small renames and re-writes for better readability
>  - update documentation
> 
> Luiz Capitulino (3):
>   virtio-balloon: drop old stats code
>   virtio-balloon: re-enable balloon stats
>   docs: document virtio-balloon stats
> 
>  docs/virtio-balloon-stats.txt |  87 +++++++++++++++++++
>  hw/virtio-balloon.c           | 189 ++++++++++++++++++++++++++++++++++++---
> ---
>  2 files changed, 252 insertions(+), 24 deletions(-)  create mode 100644
> docs/virtio-balloon-stats.txt
> 
> --
> 1.8.0
> 

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Tue, 11 Dec 2012 05:12:29 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> Can't we enable stat queries by default (10s interval),

I'm not sure I like this for two reasons. First, there will be cases where
the user doesn't want this to be enabled. Second, we'll be forcing an interval
on users.

> and simply return all stats with one API call (query-ballon)? 

We can't use query-balloon because this changes query-balloon from
synchronous to asynchronous, and this is an incompatible change.

Now, adding a new command to do this was my first proposal on this:

 http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg00983.html

But Anthony suggested doing this through qom, which I agree that is a
simpler & cleaner interface.

> That qom-get interface forces me to do 6 API calls to get all information!

Does this really matter for an application?

> 
> > -----Original Message-----
> > From: qemu-devel-bounces+dietmar=proxmox.com@nongnu.org
> > [mailto:qemu-devel-bounces+dietmar=proxmox.com@nongnu.org] On
> > Behalf Of Luiz Capitulino
> > Sent: Montag, 10. Dezember 2012 20:36
> > To: qemu-devel@nongnu.org
> > Cc: aliguori@us.ibm.com; mdroth@linux.vnet.ibm.com; agl@us.ibm.com
> > Subject: [Qemu-devel] [PATCH 0/3] re-enable balloon stats
> > 
> > This new try to re-enable the virtio-balloon driver stats uses QOM properties
> > via a polling mechanism as suggested by Anthony here:
> > 
> >  http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02390.html
> > 
> > o Changes from the rfc
> > 
> >  - avoid balloon_stats_poll_cb() and virtio_balloon_handle_output()
> >    running in parallel
> >  - small renames and re-writes for better readability
> >  - update documentation
> > 
> > Luiz Capitulino (3):
> >   virtio-balloon: drop old stats code
> >   virtio-balloon: re-enable balloon stats
> >   docs: document virtio-balloon stats
> > 
> >  docs/virtio-balloon-stats.txt |  87 +++++++++++++++++++
> >  hw/virtio-balloon.c           | 189 ++++++++++++++++++++++++++++++++++++---
> > ---
> >  2 files changed, 252 insertions(+), 24 deletions(-)  create mode 100644
> > docs/virtio-balloon-stats.txt
> > 
> > --
> > 1.8.0
> > 
> 
> 

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> > Can't we enable stat queries by default (10s interval),
> 
> I'm not sure I like this for two reasons. First, there will be cases where the
> user doesn't want this to be enabled. Second, we'll be forcing an interval on
> users.

OK.
 
> > and simply return all stats with one API call (query-ballon)?
> 
> We can't use query-balloon because this changes query-balloon from
> synchronous to asynchronous, and this is an incompatible change.

Why don't we simply cache the last received stats, and query-ballon returns
the cached values?

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Tue, 11 Dec 2012 12:05:32 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > and simply return all stats with one API call (query-ballon)?
> > 
> > We can't use query-balloon because this changes query-balloon from
> > synchronous to asynchronous, and this is an incompatible change.
> 
> Why don't we simply cache the last received stats, and query-ballon returns
> the cached values?

Giving that we're doing this through device properties, we'd duplicate
the interface by also returning this info in query-balloon.

However, it wouldn't be a problem to extend 'info balloon' (in HMP) with
that info. That is, 'info balloon' could use the QOM interface to query
balloon stats.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> > Can't we enable stat queries by default (10s interval),
> 
> I'm not sure I like this for two reasons. First, there will be cases where the
> user doesn't want this to be enabled. Second, we'll be forcing an interval on
> users.

So when should we set the stats-polling-interval? I first thought I simply
set that at VM start, but that triggers an error:

"guest doesn't support balloon stats"

because the balloon driver is not loaded.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Tue, 11 Dec 2012 12:29:01 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > Can't we enable stat queries by default (10s interval),
> > 
> > I'm not sure I like this for two reasons. First, there will be cases where the
> > user doesn't want this to be enabled. Second, we'll be forcing an interval on
> > users.
> 
> So when should we set the stats-polling-interval? I first thought I simply
> set that at VM start, but that triggers an error:
> 
> "guest doesn't support balloon stats"
> 
> because the balloon driver is not loaded.

Yes, it's required to have the balloon driver loaded. The stats are
reported by it.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> > > I'm not sure I like this for two reasons. First, there will be cases
> > > where the user doesn't want this to be enabled. Second, we'll be
> > > forcing an interval on users.
> >
> > So when should we set the stats-polling-interval? I first thought I
> > simply set that at VM start, but that triggers an error:
> >
> > "guest doesn't support balloon stats"
> >
> > because the balloon driver is not loaded.
> 
> Yes, it's required to have the balloon driver loaded. The stats are reported by
> it.

That does not really answers my question. So you think the management
framework should start the VM, and then wait until the balloon driver is
loaded? That sound very clumsy to me.

I mean, I just want to set the polling interval. Why does that need the balloon drive loaded 
inside the guest (polling is done by the host)?

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[mdroth]

On Tue, Dec 11, 2012 at 03:14:02PM +0000, Dietmar Maurer wrote:
> > > > I'm not sure I like this for two reasons. First, there will be cases
> > > > where the user doesn't want this to be enabled. Second, we'll be
> > > > forcing an interval on users.
> > >
> > > So when should we set the stats-polling-interval? I first thought I
> > > simply set that at VM start, but that triggers an error:
> > >
> > > "guest doesn't support balloon stats"
> > >
> > > because the balloon driver is not loaded.
> > 
> > Yes, it's required to have the balloon driver loaded. The stats are reported by
> > it.
> 
> That does not really answers my question. So you think the management
> framework should start the VM, and then wait until the balloon driver is
> loaded? That sound very clumsy to me.
> 
> I mean, I just want to set the polling interval. Why does that need the balloon drive loaded 
> inside the guest (polling is done by the host)?

I agree. Should lack of a balloon module disable the timer
completely, or just silently fail? Management can always reference
stats-last-update to check that stats are being reported properly. -1 would
mean driver was never loaded, longer update intervals might mean guest
was rebooted/suspended/etc, but in both cases it makes sense that the
timer still try to fetch stats, and the determination of what stats are
valid/invalid be left to management.

Alternatively, we can hook into the feature negotiation path and
defer the actual timer start until the required feature is negotiated,
and disable it (temporarilly or permanently based on guest behavior)
somewhere in the reset path). I'm not sure this will keep it from
running during hibernate/sleep...maybe hook into runstate changes?

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Tue, 11 Dec 2012 11:38:08 -0600
mdroth <mdroth@linux.vnet.ibm.com> wrote:

> On Tue, Dec 11, 2012 at 03:14:02PM +0000, Dietmar Maurer wrote:
> > > > > I'm not sure I like this for two reasons. First, there will be cases
> > > > > where the user doesn't want this to be enabled. Second, we'll be
> > > > > forcing an interval on users.
> > > >
> > > > So when should we set the stats-polling-interval? I first thought I
> > > > simply set that at VM start, but that triggers an error:
> > > >
> > > > "guest doesn't support balloon stats"
> > > >
> > > > because the balloon driver is not loaded.
> > > 
> > > Yes, it's required to have the balloon driver loaded. The stats are reported by
> > > it.
> > 
> > That does not really answers my question. So you think the management
> > framework should start the VM, and then wait until the balloon driver is
> > loaded? That sound very clumsy to me.
> > 
> > I mean, I just want to set the polling interval. Why does that need the balloon drive loaded 
> > inside the guest (polling is done by the host)?
> 
> I agree. Should lack of a balloon module disable the timer
> completely, or just silently fail? Management can always reference
> stats-last-update to check that stats are being reported properly. -1 would
> mean driver was never loaded, longer update intervals might mean guest
> was rebooted/suspended/etc, but in both cases it makes sense that the
> timer still try to fetch stats, and the determination of what stats are
> valid/invalid be left to management.

I could move the check for the stats feature bit from the function that
enables polling to the timer callback. This would solve Dietmar's use-case,
but it would silently fail if the feature is never negotiated (as you said
above).

> Alternatively, we can hook into the feature negotiation path and
> defer the actual timer start until the required feature is negotiated,

That's also possible, and helps not wasting resources.

> and disable it (temporarilly or permanently based on guest behavior)
> somewhere in the reset path). I'm not sure this will keep it from
> running during hibernate/sleep...maybe hook into runstate changes?

Then it starts to get somewhat complicated.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Tue, 11 Dec 2012 16:28:40 -0200
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Tue, 11 Dec 2012 11:38:08 -0600
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Dec 11, 2012 at 03:14:02PM +0000, Dietmar Maurer wrote:
> > > > > > I'm not sure I like this for two reasons. First, there will be cases
> > > > > > where the user doesn't want this to be enabled. Second, we'll be
> > > > > > forcing an interval on users.
> > > > >
> > > > > So when should we set the stats-polling-interval? I first thought I
> > > > > simply set that at VM start, but that triggers an error:
> > > > >
> > > > > "guest doesn't support balloon stats"
> > > > >
> > > > > because the balloon driver is not loaded.
> > > > 
> > > > Yes, it's required to have the balloon driver loaded. The stats are reported by
> > > > it.
> > > 
> > > That does not really answers my question. So you think the management
> > > framework should start the VM, and then wait until the balloon driver is
> > > loaded? That sound very clumsy to me.
> > > 
> > > I mean, I just want to set the polling interval. Why does that need the balloon drive loaded 
> > > inside the guest (polling is done by the host)?
> > 
> > I agree. Should lack of a balloon module disable the timer
> > completely, or just silently fail? Management can always reference
> > stats-last-update to check that stats are being reported properly. -1 would
> > mean driver was never loaded, longer update intervals might mean guest
> > was rebooted/suspended/etc, but in both cases it makes sense that the
> > timer still try to fetch stats, and the determination of what stats are
> > valid/invalid be left to management.
> 
> I could move the check for the stats feature bit from the function that
> enables polling to the timer callback. This would solve Dietmar's use-case,
> but it would silently fail if the feature is never negotiated (as you said
> above).

Btw, we could also add functionality to qemu-ga to list loaded modules.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> I could move the check for the stats feature bit from the function that
> enables polling to the timer callback. This would solve Dietmar's use-case,

that would be great.

> but it would silently fail if the feature is never negotiated (as you said
> above).

Why don't we raise the error when we query the values?

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Tue, 11 Dec 2012 19:07:48 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > I could move the check for the stats feature bit from the function that
> > enables polling to the timer callback. This would solve Dietmar's use-case,
> 
> that would be great.
> 
> > but it would silently fail if the feature is never negotiated (as you said
> > above).
> 
> Why don't we raise the error when we query the values?

Hmm, that's a good idea.

Only small nit is that, today old stats will remain available to be queried
even if you disable the timer. If we do what you suggest, old stats won't
be available if the module is unloaded (well, I *guess* the guest will unset
the feature bit on module removal).

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> > > but it would silently fail if the feature is never negotiated (as
> > > you said above).
> >
> > Why don't we raise the error when we query the values?
> 
> Hmm, that's a good idea.
> 
> Only small nit is that, today old stats will remain available to be queried even
> if you disable the timer. If we do what you suggest, old stats won't be
> available if the module is unloaded (well, I *guess* the guest will unset the
> feature bit on module removal).

What is the problem with that? It would be no problem to simulate the 
old behavior, but why do you want that? 

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Tue, 11 Dec 2012 19:39:40 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > > but it would silently fail if the feature is never negotiated (as
> > > > you said above).
> > >
> > > Why don't we raise the error when we query the values?
> > 
> > Hmm, that's a good idea.
> > 
> > Only small nit is that, today old stats will remain available to be queried even
> > if you disable the timer. If we do what you suggest, old stats won't be
> > available if the module is unloaded (well, I *guess* the guest will unset the
> > feature bit on module removal).
> 
> What is the problem with that? It would be no problem to simulate the 
> old behavior, but why do you want that? 

I actually just thought that it's a nice to have.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[mdroth]

On Tue, Dec 11, 2012 at 04:28:40PM -0200, Luiz Capitulino wrote:
> On Tue, 11 Dec 2012 11:38:08 -0600
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Dec 11, 2012 at 03:14:02PM +0000, Dietmar Maurer wrote:
> > > > > > I'm not sure I like this for two reasons. First, there will be cases
> > > > > > where the user doesn't want this to be enabled. Second, we'll be
> > > > > > forcing an interval on users.
> > > > >
> > > > > So when should we set the stats-polling-interval? I first thought I
> > > > > simply set that at VM start, but that triggers an error:
> > > > >
> > > > > "guest doesn't support balloon stats"
> > > > >
> > > > > because the balloon driver is not loaded.
> > > > 
> > > > Yes, it's required to have the balloon driver loaded. The stats are reported by
> > > > it.
> > > 
> > > That does not really answers my question. So you think the management
> > > framework should start the VM, and then wait until the balloon driver is
> > > loaded? That sound very clumsy to me.
> > > 
> > > I mean, I just want to set the polling interval. Why does that need the balloon drive loaded 
> > > inside the guest (polling is done by the host)?
> > 
> > I agree. Should lack of a balloon module disable the timer
> > completely, or just silently fail? Management can always reference
> > stats-last-update to check that stats are being reported properly. -1 would
> > mean driver was never loaded, longer update intervals might mean guest
> > was rebooted/suspended/etc, but in both cases it makes sense that the
> > timer still try to fetch stats, and the determination of what stats are
> > valid/invalid be left to management.
> 
> I could move the check for the stats feature bit from the function that
> enables polling to the timer callback. This would solve Dietmar's use-case,
> but it would silently fail if the feature is never negotiated (as you said
> above).
> 
> > Alternatively, we can hook into the feature negotiation path and
> > defer the actual timer start until the required feature is negotiated,
> 
> That's also possible, and helps not wasting resources.
> 
> > and disable it (temporarilly or permanently based on guest behavior)
> > somewhere in the reset path). I'm not sure this will keep it from
> > running during hibernate/sleep...maybe hook into runstate changes?
> 
> Then it starts to get somewhat complicated.
> 

Agreed...that's only if we want to go to extremes to avoid running the
timer unecessarily, but probably not worth it.

I like your idea of just having the timer callback fail silently if the
feature isn't enabled.

And personally, I think just having a stale value for stats-last-update
is sufficient for error checking, but we can squirrel away an error in
a stats-last-error field if we want something more descriptive for
clients, and just wipe it out on the next successful update. Not sure
whether we should clear the stats upon error or not, I think it might
actually be useful to leave them. If the guest crashed because we
ballooned away too much memory or something for instance we'd be able
to see the last value we logged.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> And personally, I think just having a stale value for stats-last-update is
> sufficient for error checking, but we can squirrel away an error in a stats-last-
> error field if we want something more descriptive for clients, and just wipe it
> out on the next successful update. Not sure whether we should clear the
> stats upon error or not, I think it might actually be useful to leave them. If the
> guest crashed because we ballooned away too much memory or something
> for instance we'd be able to see the last value we logged.

I think the management app should cache the value if that is needed.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> > > > Why don't we raise the error when we query the values?

You already raise an error here, so that works out of the box:

    if (s->stats[i] == -1) {
        error_setg(errp,
            "timer hasn't been enabled or guest doesn't support '%s'", name);

> > >
> > > Hmm, that's a good idea.
> > >
> > > Only small nit is that, today old stats will remain available to be
> > > queried even if you disable the timer. If we do what you suggest,
> > > old stats won't be available if the module is unloaded (well, I
> > > *guess* the guest will unset the feature bit on module removal).
> >
> > What is the problem with that? It would be no problem to simulate the
> > old behavior, but why do you want that?
> 
> I actually just thought that it's a nice to have.

The following patch seems to work for me:

Index: new/hw/virtio-balloon.c
===================================================================
--- new.orig/hw/virtio-balloon.c	2012-12-12 14:05:56.000000000 +0100
+++ new/hw/virtio-balloon.c	2012-12-12 14:07:43.000000000 +0100
@@ -111,6 +111,10 @@
 {
     VirtIOBalloon *s = opaque;
 
+    if (!balloon_stats_supported(s) || !runstate_is_running()) {
+        return;
+    }
+
     virtqueue_push(s->svq, &s->stats_vq_elem, s->stats_vq_offset);
     virtio_notify(&s->vdev, s->svq);
 }
@@ -164,11 +168,6 @@
     VirtIOBalloon *s = opaque;
     int64_t value;
 
-    if (!balloon_stats_supported(s)) {
-        error_setg(errp, "guest doesn\'t support balloon stats");
-        return;
-    }
-
     visit_type_int(v, &value, name, errp);
     if (error_is_set(errp)) {
         return;

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Wed, 12 Dec 2012 13:17:15 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > > > Why don't we raise the error when we query the values?
> 
> You already raise an error here, so that works out of the box:
> 
>     if (s->stats[i] == -1) {
>         error_setg(errp,
>             "timer hasn't been enabled or guest doesn't support '%s'", name);
> 
> > > >
> > > > Hmm, that's a good idea.
> > > >
> > > > Only small nit is that, today old stats will remain available to be
> > > > queried even if you disable the timer. If we do what you suggest,
> > > > old stats won't be available if the module is unloaded (well, I
> > > > *guess* the guest will unset the feature bit on module removal).
> > >
> > > What is the problem with that? It would be no problem to simulate the
> > > old behavior, but why do you want that?
> > 
> > I actually just thought that it's a nice to have.
> 
> The following patch seems to work for me:

Yeah, that's what I'll do and the doc has to be updated too.

> 
> Index: new/hw/virtio-balloon.c
> ===================================================================
> --- new.orig/hw/virtio-balloon.c	2012-12-12 14:05:56.000000000 +0100
> +++ new/hw/virtio-balloon.c	2012-12-12 14:07:43.000000000 +0100
> @@ -111,6 +111,10 @@
>  {
>      VirtIOBalloon *s = opaque;
>  
> +    if (!balloon_stats_supported(s) || !runstate_is_running()) {
> +        return;
> +    }
> +
>      virtqueue_push(s->svq, &s->stats_vq_elem, s->stats_vq_offset);
>      virtio_notify(&s->vdev, s->svq);
>  }
> @@ -164,11 +168,6 @@
>      VirtIOBalloon *s = opaque;
>      int64_t value;
>  
> -    if (!balloon_stats_supported(s)) {
> -        error_setg(errp, "guest doesn\'t support balloon stats");
> -        return;
> -    }
> -
>      visit_type_int(v, &value, name, errp);
>      if (error_is_set(errp)) {
>          return;
> 
> 
> 

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> > The following patch seems to work for me:
> 
> Yeah, that's what I'll do and the doc has to be updated too.

Btw, do you know a qmp call which returns the memory assigned to the VM
(the value qemu is started with (-m megs))?

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Wed, 12 Dec 2012 13:36:20 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > The following patch seems to work for me:
> > 
> > Yeah, that's what I'll do and the doc has to be updated too.
> 
> Btw, do you know a qmp call which returns the memory assigned to the VM
> (the value qemu is started with (-m megs))?

No, we only have info/query- balloon. I was also thinking about this the
other day.

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Dietmar Maurer]

> > Btw, do you know a qmp call which returns the memory assigned to the
> > VM (the value qemu is started with (-m megs))?
> 
> No, we only have info/query- balloon. I was also thinking about this the
> other day.

Can we add an additional property: "stat-max-memory"?

Or add 'max_mem' property to BalloonInfo (return with query-ballon)?

Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[Luiz Capitulino]

On Wed, 12 Dec 2012 13:45:54 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > Btw, do you know a qmp call which returns the memory assigned to the
> > > VM (the value qemu is started with (-m megs))?
> > 
> > No, we only have info/query- balloon. I was also thinking about this the
> > other day.
> 
> Can we add an additional property: "stat-max-memory"?
> 
> Or add 'max_mem' property to BalloonInfo (return with query-ballon)?

Yes, I prefer the latter. But let's have a more descriptive name, like
aximum-balloon-memory (or something like that).