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

[Qemu-devel] [PATCH v2 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 v2

 - drop qmp & hmp interface of old stats code
 - allow polling callback to run even if the guest hasn't negotiated
   stats feature bit
 - update doc

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

 docs/virtio-balloon-stats.txt |  91 +++++++++++++++++++
 hmp.c                         |  24 +----
 hw/virtio-balloon.c           | 200 +++++++++++++++++++++++++++++++++++++-----
 qapi-schema.json              |  20 +----
 qmp-commands.hx               |  13 ---
 5 files changed, 269 insertions(+), 79 deletions(-)
 create mode 100644 docs/virtio-balloon-stats.txt

-- 
1.8.0

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

[Luiz Capitulino]

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

It's important to note that the QMP and HMP interfaces are also dropped
by this commit. That shouldn't be a problem though, because:

 1. All QMP fields are optional
 2. This has never been really used

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp.c               | 24 +-----------------------
 hw/virtio-balloon.c | 22 ----------------------
 qapi-schema.json    | 20 +-------------------
 qmp-commands.hx     | 13 -------------
 4 files changed, 2 insertions(+), 77 deletions(-)

diff --git a/hmp.c b/hmp.c
index 180ba2b..f1cbbbe 100644
--- a/hmp.c
+++ b/hmp.c
@@ -464,29 +464,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/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);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..e2fdd40 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -977,28 +977,10 @@
 #
 # @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:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c692d0..c05d336 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2549,13 +2549,6 @@ Make an asynchronous request for balloon info. When the request completes a
 json-object will be returned containing the following data:
 
 - "actual": current balloon value in bytes (json-int)
-- "mem_swapped_in": Amount of memory swapped in bytes (json-int, optional)
-- "mem_swapped_out": Amount of memory swapped out in bytes (json-int, optional)
-- "major_page_faults": Number of major faults (json-int, optional)
-- "minor_page_faults": Number of minor faults (json-int, optional)
-- "free_mem": Total amount of free and unused memory in
-              bytes (json-int, optional)
-- "total_mem": Total amount of available memory in bytes (json-int, optional)
 
 Example:
 
@@ -2563,12 +2556,6 @@ Example:
 <- {
       "return":{
          "actual":1073741824,
-         "mem_swapped_in":0,
-         "mem_swapped_out":0,
-         "major_page_faults":142,
-         "minor_page_faults":239245,
-         "free_mem":1014185984,
-         "total_mem":1044668416
       }
    }
 
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Dietmar Maurer]

> Next commit will re-enable balloon stats with a different interface, but this
> old code conflicts with it. Let's drop it.

I don't really see any conflicts here?

> It's important to note that the QMP and HMP interfaces are also dropped by
> this commit. That shouldn't be a problem though, because:
> 
>  1. All QMP fields are optional
>  2. This has never been really used

Why don't we simply implement the missing parts - it's just a few lines of code.

for example:

Index: new/hw/virtio-balloon.c
===================================================================
--- new.orig/hw/virtio-balloon.c	2012-12-17 07:55:34.000000000 +0100
+++ new/hw/virtio-balloon.c	2012-12-17 09:20:32.000000000 +0100
@@ -59,7 +59,7 @@
 }
 
 static const char *balloon_stat_names[] = {
-   [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in", 
+   [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",
@@ -314,6 +314,30 @@
     VirtIOBalloon *dev = opaque;
     info->actual = ram_size - ((uint64_t) dev->actual <<
                                VIRTIO_BALLOON_PFN_SHIFT);
+
+    info->total_mem = ram_size;
+
+    if (!(balloon_stats_enabled(dev) && dev->stats_last_update)) {
+        return;
+    }
+
+    info->last_update = dev->stats_last_update;
+    info->has_last_update = true;
+
+    info->mem_swapped_in = dev->stats[VIRTIO_BALLOON_S_SWAP_IN];
+    info->has_mem_swapped_in = info->mem_swapped_in >= 0 ? true : false;
+
+    info->mem_swapped_out = dev->stats[VIRTIO_BALLOON_S_SWAP_OUT];
+    info->has_mem_swapped_out = info->mem_swapped_out >= 0 ? true : false;
+
+    info->major_page_faults = dev->stats[VIRTIO_BALLOON_S_MAJFLT];
+    info->has_major_page_faults = info->major_page_faults >= 0 ? true : false;
+
+    info->minor_page_faults = dev->stats[VIRTIO_BALLOON_S_MINFLT];
+    info->has_minor_page_faults = info->minor_page_faults >= 0 ? true : false;
+
+    info->free_mem = dev->stats[VIRTIO_BALLOON_S_MEMFREE];
+    info->has_free_mem = info->free_mem >= 0 ? true : false;
 }
 
 static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
Index: new/qapi-schema.json
===================================================================
--- new.orig/qapi-schema.json	2012-12-17 08:19:30.000000000 +0100
+++ new/qapi-schema.json	2012-12-17 08:35:55.000000000 +0100
@@ -1044,6 +1044,8 @@
 #
 # @actual: the number of bytes the balloon currently contains
 #
+# @last_update: #optional time when stats got updated from guest
+#
 # @mem_swapped_in: #optional number of pages swapped in within the guest
 #
 # @mem_swapped_out: #optional number of pages swapped out within the guest
@@ -1054,18 +1056,15 @@
 #
 # @free_mem: #optional amount of memory (in bytes) free in the guest
 #
-# @total_mem: #optional amount of memory (in bytes) visible to the guest
+# @total_mem: 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',
+  'data': {'actual': 'int', '*last_update': 'int', '*mem_swapped_in': 'int',
            '*mem_swapped_out': 'int', '*major_page_faults': 'int',
            '*minor_page_faults': 'int', '*free_mem': 'int',
-           '*total_mem': 'int'} }
+           'total_mem': 'int'} }
 
 ##
 # @query-balloon:
