Patchwork [1/3] balloon: drop old stats code & API

login
register
mail settings
Submitter Dietmar Maurer
Date Dec. 17, 2012, 10:13 a.m.
Message ID <24E144B8C0207547AD09C467A8259F75578B48B1@lisa.maurer-it.com>
Download mbox | patch
Permalink /patch/206811/
State New
Headers show

Comments

Dietmar Maurer - Dec. 17, 2012, 10:13 a.m.
> 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:
Luiz Capitulino - Dec. 17, 2012, 11:57 a.m.
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).
Dietmar Maurer - Dec. 17, 2012, 12:23 p.m.
> > 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?
Luiz Capitulino - Dec. 17, 2012, 12:33 p.m.
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.
Dietmar Maurer - Dec. 17, 2012, 12:36 p.m.
> > 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.
Luiz Capitulino - Dec. 17, 2012, 12:39 p.m.
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.
Eric Blake - Dec. 18, 2012, 9:34 p.m.
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.
Dietmar Maurer - Dec. 19, 2012, 5:18 a.m.
> 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.
Luiz Capitulino - Dec. 19, 2012, 11:27 a.m.
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.
Eric Blake - Dec. 20, 2012, 6:36 p.m.
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.

Patch

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