| Message ID | 24E144B8C0207547AD09C467A8259F75578B48B1@lisa.maurer-it.com |
|---|---|
| State | New |
| Headers | show |
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).
> > 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?
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.
> > 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.
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.
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.
> 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.
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.
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.
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");