Index: new/hmp.c
===================================================================
--- new.orig/hmp.c	2012-12-17 08:36:51.000000000 +0100
+++ new/hmp.c	2012-12-17 09:06:15.000000000 +0100
@@ -497,6 +497,11 @@
     }
 
     monitor_printf(mon, "balloon: actual=%" PRId64, info->actual >> 20);
+    monitor_printf(mon, " total_mem=%" PRId64, info->total_mem >> 20);
+    if (info->has_free_mem) {
+        monitor_printf(mon, " free_mem=%" PRId64, info->free_mem >> 20);
+    }
+
     if (info->has_mem_swapped_in) {
         monitor_printf(mon, " mem_swapped_in=%" PRId64, info->mem_swapped_in);
     }
@@ -511,11 +516,9 @@
         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);
+    if (info->has_last_update) {
+        monitor_printf(mon, " last_update=%" PRId64,
+                       info->last_update);
     }
 
     monitor_printf(mon, "\n");

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Luiz Capitulino]

On Mon, 17 Dec 2012 10:13:40 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > Next commit will re-enable balloon stats with a different interface, but this
> > old code conflicts with it. Let's drop it.
> 
> I don't really see any conflicts here?

query-balloon resets all stats to -1. So, if it's issued after the stats have
been polled all values are lost.

> > It's important to note that the QMP and HMP interfaces are also dropped by
> > this commit. That shouldn't be a problem though, because:
> > 
> >  1. All QMP fields are optional
> >  2. This has never been really used
> 
> Why don't we simply implement the missing parts - it's just a few lines of code.

For the reasons explained in my last email. We'd need at least two commands,
one for to enable polling/set time interval and also query-balloon-stats,
as adding this to query-balloon would mix semantics (ie. one field is always
there and the others have polling semantics).

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Dietmar Maurer]

> > I don't really see any conflicts here?
> 
> query-balloon resets all stats to -1. So, if it's issued after the stats have been
> polled all values are lost.

I can't see that in the code. I also tested with my patch, and it does not reset
any stats. So I can't see that conflict?

There is no code to reset stats inside virtio_balloon_stat()

> > > It's important to note that the QMP and HMP interfaces are also
> > > dropped by this commit. That shouldn't be a problem though, because:
> > >
> > >  1. All QMP fields are optional
> > >  2. This has never been really used
> >
> > Why don't we simply implement the missing parts - it's just a few lines of
> code.
> 
> For the reasons explained in my last email. We'd need at least two
> commands, one for to enable polling/set time interval and also query-
> balloon-stats, as adding this to query-balloon would mix semantics (ie. one
> field is always there and the others have polling semantics).

Yes, we need 2 commands - and what is the problem?

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Luiz Capitulino]

On Mon, 17 Dec 2012 12:23:59 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > I don't really see any conflicts here?
> > 
> > query-balloon resets all stats to -1. So, if it's issued after the stats have been
> > polled all values are lost.
> 
> I can't see that in the code. I also tested with my patch, and it does not reset
> any stats. So I can't see that conflict?
> 
> There is no code to reset stats inside virtio_balloon_stat()

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);
}

> 
> > > > It's important to note that the QMP and HMP interfaces are also
> > > > dropped by this commit. That shouldn't be a problem though, because:
> > > >
> > > >  1. All QMP fields are optional
> > > >  2. This has never been really used
> > >
> > > Why don't we simply implement the missing parts - it's just a few lines of
> > code.
> > 
> > For the reasons explained in my last email. We'd need at least two
> > commands, one for to enable polling/set time interval and also query-
> > balloon-stats, as adding this to query-balloon would mix semantics (ie. one
> > field is always there and the others have polling semantics).
> 
> Yes, we need 2 commands - and what is the problem?

The problem is adding new commands when that's not required. With QOM
everything can be done with existing commands.

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Dietmar Maurer]

> > There is no code to reset stats inside virtio_balloon_stat()
> 
> 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

My patch allies on top of your patch, and you already removed that code.

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Luiz Capitulino]

On Mon, 17 Dec 2012 12:36:59 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > There is no code to reset stats inside virtio_balloon_stat()
> > 
> > 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
> 
> My patch allies on top of your patch, and you already removed that code.

This was in reply to your comment that said you didn't see any
conflict with the code being dropped and *my* new code.

Please, stop trolling.

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Eric Blake]

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

On 12/17/2012 03:13 AM, Dietmar Maurer wrote:
>> Next commit will re-enable balloon stats with a different interface, but this
>> old code conflicts with it. Let's drop it.
> 
> I don't really see any conflicts here?
> 
>> It's important to note that the QMP and HMP interfaces are also dropped by
>> this commit. That shouldn't be a problem though, because:
>>
>>  1. All QMP fields are optional
>>  2. This has never been really used

Libvirt has been using it when available (although reluctantly, as it
risks hanging on an uncooperative guest); and while libvirt can be
patched to call 6 QOM commands in a row to query six different QOM
stats, I still think it would be nicer to add a command that provides
all the stats at once.  In particular, when calling 6 commands in
series, you no longer have an atomic picture of the guest (the polling
interval could hit between two QOM queries, resulting in a combined set
of statistics that has no counterpart to the transition of states that
the guest actually went through).  On the other hand, since the stats
are already polling-based, and since it requires cooperation from the
guest, not having a guarantee of an atomic set of stats is not really
much of a loss.

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Dietmar Maurer]

> Libvirt has been using it when available (although reluctantly, as it risks
> hanging on an uncooperative guest); and while libvirt can be patched to call 6
> QOM commands in a row to query six different QOM stats, I still think it
> would be nicer to add a command that provides all the stats at once.  In
> particular, when calling 6 commands in series, you no longer have an atomic
> picture of the guest (the polling interval could hit between two QOM queries,
> resulting in a combined set of statistics that has no counterpart to the
> transition of states that the guest actually went through).  On the other hand,
> since the stats are already polling-based, and since it requires cooperation
> from the guest, not having a guarantee of an atomic set of stats is not really
> much of a loss.

I already implemented that and sent patches to this list. The patch is more or less
trivial -  we simply read the cached values. It does not hang on an 
uncooperative guest, and works without any problems.

But unfortunately, Luiz dislikes the patch (I do not understand why). So I stopped posting it.

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Luiz Capitulino]

On Tue, 18 Dec 2012 14:34:16 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 12/17/2012 03:13 AM, Dietmar Maurer wrote:
> >> Next commit will re-enable balloon stats with a different interface, but this
> >> old code conflicts with it. Let's drop it.
> > 
> > I don't really see any conflicts here?
> > 
> >> It's important to note that the QMP and HMP interfaces are also dropped by
> >> this commit. That shouldn't be a problem though, because:
> >>
> >>  1. All QMP fields are optional
> >>  2. This has never been really used
> 
> Libvirt has been using it when available (although reluctantly, as it
> risks hanging on an uncooperative guest);

This has always been disabled and qemu never returns the stats info.
I believe libvirt's code is rotting just like qemu's is.

> and while libvirt can be
> patched to call 6 QOM commands in a row to query six different QOM
> stats, I still think it would be nicer to add a command that provides
> all the stats at once.  In particular, when calling 6 commands in
> series, you no longer have an atomic picture of the guest (the polling
> interval could hit between two QOM queries, resulting in a combined set
> of statistics that has no counterpart to the transition of states that
> the guest actually went through). On the other hand, since the stats
> are already polling-based, and since it requires cooperation from the
> guest, not having a guarantee of an atomic set of stats is not really
> much of a loss.

Something I have been wondering if whether it's possible to have only
one property (say balloon-statistics) and return all properties in a
dict. QOM properties return a visitor, so maybe that's possible.

I'll check that.

Re: [Qemu-devel] [PATCH 1/3] balloon: drop old stats code & API

[Eric Blake]

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

On 12/19/2012 04:27 AM, Luiz Capitulino wrote:
>> Libvirt has been using it when available (although reluctantly, as it
>> risks hanging on an uncooperative guest);
> 
> This has always been disabled and qemu never returns the stats info.
> I believe libvirt's code is rotting just like qemu's is.

I agree that the QMP has never been providing it (since we pulled it out
of QMP precisely because of the potential for a hang), but if you go
back far enough to an old qemu that provided the stats via HMP, then
libvirt is indeed exposing those stats to the user.

There are two places in libvirt code that were collecting stats.  One
for determining what to print for simple commands like 'virsh dumpxml',
where we only cared about balloon size and not the rest of the
statistics, and where we do NOT want to hang waiting on the guest; for
that case, the solution that Dan came up with was adding a balloon
event, and using the event instead of a query.  The other usage is that
we have an API where the user can specifically request all the stats;
and there, we DO want to block on the guest to get the stats, and would
prefer getting the stats in a single command (but can tolerate getting
the stats via 6 separate commands, if that's what it takes).  It is this
second usage that has been broken in libvirt ever since we crippled the
stats collection in qemu because of the undesirable hang in the first case.

> Something I have been wondering if whether it's possible to have only
> one property (say balloon-statistics) and return all properties in a
> dict. QOM properties return a visitor, so maybe that's possible.
> 
> I'll check that.

Ah, that would be a nice trick - querying a single QOM property that
turns out to be a dict exposing multiple sub-properties at once.  And if
you can get that to work, you are back to your goal of no new QMP command.

-- 
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: 619 bytes --]

[Qemu-devel] [PATCH 2/3] 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 | 178 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 176 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 4398025..47e35b1 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,138 @@ 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;
+
+    if (!balloon_stats_supported(s)) {
+        /* re-schedule */
+        balloon_stats_change_timer(s, s->stats_poll_interval);
+        return;
+    }
+
+    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) {
+        /*
+         * Possible reasons for this error:
+         *
+         * - The timer hasn't been enabled
+         * - The guest hasn't loaded its balloon driver
+         * - The guest's balloon driver doesn't support memory stats
+         * - The guest's balloon driver doesn't support this stat
+         * - The guest's balloon driver didn't send this stat for
+         *   whatever reason
+         */
+        error_setg(errp,
+            "guest didn't update '%s' (does the guest support it?)", 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;
+
+    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 +254,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 +276,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 +372,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 +399,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 +419,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 | 91 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 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..990e746
--- /dev/null
+++ b/docs/virtio-balloon-stats.txt
@@ -0,0 +1,91 @@
+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
+
+ - Polling can be enabled even if the guest doesn't support memory
+   statistics or its balloon driver hasn't been loaded. Applications
+   can check this condition by checking that stats-last-update doesn't
+   change
+
+ - The polling timer is only re-armed when the guest responds to the
+   statistics request. This means that if a (buggy) guest doesn't
+   ever respond to the request the timer will never be re-armed,
+   which has the same effect as disabling polling
+
+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 v2 0/3] re-enable balloon stats

[Dietmar Maurer]

>  - drop qmp & hmp interface of old stats code

I think the old interface is not that bad (the new one is clumsy, because
we need 6 qmp call instead of one to get all stats). Can't
we try to make it functional and keep it?

Seriously, I think 'interval' should be a property we can pass at qemu
startup. Then we can simply use the old interface to get cached values.

I also do not understand you argument that you can't use the old
interface because it was synchrounous  - removing the old interface is also
not compatible ;-)

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

[Luiz Capitulino]

On Sat, 15 Dec 2012 07:19:13 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> >  - drop qmp & hmp interface of old stats code
> 
> I think the old interface is not that bad (the new one is clumsy, because
> we need 6 qmp call instead of one to get all stats). Can't
> we try to make it functional and keep it?

No, because it breaks existing clients and that's why it's disabled since
qemu 0.12 or so.

Let me try once more to explain our options and why I think this is series
is our best choice.

Basically, we have try options:

1. Add stats to query-balloon

This breaks existing clients, because query-balloon is a synchronous command
that returns immediately. If we add stats to it (as we did) then it will
have to wait for the guest to respond, which can take a long time or even
doesn't happen at all if the guest is paused.

This broke libvirt, as explained here:

 https://bugzilla.redhat.com/show_bug.cgi?id=623903

2. Add a new command that just request the stats to be sent (ie. it doesn't
block), and then get the stats through an event

This was my last proposal:

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

This sort of worked, but it has two problems. First, events can be lost.
Second, most failures can't be synchronously reported.

3. Add stats through device properties (this series)

While this isn't perfect, it does solve the issues with previous
implementations. Besides, it has the advantage of avoiding adding new
commands and it integrates nicely with the balloon driver, making some
errors automatically reported (eg. if the balloon is not enabled).

> Seriously, I think 'interval' should be a property we can pass at qemu
> startup. Then we can simply use the old interface to get cached values.

What if you want to change the interval or even set it case it wasn't set?
You'll need a new command. Any different behavior we want to add will require
a new command. Which is one of your own complaints against this series.

> I also do not understand you argument that you can't use the old
> interface because it was synchrounous  -

Explained above.

> removing the old interface is also
> not compatible ;-)

It's not, for two reasons. First, all fields are optional. Second, this was
never in a stable release (or was disabled right after the release).

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

[Dietmar Maurer]

> Let me try once more to explain our options and why I think this is series is
> our best choice.
> 
> Basically, we have try options:
> 
> 1. Add stats to query-balloon
> 
> This breaks existing clients, because query-balloon is a synchronous
> command that returns immediately. If we add stats to it (as we did) then it
> will have to wait for the guest to respond, which can take a long time or even
> doesn't happen at all if the guest is paused.

No, there is no need to wait (see my patch).
 
> This broke libvirt, as explained here:
> 
>  https://bugzilla.redhat.com/show_bug.cgi?id=623903

My new patch does not introduce such bug.
 
> 2. Add a new command that just request the stats to be sent (ie. it doesn't
> block), and then get the stats through an event
> 
> This was my last proposal:
> 
>  http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg00983.html
> 
> This sort of worked, but it has two problems. First, events can be lost.
> Second, most failures can't be synchronously reported.
> 
> 3. Add stats through device properties (this series)
> 
> While this isn't perfect, it does solve the issues with previous
> implementations. Besides, it has the advantage of avoiding adding new
> commands and it integrates nicely with the balloon driver, making some
> errors automatically reported (eg. if the balloon is not enabled).

We can have both - see my new patch.

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

[Luiz Capitulino]

On Mon, 17 Dec 2012 12:17:28 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > Let me try once more to explain our options and why I think this is series is
> > our best choice.
> > 
> > Basically, we have try options:
> > 
> > 1. Add stats to query-balloon
> > 
> > This breaks existing clients, because query-balloon is a synchronous
> > command that returns immediately. If we add stats to it (as we did) then it
> > will have to wait for the guest to respond, which can take a long time or even
> > doesn't happen at all if the guest is paused.
> 
> No, there is no need to wait (see my patch).

It has problems, replied to it.

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

[Dietmar Maurer]

> > No, there is no need to wait (see my patch).
> 
> It has problems, replied to it.

Please can you post a test case which show those problems? I simply can't 
reproduce any problem you mention.

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

[Luiz Capitulino]

On Mon, 17 Dec 2012 12:26:34 +0000
Dietmar Maurer <dietmar@proxmox.com> wrote:

> > > No, there is no need to wait (see my patch).
> > 
> > It has problems, replied to it.
> 
> Please can you post a test case which show those problems? I simply can't 
> reproduce any problem you mention.

Which problems are you referring to? Also, I think it's time for you to
search for answers and make some effort at understanding my arguments.

Since the beginning of this thread I've tried to respond you and provide
you all information I have, but all I heard from you is "show me", "I don't
see it" and "I want it implemented my way, period".

That's by far not productive.

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

[Dietmar Maurer]

> Since the beginning of this thread I've tried to respond you and provide you
> all information I have, but all I heard from you is "show me", "I don't see it"
> and "I want it implemented my way, period".
> 
> That's by far not productive.

I event sent you a working patch!

Anyways - I also give up now.