[Qemu-devel] [PATCH 0/2] virtio-balloon: improve balloon statistics

[Qemu-devel] [PATCH 0/2] virtio-balloon: improve balloon statistics

[Denis V. Lunev]

New counter from the Linux kernel + generic framework to pass currently
unknown counters via QMP for debug purposes.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Igor Redko <redkoi@virtuozzo.com>
CC: Michael S. Tsirkin <mst@redhat.com>

[Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Denis V. Lunev]

From: Igor Redko <redkoi@virtuozzo.com>

We are making experiments with different autoballooning strategies
based on the guest behavior. Thus we need to experiment with different
guest statistics. For now every counter change requires QEMU recompilation
and dances with Libvirt.

This patch introduces transport for unrecognized counters in virtio-balloon.
This transport can be used for measuring benefits from using new
balloon counters, before submitting any patches. Current alternative
is 'guest-exec' transport which isn't made for such delicate matters
and can influence test results.

Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
where XXXX is a tag number in hex. Defined counters are reported with their
regular names.

Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c         | 30 ++++++++++++++++++++++++------
 include/hw/virtio/virtio-balloon.h |  3 ++-
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a382f43..cc551a3 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -66,8 +66,7 @@ static const char *balloon_stat_names[] = {
  */
 static inline void reset_stats(VirtIOBalloon *dev)
 {
-    int i;
-    for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
+    dev->stats_cnt = 0;
 }
 
 static bool balloon_stats_supported(const VirtIOBalloon *s)
@@ -133,12 +132,20 @@ static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
     if (err) {
         goto out_end;
     }
-    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
-        visit_type_uint64(v, balloon_stat_names[i], &s->stats[i], &err);
+    for (i = 0; !err && i < s->stats_cnt; i++) {
+        if (s->stats[i].tag < VIRTIO_BALLOON_S_NR) {
+            visit_type_uint64(v, balloon_stat_names[s->stats[i].tag],
+                              &s->stats[i].val, &err);
+        } else {
+            gchar *str = g_strdup_printf("x-stat-%04x", s->stats[i].tag);
+            visit_type_uint64(v, str, &s->stats[i].val, &err);
+            g_free(str);
+        }
         if (err) {
             break;
         }
     }
+
     error_propagate(errp, err);
     err = NULL;
     visit_end_struct(v, &err);
@@ -273,10 +280,21 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
            == sizeof(stat)) {
         uint16_t tag = virtio_tswap16(vdev, stat.tag);
         uint64_t val = virtio_tswap64(vdev, stat.val);
+        int i;
 
         offset += sizeof(stat);
-        if (tag < VIRTIO_BALLOON_S_NR)
-            s->stats[tag] = val;
+        for (i = 0; i < s->stats_cnt; i++) {
+            if (s->stats[i].tag == tag) {
+                break;
+            }
+        }
+        if (i < ARRAY_SIZE(s->stats)) {
+            s->stats[i].tag = tag;
+            s->stats[i].val = val;
+            if (s->stats_cnt <= i) {
+                s->stats_cnt = i + 1;
+            }
+        }
     }
     s->stats_vq_offset = offset;
 
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 35f62ac..5c8730e 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -36,7 +36,8 @@ typedef struct VirtIOBalloon {
     VirtQueue *ivq, *dvq, *svq;
     uint32_t num_pages;
     uint32_t actual;
-    uint64_t stats[VIRTIO_BALLOON_S_NR];
+    VirtIOBalloonStatModern stats[VIRTIO_BALLOON_S_NR + 32];
+    uint16_t stats_cnt;
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
     QEMUTimer *stats_timer;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Eric Blake]

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

On 02/20/2016 12:54 AM, Denis V. Lunev wrote:
> From: Igor Redko <redkoi@virtuozzo.com>
> 
> We are making experiments with different autoballooning strategies
> based on the guest behavior. Thus we need to experiment with different
> guest statistics. For now every counter change requires QEMU recompilation
> and dances with Libvirt.
> 
> This patch introduces transport for unrecognized counters in virtio-balloon.
> This transport can be used for measuring benefits from using new
> balloon counters, before submitting any patches. Current alternative
> is 'guest-exec' transport which isn't made for such delicate matters
> and can influence test results.
> 
> Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> where XXXX is a tag number in hex. Defined counters are reported with their
> regular names.

The name implies experimental, so apps like libvirt shouldn't rely on
it; while it does fill a gap for making development easier, so I'm in
favor of the idea.


> @@ -133,12 +132,20 @@ static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
>      if (err) {
>          goto out_end;
>      }
> -    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
> -        visit_type_uint64(v, balloon_stat_names[i], &s->stats[i], &err);
> +    for (i = 0; !err && i < s->stats_cnt; i++) {

Why are you checking for err here?  You cannot enter the loop with err
set...

> +        if (s->stats[i].tag < VIRTIO_BALLOON_S_NR) {
> +            visit_type_uint64(v, balloon_stat_names[s->stats[i].tag],
> +                              &s->stats[i].val, &err);
> +        } else {
> +            gchar *str = g_strdup_printf("x-stat-%04x", s->stats[i].tag);
> +            visit_type_uint64(v, str, &s->stats[i].val, &err);
> +            g_free(str);
> +        }
>          if (err) {
>              break;
>          }

...and you cannot exit the loop with it set. Therefore the check is dead
code.

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

[Qemu-devel] [PATCH 2/2] virtio-balloon: add 'available' counter

[Denis V. Lunev]

The patch for the kernel part is in linux-next already:
commit ac88e7c908b920866e529862f2b2f0129b254ab2
    Author: Igor Redko <redkoi@virtuozzo.com>
    Date:   Thu Feb 18 09:23:01 2016 +1100

    virtio_balloon: export 'available' memory to balloon statistics

    Add a new field, VIRTIO_BALLOON_S_AVAIL, to virtio_balloon memory
    statistics protocol, corresponding to 'Available' in /proc/meminfo.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Igor Redko <redkoi@virtuozzo.com>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c                      | 1 +
 include/standard-headers/linux/virtio_balloon.h | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index cc551a3..b052dfb 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -53,6 +53,7 @@ static const char *balloon_stat_names[] = {
    [VIRTIO_BALLOON_S_MINFLT] = "stat-minor-faults",
    [VIRTIO_BALLOON_S_MEMFREE] = "stat-free-memory",
    [VIRTIO_BALLOON_S_MEMTOT] = "stat-total-memory",
+   [VIRTIO_BALLOON_S_AVAIL] = "stat-available-memory",
    [VIRTIO_BALLOON_S_NR] = NULL
 };
 
diff --git a/include/standard-headers/linux/virtio_balloon.h b/include/standard-headers/linux/virtio_balloon.h
index 2e2a6dc..0df7c2e 100644
--- a/include/standard-headers/linux/virtio_balloon.h
+++ b/include/standard-headers/linux/virtio_balloon.h
@@ -51,7 +51,8 @@ struct virtio_balloon_config {
 #define VIRTIO_BALLOON_S_MINFLT   3   /* Number of minor faults */
 #define VIRTIO_BALLOON_S_MEMFREE  4   /* Total amount of free memory */
 #define VIRTIO_BALLOON_S_MEMTOT   5   /* Total amount of memory */
-#define VIRTIO_BALLOON_S_NR       6
+#define VIRTIO_BALLOON_S_AVAIL    6   /* Amount of available memory in guest */
+#define VIRTIO_BALLOON_S_NR       7
 
 /*
  * Memory statistics structure.
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: add 'available' counter

[Eric Blake]

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

On 02/20/2016 12:54 AM, Denis V. Lunev wrote:
> The patch for the kernel part is in linux-next already:
> commit ac88e7c908b920866e529862f2b2f0129b254ab2
>     Author: Igor Redko <redkoi@virtuozzo.com>
>     Date:   Thu Feb 18 09:23:01 2016 +1100
> 
>     virtio_balloon: export 'available' memory to balloon statistics
> 
>     Add a new field, VIRTIO_BALLOON_S_AVAIL, to virtio_balloon memory
>     statistics protocol, corresponding to 'Available' in /proc/meminfo.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Igor Redko <redkoi@virtuozzo.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c                      | 1 +
>  include/standard-headers/linux/virtio_balloon.h | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH v2 0/2] virtio-balloon: improve balloon statistics

[Denis V. Lunev]

New counter from the Linux kernel + generic framework to pass currently
unknown counters via QMP for debug purposes.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Igor Redko <redkoi@virtuozzo.com>
CC: Michael S. Tsirkin <mst@redhat.com>

Changes from v1:
- removed !err in patch 1 by suggestion from Eric

[Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Denis V. Lunev]

From: Igor Redko <redkoi@virtuozzo.com>

We are making experiments with different autoballooning strategies
based on the guest behavior. Thus we need to experiment with different
guest statistics. For now every counter change requires QEMU recompilation
and dances with Libvirt.

This patch introduces transport for unrecognized counters in virtio-balloon.
This transport can be used for measuring benefits from using new
balloon counters, before submitting any patches. Current alternative
is 'guest-exec' transport which isn't made for such delicate matters
and can influence test results.

Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
where XXXX is a tag number in hex. Defined counters are reported with their
regular names.

Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c         | 30 ++++++++++++++++++++++++------
 include/hw/virtio/virtio-balloon.h |  3 ++-
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a382f43..1740293 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -66,8 +66,7 @@ static const char *balloon_stat_names[] = {
  */
 static inline void reset_stats(VirtIOBalloon *dev)
 {
-    int i;
-    for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
+    dev->stats_cnt = 0;
 }
 
 static bool balloon_stats_supported(const VirtIOBalloon *s)
@@ -133,12 +132,20 @@ static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
     if (err) {
         goto out_end;
     }
-    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
-        visit_type_uint64(v, balloon_stat_names[i], &s->stats[i], &err);
+    for (i = 0; i < s->stats_cnt; i++) {
+        if (s->stats[i].tag < VIRTIO_BALLOON_S_NR) {
+            visit_type_uint64(v, balloon_stat_names[s->stats[i].tag],
+                              &s->stats[i].val, &err);
+        } else {
+            gchar *str = g_strdup_printf("x-stat-%04x", s->stats[i].tag);
+            visit_type_uint64(v, str, &s->stats[i].val, &err);
+            g_free(str);
+        }
         if (err) {
             break;
         }
     }
+
     error_propagate(errp, err);
     err = NULL;
     visit_end_struct(v, &err);
@@ -273,10 +280,21 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
            == sizeof(stat)) {
         uint16_t tag = virtio_tswap16(vdev, stat.tag);
         uint64_t val = virtio_tswap64(vdev, stat.val);
+        int i;
 
         offset += sizeof(stat);
-        if (tag < VIRTIO_BALLOON_S_NR)
-            s->stats[tag] = val;
+        for (i = 0; i < s->stats_cnt; i++) {
+            if (s->stats[i].tag == tag) {
+                break;
+            }
+        }
+        if (i < ARRAY_SIZE(s->stats)) {
+            s->stats[i].tag = tag;
+            s->stats[i].val = val;
+            if (s->stats_cnt <= i) {
+                s->stats_cnt = i + 1;
+            }
+        }
     }
     s->stats_vq_offset = offset;
 
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 35f62ac..5c8730e 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -36,7 +36,8 @@ typedef struct VirtIOBalloon {
     VirtQueue *ivq, *dvq, *svq;
     uint32_t num_pages;
     uint32_t actual;
-    uint64_t stats[VIRTIO_BALLOON_S_NR];
+    VirtIOBalloonStatModern stats[VIRTIO_BALLOON_S_NR + 32];
+    uint16_t stats_cnt;
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
     QEMUTimer *stats_timer;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Michael S. Tsirkin]

On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> From: Igor Redko <redkoi@virtuozzo.com>
> 
> We are making experiments with different autoballooning strategies
> based on the guest behavior. Thus we need to experiment with different
> guest statistics. For now every counter change requires QEMU recompilation
> and dances with Libvirt.
> 
> This patch introduces transport for unrecognized counters in virtio-balloon.
> This transport can be used for measuring benefits from using new
> balloon counters, before submitting any patches. Current alternative
> is 'guest-exec' transport which isn't made for such delicate matters
> and can influence test results.
> 
> Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> where XXXX is a tag number in hex. Defined counters are reported with their
> regular names.
> 
> Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael S. Tsirkin <mst@redhat.com>

This seems to open the ABI to abuse.
Seems like a reasonable way to experiment though.
How about adding this within #if 0 statements?
You can uncomment them for debugging ...

> ---
>  hw/virtio/virtio-balloon.c         | 30 ++++++++++++++++++++++++------
>  include/hw/virtio/virtio-balloon.h |  3 ++-
>  2 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a382f43..1740293 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -66,8 +66,7 @@ static const char *balloon_stat_names[] = {
>   */
>  static inline void reset_stats(VirtIOBalloon *dev)
>  {
> -    int i;
> -    for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
> +    dev->stats_cnt = 0;
>  }
>  
>  static bool balloon_stats_supported(const VirtIOBalloon *s)
> @@ -133,12 +132,20 @@ static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
>      if (err) {
>          goto out_end;
>      }
> -    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
> -        visit_type_uint64(v, balloon_stat_names[i], &s->stats[i], &err);
> +    for (i = 0; i < s->stats_cnt; i++) {
> +        if (s->stats[i].tag < VIRTIO_BALLOON_S_NR) {
> +            visit_type_uint64(v, balloon_stat_names[s->stats[i].tag],
> +                              &s->stats[i].val, &err);
> +        } else {
> +            gchar *str = g_strdup_printf("x-stat-%04x", s->stats[i].tag);
> +            visit_type_uint64(v, str, &s->stats[i].val, &err);
> +            g_free(str);
> +        }
>          if (err) {
>              break;
>          }
>      }
> +
>      error_propagate(errp, err);
>      err = NULL;
>      visit_end_struct(v, &err);
> @@ -273,10 +280,21 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
>             == sizeof(stat)) {
>          uint16_t tag = virtio_tswap16(vdev, stat.tag);
>          uint64_t val = virtio_tswap64(vdev, stat.val);
> +        int i;
>  
>          offset += sizeof(stat);
> -        if (tag < VIRTIO_BALLOON_S_NR)
> -            s->stats[tag] = val;
> +        for (i = 0; i < s->stats_cnt; i++) {
> +            if (s->stats[i].tag == tag) {
> +                break;
> +            }
> +        }
> +        if (i < ARRAY_SIZE(s->stats)) {
> +            s->stats[i].tag = tag;
> +            s->stats[i].val = val;
> +            if (s->stats_cnt <= i) {
> +                s->stats_cnt = i + 1;
> +            }
> +        }
>      }
>      s->stats_vq_offset = offset;
>  
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 35f62ac..5c8730e 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -36,7 +36,8 @@ typedef struct VirtIOBalloon {
>      VirtQueue *ivq, *dvq, *svq;
>      uint32_t num_pages;
>      uint32_t actual;
> -    uint64_t stats[VIRTIO_BALLOON_S_NR];
> +    VirtIOBalloonStatModern stats[VIRTIO_BALLOON_S_NR + 32];
> +    uint16_t stats_cnt;
>      VirtQueueElement *stats_vq_elem;
>      size_t stats_vq_offset;
>      QEMUTimer *stats_timer;
> -- 
> 2.1.4

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Denis V. Lunev]

On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
>> From: Igor Redko <redkoi@virtuozzo.com>
>>
>> We are making experiments with different autoballooning strategies
>> based on the guest behavior. Thus we need to experiment with different
>> guest statistics. For now every counter change requires QEMU recompilation
>> and dances with Libvirt.
>>
>> This patch introduces transport for unrecognized counters in virtio-balloon.
>> This transport can be used for measuring benefits from using new
>> balloon counters, before submitting any patches. Current alternative
>> is 'guest-exec' transport which isn't made for such delicate matters
>> and can influence test results.
>>
>> Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
>> Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
>> queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
>> where XXXX is a tag number in hex. Defined counters are reported with their
>> regular names.
>>
>> Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Michael S. Tsirkin <mst@redhat.com>
> This seems to open the ABI to abuse.
> Seems like a reasonable way to experiment though.
> How about adding this within #if 0 statements?
> You can uncomment them for debugging ...
I'd prefer to have this enabled.

Why do you think that it opens "abuse" way?

Actually the amount of host data is limited. If the guest
will send fake stats before real ones - this guest is
not cooperative and in this case the guest can
hust ignore any balloon change requests.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Michael S. Tsirkin]

On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
> On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> >>From: Igor Redko <redkoi@virtuozzo.com>
> >>
> >>We are making experiments with different autoballooning strategies
> >>based on the guest behavior. Thus we need to experiment with different
> >>guest statistics. For now every counter change requires QEMU recompilation
> >>and dances with Libvirt.
> >>
> >>This patch introduces transport for unrecognized counters in virtio-balloon.
> >>This transport can be used for measuring benefits from using new
> >>balloon counters, before submitting any patches. Current alternative
> >>is 'guest-exec' transport which isn't made for such delicate matters
> >>and can influence test results.
> >>
> >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> >>where XXXX is a tag number in hex. Defined counters are reported with their
> >>regular names.
> >>
> >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>CC: Michael S. Tsirkin <mst@redhat.com>
> >This seems to open the ABI to abuse.
> >Seems like a reasonable way to experiment though.
> >How about adding this within #if 0 statements?
> >You can uncomment them for debugging ...
> I'd prefer to have this enabled.
> 
> Why do you think that it opens "abuse" way?

Because people will use this to hack drivers and management tools
bypassing qemu.

> Actually the amount of host data is limited. If the guest
> will send fake stats before real ones - this guest is
> not cooperative and in this case the guest can
> hust ignore any balloon change requests.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Roman Kagan]

On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> > >>From: Igor Redko <redkoi@virtuozzo.com>
> > >>
> > >>We are making experiments with different autoballooning strategies
> > >>based on the guest behavior. Thus we need to experiment with different
> > >>guest statistics. For now every counter change requires QEMU recompilation
> > >>and dances with Libvirt.
> > >>
> > >>This patch introduces transport for unrecognized counters in virtio-balloon.
> > >>This transport can be used for measuring benefits from using new
> > >>balloon counters, before submitting any patches. Current alternative
> > >>is 'guest-exec' transport which isn't made for such delicate matters
> > >>and can influence test results.
> > >>
> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> > >>where XXXX is a tag number in hex. Defined counters are reported with their
> > >>regular names.
> > >>
> > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> > >>CC: Michael S. Tsirkin <mst@redhat.com>
> > >This seems to open the ABI to abuse.
> > >Seems like a reasonable way to experiment though.
> > >How about adding this within #if 0 statements?
> > >You can uncomment them for debugging ...
> > I'd prefer to have this enabled.
> > 
> > Why do you think that it opens "abuse" way?
> 
> Because people will use this to hack drivers and management tools
> bypassing qemu.

I'm curious why you think it's a problem?  Even the existing stats are
simply propagated to the management level by qemu with no processing
other than assigning text labels.  The proposed naming scheme for
unrecognized counters includes "x-" prefix which explicitly marks them
as unstable so people using them take their risk.

One of the benefits is forward compatibility, so that counters that have
graduated into supported ones and have got their own number and name,
can be made to work with qemu that doesn't yet recognize them.

[ Yes I've seen Den having posted a version hiding this under #ifdef, but I'd
still be interested to know why the initial proposal wasn't found
generally useful ]

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Michael S. Tsirkin]

On Wed, Feb 24, 2016 at 01:01:34PM +0300, Roman Kagan wrote:
> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
> > > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> > > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> > > >>From: Igor Redko <redkoi@virtuozzo.com>
> > > >>
> > > >>We are making experiments with different autoballooning strategies
> > > >>based on the guest behavior. Thus we need to experiment with different
> > > >>guest statistics. For now every counter change requires QEMU recompilation
> > > >>and dances with Libvirt.
> > > >>
> > > >>This patch introduces transport for unrecognized counters in virtio-balloon.
> > > >>This transport can be used for measuring benefits from using new
> > > >>balloon counters, before submitting any patches. Current alternative
> > > >>is 'guest-exec' transport which isn't made for such delicate matters
> > > >>and can influence test results.
> > > >>
> > > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> > > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> > > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> > > >>where XXXX is a tag number in hex. Defined counters are reported with their
> > > >>regular names.
> > > >>
> > > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
> > > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> > > >>CC: Michael S. Tsirkin <mst@redhat.com>
> > > >This seems to open the ABI to abuse.
> > > >Seems like a reasonable way to experiment though.
> > > >How about adding this within #if 0 statements?
> > > >You can uncomment them for debugging ...
> > > I'd prefer to have this enabled.
> > > 
> > > Why do you think that it opens "abuse" way?
> > 
> > Because people will use this to hack drivers and management tools
> > bypassing qemu.
> 
> I'm curious why you think it's a problem?

Because we'll be stuck maintaining them with these x- names.

> Even the existing stats are
> simply propagated to the management level by qemu with no processing
> other than assigning text labels.
>
>  The proposed naming scheme for
> unrecognized counters includes "x-" prefix which explicitly marks them
> as unstable so people using them take their risk.
>
> One of the benefits is forward compatibility, so that counters that have
> graduated into supported ones and have got their own number and name,
> can be made to work with qemu that doesn't yet recognize them.

Then management does start relying on the x- prefixed things,
and once it's used to that it's a slippery slope.

> [ Yes I've seen Den having posted a version hiding this under #ifdef, but I'd
> still be interested to know why the initial proposal wasn't found
> generally useful ]
> 
> Thanks,
> Roman.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Eric Blake]

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

On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
>> One of the benefits is forward compatibility, so that counters that have
>> graduated into supported ones and have got their own number and name,
>> can be made to work with qemu that doesn't yet recognize them.
> 
> Then management does start relying on the x- prefixed things,
> and once it's used to that it's a slippery slope.

Any management tool that relies on an x- prefix name is broken.  We've
explicitly documented that the x- prefix is unstable and liable to go
away with a future release. Any management app that wants to use a
feature beginning with x- should FIRST push hard to get the x- removed
and stabilize the interface (and libvirt, at least, does just that).

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

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Denis V. Lunev]

On 02/24/2016 06:43 PM, Eric Blake wrote:
> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
>>> One of the benefits is forward compatibility, so that counters that have
>>> graduated into supported ones and have got their own number and name,
>>> can be made to work with qemu that doesn't yet recognize them.
>> Then management does start relying on the x- prefixed things,
>> and once it's used to that it's a slippery slope.
> Any management tool that relies on an x- prefix name is broken.  We've
> explicitly documented that the x- prefix is unstable and liable to go
> away with a future release. Any management app that wants to use a
> feature beginning with x- should FIRST push hard to get the x- removed
> and stabilize the interface (and libvirt, at least, does just that).
>
this was exactly an original idea. Names started with 'x-' are
_officially_ unstable and for debug purpose. That is why I'd
prefer if v2 of the patchset will be taken.

Thank you.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> On 02/24/2016 06:43 PM, Eric Blake wrote:
>> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
>>> Roman Kagan <rkagan@virtuozzo.com> writes:
>>>> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
>>>>> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
>>>>> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
>>>>> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
>>>>> > >>From: Igor Redko <redkoi@virtuozzo.com>
>>>>> > >>
>>>>> > >>We are making experiments with different autoballooning strategies
>>>>> > >>based on the guest behavior. Thus we need to experiment with different
>>>>> > >>guest statistics. For now every counter change requires QEMU recompilation
>>>>> > >>and dances with Libvirt.
>>>>> > >>
>>>>> > >>This patch introduces transport for unrecognized counters in virtio-balloon.
>>>>> > >>This transport can be used for measuring benefits from using new
>>>>> > >>balloon counters, before submitting any patches. Current alternative
>>>>> > >>is 'guest-exec' transport which isn't made for such delicate matters
>>>>> > >>and can influence test results.
>>>>> > >>
>>>>> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
>>>>> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
>>>>> > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
>>>>> > >>where XXXX is a tag number in hex. Defined counters are reported with their
>>>>> > >>regular names.
>>>>> > >>
>>>>> > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
>>>>> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>> > >>CC: Michael S. Tsirkin <mst@redhat.com>
>>>>> > >This seems to open the ABI to abuse.
>>>>> > >Seems like a reasonable way to experiment though.
>>>>> > >How about adding this within #if 0 statements?
>>>>> > >You can uncomment them for debugging ...
>>>>> > I'd prefer to have this enabled.

Yes, conditional compilation should be used sparingly.  I don't have an
opinion on whether using it here is appropriate.

>>>>> > Why do you think that it opens "abuse" way?
>>>>> 
>>>>> Because people will use this to hack drivers and management tools
>>>>> bypassing qemu.

Easy to avoid: shuffle the N in x-stat-N around from time to time, to
reinforce the lesson that you must not rely on their presence or
semantics.  I doubt it'll be necessary beyond the renumbering that
happens naturally when we add supported counters, or the reshuffling
that happens when somebody messes with the unsupported counters.

>>>> I'm curious why you think it's a problem?  Even the existing stats are
>>>> simply propagated to the management level by qemu with no processing
>>>> other than assigning text labels.  The proposed naming scheme for
>>>> unrecognized counters includes "x-" prefix which explicitly marks them
>>>> as unstable so people using them take their risk.
>>>>
>>>> One of the benefits is forward compatibility, so that counters that have
>>>> graduated into supported ones and have got their own number and name,
>>>> can be made to work with qemu that doesn't yet recognize them.
>>> Then management does start relying on the x- prefixed things,
>>> and once it's used to that it's a slippery slope.
>> Any management tool that relies on an x- prefix name is broken.

Or at least assumes the full risk of breaking without notice whenever
QEMU changes.  Abbreviating that to just "broken" seems fair enough :)

>>                                                                  We've
>> explicitly documented that the x- prefix is unstable and liable to go
>> away with a future release. Any management app that wants to use a
>> feature beginning with x- should FIRST push hard to get the x- removed
>> and stabilize the interface (and libvirt, at least, does just that).
>>
> this was exactly an original idea. Names started with 'x-' are
> _officially_ unstable and for debug purpose. That is why I'd
> prefer if v2 of the patchset will be taken.

Looks like fair use of x- to me.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Michael S. Tsirkin]

On Thu, Feb 25, 2016 at 09:44:06AM +0100, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
> 
> > On 02/24/2016 06:43 PM, Eric Blake wrote:
> >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
> >>> Roman Kagan <rkagan@virtuozzo.com> writes:
> >>>> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
> >>>>> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
> >>>>> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> >>>>> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> >>>>> > >>From: Igor Redko <redkoi@virtuozzo.com>
> >>>>> > >>
> >>>>> > >>We are making experiments with different autoballooning strategies
> >>>>> > >>based on the guest behavior. Thus we need to experiment with different
> >>>>> > >>guest statistics. For now every counter change requires QEMU recompilation
> >>>>> > >>and dances with Libvirt.
> >>>>> > >>
> >>>>> > >>This patch introduces transport for unrecognized counters in virtio-balloon.
> >>>>> > >>This transport can be used for measuring benefits from using new
> >>>>> > >>balloon counters, before submitting any patches. Current alternative
> >>>>> > >>is 'guest-exec' transport which isn't made for such delicate matters
> >>>>> > >>and can influence test results.
> >>>>> > >>
> >>>>> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> >>>>> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> >>>>> > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> >>>>> > >>where XXXX is a tag number in hex. Defined counters are reported with their
> >>>>> > >>regular names.
> >>>>> > >>
> >>>>> > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
> >>>>> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>>>> > >>CC: Michael S. Tsirkin <mst@redhat.com>
> >>>>> > >This seems to open the ABI to abuse.
> >>>>> > >Seems like a reasonable way to experiment though.
> >>>>> > >How about adding this within #if 0 statements?
> >>>>> > >You can uncomment them for debugging ...
> >>>>> > I'd prefer to have this enabled.
> 
> Yes, conditional compilation should be used sparingly.  I don't have an
> opinion on whether using it here is appropriate.
> 
> >>>>> > Why do you think that it opens "abuse" way?
> >>>>> 
> >>>>> Because people will use this to hack drivers and management tools
> >>>>> bypassing qemu.
> 
> Easy to avoid: shuffle the N in x-stat-N around from time to time, to
> reinforce the lesson that you must not rely on their presence or
> semantics.  I doubt it'll be necessary beyond the renumbering that
> happens naturally when we add supported counters, or the reshuffling
> that happens when somebody messes with the unsupported counters.
> 
> >>>> I'm curious why you think it's a problem?  Even the existing stats are
> >>>> simply propagated to the management level by qemu with no processing
> >>>> other than assigning text labels.  The proposed naming scheme for
> >>>> unrecognized counters includes "x-" prefix which explicitly marks them
> >>>> as unstable so people using them take their risk.
> >>>>
> >>>> One of the benefits is forward compatibility, so that counters that have
> >>>> graduated into supported ones and have got their own number and name,
> >>>> can be made to work with qemu that doesn't yet recognize them.
> >>> Then management does start relying on the x- prefixed things,
> >>> and once it's used to that it's a slippery slope.
> >> Any management tool that relies on an x- prefix name is broken.
> 
> Or at least assumes the full risk of breaking without notice whenever
> QEMU changes.  Abbreviating that to just "broken" seems fair enough :)
> 
> >>                                                                  We've
> >> explicitly documented that the x- prefix is unstable and liable to go
> >> away with a future release. Any management app that wants to use a
> >> feature beginning with x- should FIRST push hard to get the x- removed
> >> and stabilize the interface (and libvirt, at least, does just that).
> >>
> > this was exactly an original idea. Names started with 'x-' are
> > _officially_ unstable and for debug purpose. That is why I'd
> > prefer if v2 of the patchset will be taken.
> 
> Looks like fair use of x- to me.


Well I already heard:

	One of the benefits is forward compatibility, so that counters that have
	graduated into supported ones and have got their own number and name,
	can be made to work with qemu that doesn't yet recognize them.

in this thread, which seems to mean exactly that people start planning to abuse it
even before it's merged.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Roman Kagan]

On Thu, Feb 25, 2016 at 10:54:17AM +0200, Michael S. Tsirkin wrote:
> On Thu, Feb 25, 2016 at 09:44:06AM +0100, Markus Armbruster wrote:
> > "Denis V. Lunev" <den@openvz.org> writes:
> > 
> > > On 02/24/2016 06:43 PM, Eric Blake wrote:
> > >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
> > >>> Roman Kagan <rkagan@virtuozzo.com> writes:
> > >>>> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
> > >>>>> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
> > >>>>> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> > >>>>> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> > >>>>> > >>From: Igor Redko <redkoi@virtuozzo.com>
> > >>>>> > >>
> > >>>>> > >>We are making experiments with different autoballooning strategies
> > >>>>> > >>based on the guest behavior. Thus we need to experiment with different
> > >>>>> > >>guest statistics. For now every counter change requires QEMU recompilation
> > >>>>> > >>and dances with Libvirt.
> > >>>>> > >>
> > >>>>> > >>This patch introduces transport for unrecognized counters in virtio-balloon.
> > >>>>> > >>This transport can be used for measuring benefits from using new
> > >>>>> > >>balloon counters, before submitting any patches. Current alternative
> > >>>>> > >>is 'guest-exec' transport which isn't made for such delicate matters
> > >>>>> > >>and can influence test results.
> > >>>>> > >>
> > >>>>> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> > >>>>> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> > >>>>> > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> > >>>>> > >>where XXXX is a tag number in hex. Defined counters are reported with their
> > >>>>> > >>regular names.
> > >>>>> > >>
> > >>>>> > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
> > >>>>> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> > >>>>> > >>CC: Michael S. Tsirkin <mst@redhat.com>
> > >>>>> > >This seems to open the ABI to abuse.
> > >>>>> > >Seems like a reasonable way to experiment though.
> > >>>>> > >How about adding this within #if 0 statements?
> > >>>>> > >You can uncomment them for debugging ...
> > >>>>> > I'd prefer to have this enabled.
> > 
> > Yes, conditional compilation should be used sparingly.  I don't have an
> > opinion on whether using it here is appropriate.
> > 
> > >>>>> > Why do you think that it opens "abuse" way?
> > >>>>> 
> > >>>>> Because people will use this to hack drivers and management tools
> > >>>>> bypassing qemu.
> > 
> > Easy to avoid: shuffle the N in x-stat-N around from time to time, to
> > reinforce the lesson that you must not rely on their presence or
> > semantics.  I doubt it'll be necessary beyond the renumbering that
> > happens naturally when we add supported counters, or the reshuffling
> > that happens when somebody messes with the unsupported counters.
> > 
> > >>>> I'm curious why you think it's a problem?  Even the existing stats are
> > >>>> simply propagated to the management level by qemu with no processing
> > >>>> other than assigning text labels.  The proposed naming scheme for
> > >>>> unrecognized counters includes "x-" prefix which explicitly marks them
> > >>>> as unstable so people using them take their risk.
> > >>>>
> > >>>> One of the benefits is forward compatibility, so that counters that have
> > >>>> graduated into supported ones and have got their own number and name,
> > >>>> can be made to work with qemu that doesn't yet recognize them.
> > >>> Then management does start relying on the x- prefixed things,
> > >>> and once it's used to that it's a slippery slope.
> > >> Any management tool that relies on an x- prefix name is broken.
> > 
> > Or at least assumes the full risk of breaking without notice whenever
> > QEMU changes.  Abbreviating that to just "broken" seems fair enough :)
> > 
> > >>                                                                  We've
> > >> explicitly documented that the x- prefix is unstable and liable to go
> > >> away with a future release. Any management app that wants to use a
> > >> feature beginning with x- should FIRST push hard to get the x- removed
> > >> and stabilize the interface (and libvirt, at least, does just that).
> > >>
> > > this was exactly an original idea. Names started with 'x-' are
> > > _officially_ unstable and for debug purpose. That is why I'd
> > > prefer if v2 of the patchset will be taken.
> > 
> > Looks like fair use of x- to me.
> 
> 
> Well I already heard:
> 
> 	One of the benefits is forward compatibility, so that counters that have
> 	graduated into supported ones and have got their own number and name,
> 	can be made to work with qemu that doesn't yet recognize them.
> 
> in this thread, which seems to mean exactly that people start planning to abuse it
> even before it's merged.

That quote (from yours truly) states the opposite.

The whole point is that there are several participants in the process,
with independent release cycles and policies, but with a common
"registry" of supported stats (with the master copy being in the kernel,
right?).  Once a counter is accepted there, you can start shipping the
guest driver providing it, and you don't have to wait until qemu catches
up: your management level can read "x-stat-NEW_NUMBER" *or* "new_name",
as both NEW_NUMBER and new_name are now allocated for that new counter.

So yes, people are planning to use it, in particluar, before it's merged
into qemu proper, but I don't see how that creates any extra maintenance
burden on qemu side.  Anybody using x- is on their own; the scheme I
sketched seems reasonably safe but is the headache of that management
software anyway.

Roman.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Michael S. Tsirkin]

On Thu, Feb 25, 2016 at 12:30:21PM +0300, Roman Kagan wrote:
> On Thu, Feb 25, 2016 at 10:54:17AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Feb 25, 2016 at 09:44:06AM +0100, Markus Armbruster wrote:
> > > "Denis V. Lunev" <den@openvz.org> writes:
> > > 
> > > > On 02/24/2016 06:43 PM, Eric Blake wrote:
> > > >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
> > > >>> Roman Kagan <rkagan@virtuozzo.com> writes:
> > > >>>> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
> > > >>>>> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
> > > >>>>> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> > > >>>>> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> > > >>>>> > >>From: Igor Redko <redkoi@virtuozzo.com>
> > > >>>>> > >>
> > > >>>>> > >>We are making experiments with different autoballooning strategies
> > > >>>>> > >>based on the guest behavior. Thus we need to experiment with different
> > > >>>>> > >>guest statistics. For now every counter change requires QEMU recompilation
> > > >>>>> > >>and dances with Libvirt.
> > > >>>>> > >>
> > > >>>>> > >>This patch introduces transport for unrecognized counters in virtio-balloon.
> > > >>>>> > >>This transport can be used for measuring benefits from using new
> > > >>>>> > >>balloon counters, before submitting any patches. Current alternative
> > > >>>>> > >>is 'guest-exec' transport which isn't made for such delicate matters
> > > >>>>> > >>and can influence test results.
> > > >>>>> > >>
> > > >>>>> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> > > >>>>> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> > > >>>>> > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> > > >>>>> > >>where XXXX is a tag number in hex. Defined counters are reported with their
> > > >>>>> > >>regular names.
> > > >>>>> > >>
> > > >>>>> > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
> > > >>>>> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> > > >>>>> > >>CC: Michael S. Tsirkin <mst@redhat.com>
> > > >>>>> > >This seems to open the ABI to abuse.
> > > >>>>> > >Seems like a reasonable way to experiment though.
> > > >>>>> > >How about adding this within #if 0 statements?
> > > >>>>> > >You can uncomment them for debugging ...
> > > >>>>> > I'd prefer to have this enabled.
> > > 
> > > Yes, conditional compilation should be used sparingly.  I don't have an
> > > opinion on whether using it here is appropriate.
> > > 
> > > >>>>> > Why do you think that it opens "abuse" way?
> > > >>>>> 
> > > >>>>> Because people will use this to hack drivers and management tools
> > > >>>>> bypassing qemu.
> > > 
> > > Easy to avoid: shuffle the N in x-stat-N around from time to time, to
> > > reinforce the lesson that you must not rely on their presence or
> > > semantics.  I doubt it'll be necessary beyond the renumbering that
> > > happens naturally when we add supported counters, or the reshuffling
> > > that happens when somebody messes with the unsupported counters.
> > > 
> > > >>>> I'm curious why you think it's a problem?  Even the existing stats are
> > > >>>> simply propagated to the management level by qemu with no processing
> > > >>>> other than assigning text labels.  The proposed naming scheme for
> > > >>>> unrecognized counters includes "x-" prefix which explicitly marks them
> > > >>>> as unstable so people using them take their risk.
> > > >>>>
> > > >>>> One of the benefits is forward compatibility, so that counters that have
> > > >>>> graduated into supported ones and have got their own number and name,
> > > >>>> can be made to work with qemu that doesn't yet recognize them.
> > > >>> Then management does start relying on the x- prefixed things,
> > > >>> and once it's used to that it's a slippery slope.
> > > >> Any management tool that relies on an x- prefix name is broken.
> > > 
> > > Or at least assumes the full risk of breaking without notice whenever
> > > QEMU changes.  Abbreviating that to just "broken" seems fair enough :)
> > > 
> > > >>                                                                  We've
> > > >> explicitly documented that the x- prefix is unstable and liable to go
> > > >> away with a future release. Any management app that wants to use a
> > > >> feature beginning with x- should FIRST push hard to get the x- removed
> > > >> and stabilize the interface (and libvirt, at least, does just that).
> > > >>
> > > > this was exactly an original idea. Names started with 'x-' are
> > > > _officially_ unstable and for debug purpose. That is why I'd
> > > > prefer if v2 of the patchset will be taken.
> > > 
> > > Looks like fair use of x- to me.
> > 
> > 
> > Well I already heard:
> > 
> > 	One of the benefits is forward compatibility, so that counters that have
> > 	graduated into supported ones and have got their own number and name,
> > 	can be made to work with qemu that doesn't yet recognize them.
> > 
> > in this thread, which seems to mean exactly that people start planning to abuse it
> > even before it's merged.
> 
> That quote (from yours truly) states the opposite.
> 
> The whole point is that there are several participants in the process,
> with independent release cycles and policies, but with a common
> "registry" of supported stats (with the master copy being in the kernel,
> right?).

For most devices it's the virtio spec.

> Once a counter is accepted there, you can start shipping the
> guest driver providing it, and you don't have to wait until qemu catches
> up: your management level can read "x-stat-NEW_NUMBER" *or* "new_name",
> as both NEW_NUMBER and new_name are now allocated for that new counter.
> 
> So yes, people are planning to use it, in particluar, before it's merged
> into qemu proper, but I don't see how that creates any extra maintenance
> burden on qemu side.  Anybody using x- is on their own; the scheme I
> sketched seems reasonably safe but is the headache of that management
> software anyway.
> 
> Roman.

Basically if you do this hack QEMU must not reuse the x-stat-NEW_NUMBER
ever, otherwise management handling it will intepret it
as legacy QEMU and will break.

And the fact we have to argue about it tells me this is
a dangerous place to put the debugging hooks since it
seems to be begging to be misused.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Feb 25, 2016 at 12:30:21PM +0300, Roman Kagan wrote:
>> On Thu, Feb 25, 2016 at 10:54:17AM +0200, Michael S. Tsirkin wrote:
>> > On Thu, Feb 25, 2016 at 09:44:06AM +0100, Markus Armbruster wrote:
>> > > "Denis V. Lunev" <den@openvz.org> writes:
>> > > 
>> > > > On 02/24/2016 06:43 PM, Eric Blake wrote:
>> > > >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
>> > > >>> Roman Kagan <rkagan@virtuozzo.com> writes:
>> > > >>>> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
>> > > >>>>> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
>> > > >>>>> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
>> > > >>>>> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
>> > > >>>>> > >>From: Igor Redko <redkoi@virtuozzo.com>
>> > > >>>>> > >>
>> > > >>>>> > >>We are making experiments with different autoballooning strategies
>> > > >>>>> > >>based on the guest behavior. Thus we need to experiment with different
>> > > >>>>> > >>guest statistics. For now every counter change requires QEMU recompilation
>> > > >>>>> > >>and dances with Libvirt.
>> > > >>>>> > >>
>> > > >>>>> > >>This patch introduces transport for unrecognized counters in virtio-balloon.
>> > > >>>>> > >>This transport can be used for measuring benefits from using new
>> > > >>>>> > >>balloon counters, before submitting any patches. Current alternative
>> > > >>>>> > >>is 'guest-exec' transport which isn't made for such delicate matters
>> > > >>>>> > >>and can influence test results.
>> > > >>>>> > >>
>> > > >>>>> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
>> > > >>>>> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
>> > > >>>>> > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
>> > > >>>>> > >>where XXXX is a tag number in hex. Defined counters are reported with their
>> > > >>>>> > >>regular names.
>> > > >>>>> > >>
>> > > >>>>> > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
>> > > >>>>> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
>> > > >>>>> > >>CC: Michael S. Tsirkin <mst@redhat.com>
>> > > >>>>> > >This seems to open the ABI to abuse.
>> > > >>>>> > >Seems like a reasonable way to experiment though.
>> > > >>>>> > >How about adding this within #if 0 statements?
>> > > >>>>> > >You can uncomment them for debugging ...
>> > > >>>>> > I'd prefer to have this enabled.
>> > > 
>> > > Yes, conditional compilation should be used sparingly.  I don't have an
>> > > opinion on whether using it here is appropriate.
>> > > 
>> > > >>>>> > Why do you think that it opens "abuse" way?
>> > > >>>>> 
>> > > >>>>> Because people will use this to hack drivers and management tools
>> > > >>>>> bypassing qemu.
>> > > 
>> > > Easy to avoid: shuffle the N in x-stat-N around from time to time, to
>> > > reinforce the lesson that you must not rely on their presence or
>> > > semantics.  I doubt it'll be necessary beyond the renumbering that
>> > > happens naturally when we add supported counters, or the reshuffling
>> > > that happens when somebody messes with the unsupported counters.
>> > > 
>> > > >>>> I'm curious why you think it's a problem?  Even the existing stats are
>> > > >>>> simply propagated to the management level by qemu with no processing
>> > > >>>> other than assigning text labels.  The proposed naming scheme for
>> > > >>>> unrecognized counters includes "x-" prefix which explicitly marks them
>> > > >>>> as unstable so people using them take their risk.
>> > > >>>>
>> > > >>>> One of the benefits is forward compatibility, so that counters that have
>> > > >>>> graduated into supported ones and have got their own number and name,
>> > > >>>> can be made to work with qemu that doesn't yet recognize them.
>> > > >>> Then management does start relying on the x- prefixed things,
>> > > >>> and once it's used to that it's a slippery slope.
>> > > >> Any management tool that relies on an x- prefix name is broken.
>> > > 
>> > > Or at least assumes the full risk of breaking without notice whenever
>> > > QEMU changes.  Abbreviating that to just "broken" seems fair enough :)
>> > > 
>> > > >>                                                                  We've
>> > > >> explicitly documented that the x- prefix is unstable and liable to go
>> > > >> away with a future release. Any management app that wants to use a
>> > > >> feature beginning with x- should FIRST push hard to get the x- removed
>> > > >> and stabilize the interface (and libvirt, at least, does just that).
>> > > >>
>> > > > this was exactly an original idea. Names started with 'x-' are
>> > > > _officially_ unstable and for debug purpose. That is why I'd
>> > > > prefer if v2 of the patchset will be taken.
>> > > 
>> > > Looks like fair use of x- to me.
>> > 
>> > 
>> > Well I already heard:
>> > 
>> > 	One of the benefits is forward compatibility, so that counters that have
>> > 	graduated into supported ones and have got their own number and name,
>> > 	can be made to work with qemu that doesn't yet recognize them.
>> > 
>> > in this thread, which seems to mean exactly that people start planning to abuse it
>> > even before it's merged.
>> 
>> That quote (from yours truly) states the opposite.
>> 
>> The whole point is that there are several participants in the process,
>> with independent release cycles and policies, but with a common
>> "registry" of supported stats (with the master copy being in the kernel,
>> right?).
>
> For most devices it's the virtio spec.
>
>> Once a counter is accepted there, you can start shipping the
>> guest driver providing it, and you don't have to wait until qemu catches
>> up: your management level can read "x-stat-NEW_NUMBER" *or* "new_name",
>> as both NEW_NUMBER and new_name are now allocated for that new counter.
>> 
>> So yes, people are planning to use it, in particluar, before it's merged
>> into qemu proper, but I don't see how that creates any extra maintenance
>> burden on qemu side.  Anybody using x- is on their own; the scheme I
>> sketched seems reasonably safe but is the headache of that management
>> software anyway.
>> 
>> Roman.
>
> Basically if you do this hack QEMU must not reuse the x-stat-NEW_NUMBER
> ever, otherwise management handling it will intepret it
> as legacy QEMU and will break.

No, QEMU should aggressively reuse the number part.  Heck, it's free to
randomly mess with it without notice.  Makes the x-stat-N effectively
useless for anything but experimenting.  Which is exactly the point of
naming them x-.

> And the fact we have to argue about it tells me this is
> a dangerous place to put the debugging hooks since it
> seems to be begging to be misused.

x-things always look like they'd invite misuse.  That look goes away
fast when your misuse breaks on every other QEMU upgrade.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Roman Kagan]

On Thu, Feb 25, 2016 at 11:46:59AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Feb 25, 2016 at 12:30:21PM +0300, Roman Kagan wrote:
> >> On Thu, Feb 25, 2016 at 10:54:17AM +0200, Michael S. Tsirkin wrote:
> >> > On Thu, Feb 25, 2016 at 09:44:06AM +0100, Markus Armbruster wrote:
> >> > > "Denis V. Lunev" <den@openvz.org> writes:
> >> > > 
> >> > > > On 02/24/2016 06:43 PM, Eric Blake wrote:
> >> > > >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
> >> > > >>> Roman Kagan <rkagan@virtuozzo.com> writes:
> >> > > >>>> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
> >> > > >>>>> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
> >> > > >>>>> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> >> > > >>>>> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> >> > > >>>>> > >>From: Igor Redko <redkoi@virtuozzo.com>
> >> > > >>>>> > >>
> >> > > >>>>> > >>We are making experiments with different autoballooning strategies
> >> > > >>>>> > >>based on the guest behavior. Thus we need to experiment with different
> >> > > >>>>> > >>guest statistics. For now every counter change requires QEMU recompilation
> >> > > >>>>> > >>and dances with Libvirt.
> >> > > >>>>> > >>
> >> > > >>>>> > >>This patch introduces transport for unrecognized counters in virtio-balloon.
> >> > > >>>>> > >>This transport can be used for measuring benefits from using new
> >> > > >>>>> > >>balloon counters, before submitting any patches. Current alternative
> >> > > >>>>> > >>is 'guest-exec' transport which isn't made for such delicate matters
> >> > > >>>>> > >>and can influence test results.
> >> > > >>>>> > >>
> >> > > >>>>> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
> >> > > >>>>> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
> >> > > >>>>> > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
> >> > > >>>>> > >>where XXXX is a tag number in hex. Defined counters are reported with their
> >> > > >>>>> > >>regular names.
> >> > > >>>>> > >>
> >> > > >>>>> > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
> >> > > >>>>> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >> > > >>>>> > >>CC: Michael S. Tsirkin <mst@redhat.com>
> >> > > >>>>> > >This seems to open the ABI to abuse.
> >> > > >>>>> > >Seems like a reasonable way to experiment though.
> >> > > >>>>> > >How about adding this within #if 0 statements?
> >> > > >>>>> > >You can uncomment them for debugging ...
> >> > > >>>>> > I'd prefer to have this enabled.
> >> > > 
> >> > > Yes, conditional compilation should be used sparingly.  I don't have an
> >> > > opinion on whether using it here is appropriate.
> >> > > 
> >> > > >>>>> > Why do you think that it opens "abuse" way?
> >> > > >>>>> 
> >> > > >>>>> Because people will use this to hack drivers and management tools
> >> > > >>>>> bypassing qemu.
> >> > > 
> >> > > Easy to avoid: shuffle the N in x-stat-N around from time to time, to
> >> > > reinforce the lesson that you must not rely on their presence or
> >> > > semantics.  I doubt it'll be necessary beyond the renumbering that
> >> > > happens naturally when we add supported counters, or the reshuffling
> >> > > that happens when somebody messes with the unsupported counters.
> >> > > 
> >> > > >>>> I'm curious why you think it's a problem?  Even the existing stats are
> >> > > >>>> simply propagated to the management level by qemu with no processing
> >> > > >>>> other than assigning text labels.  The proposed naming scheme for
> >> > > >>>> unrecognized counters includes "x-" prefix which explicitly marks them
> >> > > >>>> as unstable so people using them take their risk.
> >> > > >>>>
> >> > > >>>> One of the benefits is forward compatibility, so that counters that have
> >> > > >>>> graduated into supported ones and have got their own number and name,
> >> > > >>>> can be made to work with qemu that doesn't yet recognize them.
> >> > > >>> Then management does start relying on the x- prefixed things,
> >> > > >>> and once it's used to that it's a slippery slope.
> >> > > >> Any management tool that relies on an x- prefix name is broken.
> >> > > 
> >> > > Or at least assumes the full risk of breaking without notice whenever
> >> > > QEMU changes.  Abbreviating that to just "broken" seems fair enough :)
> >> > > 
> >> > > >>                                                                  We've
> >> > > >> explicitly documented that the x- prefix is unstable and liable to go
> >> > > >> away with a future release. Any management app that wants to use a
> >> > > >> feature beginning with x- should FIRST push hard to get the x- removed
> >> > > >> and stabilize the interface (and libvirt, at least, does just that).
> >> > > >>
> >> > > > this was exactly an original idea. Names started with 'x-' are
> >> > > > _officially_ unstable and for debug purpose. That is why I'd
> >> > > > prefer if v2 of the patchset will be taken.
> >> > > 
> >> > > Looks like fair use of x- to me.
> >> > 
> >> > 
> >> > Well I already heard:
> >> > 
> >> > 	One of the benefits is forward compatibility, so that counters that have
> >> > 	graduated into supported ones and have got their own number and name,
> >> > 	can be made to work with qemu that doesn't yet recognize them.
> >> > 
> >> > in this thread, which seems to mean exactly that people start planning to abuse it
> >> > even before it's merged.
> >> 
> >> That quote (from yours truly) states the opposite.
> >> 
> >> The whole point is that there are several participants in the process,
> >> with independent release cycles and policies, but with a common
> >> "registry" of supported stats (with the master copy being in the kernel,
> >> right?).
> >
> > For most devices it's the virtio spec.
> >
> >> Once a counter is accepted there, you can start shipping the
> >> guest driver providing it, and you don't have to wait until qemu catches
> >> up: your management level can read "x-stat-NEW_NUMBER" *or* "new_name",
> >> as both NEW_NUMBER and new_name are now allocated for that new counter.
> >> 
> >> So yes, people are planning to use it, in particluar, before it's merged
> >> into qemu proper, but I don't see how that creates any extra maintenance
> >> burden on qemu side.  Anybody using x- is on their own; the scheme I
> >> sketched seems reasonably safe but is the headache of that management
> >> software anyway.
> >> 
> >> Roman.
> >
> > Basically if you do this hack QEMU must not reuse the x-stat-NEW_NUMBER
> > ever, otherwise management handling it will intepret it
> > as legacy QEMU and will break.
> 
> No, QEMU should aggressively reuse the number part.  Heck, it's free to
> randomly mess with it without notice.  Makes the x-stat-N effectively
> useless for anything but experimenting.  Which is exactly the point of
> naming them x-.

I must be missing something...  QEMU has no business with the number at
all unless it recognizes it; in that case it only replaces the dumb
x-stat-N label with the one it knows.  How can it "randomly mess with
it"?

Let's get this straight: the only thing QEMU does with balloon stats is
marshalling them into json.  (For that matter, libvirt only unmarshalls
them back into an array of (int tag, long value) pairs so is similar.)
Basically QEMU only plays the role of transport for memory stats between
the guest driver and the management layer; I'm not even sure why it has
to know what the individual fields mean.  What this patch proposes is
essentially to make QEMU not stand in the way of the data it transports;
it's only the endpoints' responsibility to agree on the interpretation
of the contents.

Roman.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Markus Armbruster]

Roman Kagan <rkagan@virtuozzo.com> writes:

> On Thu, Feb 25, 2016 at 11:46:59AM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Thu, Feb 25, 2016 at 12:30:21PM +0300, Roman Kagan wrote:
>> >> On Thu, Feb 25, 2016 at 10:54:17AM +0200, Michael S. Tsirkin wrote:
>> >> > On Thu, Feb 25, 2016 at 09:44:06AM +0100, Markus Armbruster wrote:
>> >> > > "Denis V. Lunev" <den@openvz.org> writes:
>> >> > > 
>> >> > > > On 02/24/2016 06:43 PM, Eric Blake wrote:
>> >> > > >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
>> >> > > >>> Roman Kagan <rkagan@virtuozzo.com> writes:
>> >> > > >>>> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
>> >> > > >>>>> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
>> >> > > >>>>> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
>> >> > > >>>>> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
>> >> > > >>>>> > >>From: Igor Redko <redkoi@virtuozzo.com>
>> >> > > >>>>> > >>
>> >> > > >>>>> > >>We are making experiments with different autoballooning strategies
>> >> > > >>>>> > >>based on the guest behavior. Thus we need to experiment with different
>> >> > > >>>>> > >>guest statistics. For now every counter change requires QEMU recompilation
>> >> > > >>>>> > >>and dances with Libvirt.
>> >> > > >>>>> > >>
>> >> > > >>>>> > >>This patch introduces transport for unrecognized counters in virtio-balloon.
>> >> > > >>>>> > >>This transport can be used for measuring benefits from using new
>> >> > > >>>>> > >>balloon counters, before submitting any patches. Current alternative
>> >> > > >>>>> > >>is 'guest-exec' transport which isn't made for such delicate matters
>> >> > > >>>>> > >>and can influence test results.
>> >> > > >>>>> > >>
>> >> > > >>>>> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
>> >> > > >>>>> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
>> >> > > >>>>> > >>queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
>> >> > > >>>>> > >>where XXXX is a tag number in hex. Defined counters are reported with their
>> >> > > >>>>> > >>regular names.
>> >> > > >>>>> > >>
>> >> > > >>>>> > >>Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
>> >> > > >>>>> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
>> >> > > >>>>> > >>CC: Michael S. Tsirkin <mst@redhat.com>
>> >> > > >>>>> > >This seems to open the ABI to abuse.
>> >> > > >>>>> > >Seems like a reasonable way to experiment though.
>> >> > > >>>>> > >How about adding this within #if 0 statements?
>> >> > > >>>>> > >You can uncomment them for debugging ...
>> >> > > >>>>> > I'd prefer to have this enabled.
>> >> > > 
>> >> > > Yes, conditional compilation should be used sparingly.  I don't have an
>> >> > > opinion on whether using it here is appropriate.
>> >> > > 
>> >> > > >>>>> > Why do you think that it opens "abuse" way?
>> >> > > >>>>> 
>> >> > > >>>>> Because people will use this to hack drivers and management tools
>> >> > > >>>>> bypassing qemu.
>> >> > > 
>> >> > > Easy to avoid: shuffle the N in x-stat-N around from time to time, to
>> >> > > reinforce the lesson that you must not rely on their presence or
>> >> > > semantics.  I doubt it'll be necessary beyond the renumbering that
>> >> > > happens naturally when we add supported counters, or the reshuffling
>> >> > > that happens when somebody messes with the unsupported counters.
>> >> > > 
>> >> > > >>>> I'm curious why you think it's a problem?  Even the existing stats are
>> >> > > >>>> simply propagated to the management level by qemu with no processing
>> >> > > >>>> other than assigning text labels.  The proposed naming scheme for
>> >> > > >>>> unrecognized counters includes "x-" prefix which explicitly marks them
>> >> > > >>>> as unstable so people using them take their risk.
>> >> > > >>>>
>> >> > > >>>> One of the benefits is forward compatibility, so that counters that have
>> >> > > >>>> graduated into supported ones and have got their own number and name,
>> >> > > >>>> can be made to work with qemu that doesn't yet recognize them.
>> >> > > >>> Then management does start relying on the x- prefixed things,
>> >> > > >>> and once it's used to that it's a slippery slope.
>> >> > > >> Any management tool that relies on an x- prefix name is broken.
>> >> > > 
>> >> > > Or at least assumes the full risk of breaking without notice whenever
>> >> > > QEMU changes.  Abbreviating that to just "broken" seems fair enough :)
>> >> > > 
>> >> > > >>                                                                  We've
>> >> > > >> explicitly documented that the x- prefix is unstable and liable to go
>> >> > > >> away with a future release. Any management app that wants to use a
>> >> > > >> feature beginning with x- should FIRST push hard to get the x- removed
>> >> > > >> and stabilize the interface (and libvirt, at least, does just that).
>> >> > > >>
>> >> > > > this was exactly an original idea. Names started with 'x-' are
>> >> > > > _officially_ unstable and for debug purpose. That is why I'd
>> >> > > > prefer if v2 of the patchset will be taken.
>> >> > > 
>> >> > > Looks like fair use of x- to me.
>> >> > 
>> >> > 
>> >> > Well I already heard:
>> >> > 
>> >> > 	One of the benefits is forward compatibility, so that counters that have
>> >> > 	graduated into supported ones and have got their own number and name,
>> >> > 	can be made to work with qemu that doesn't yet recognize them.
>> >> > 
>> >> > in this thread, which seems to mean exactly that people start planning to abuse it
>> >> > even before it's merged.
>> >> 
>> >> That quote (from yours truly) states the opposite.
>> >> 
>> >> The whole point is that there are several participants in the process,
>> >> with independent release cycles and policies, but with a common
>> >> "registry" of supported stats (with the master copy being in the kernel,
>> >> right?).
>> >
>> > For most devices it's the virtio spec.
>> >
>> >> Once a counter is accepted there, you can start shipping the
>> >> guest driver providing it, and you don't have to wait until qemu catches
>> >> up: your management level can read "x-stat-NEW_NUMBER" *or* "new_name",
>> >> as both NEW_NUMBER and new_name are now allocated for that new counter.
>> >> 
>> >> So yes, people are planning to use it, in particluar, before it's merged
>> >> into qemu proper, but I don't see how that creates any extra maintenance
>> >> burden on qemu side.  Anybody using x- is on their own; the scheme I
>> >> sketched seems reasonably safe but is the headache of that management
>> >> software anyway.
>> >> 
>> >> Roman.
>> >
>> > Basically if you do this hack QEMU must not reuse the x-stat-NEW_NUMBER
>> > ever, otherwise management handling it will intepret it
>> > as legacy QEMU and will break.
>> 
>> No, QEMU should aggressively reuse the number part.  Heck, it's free to
>> randomly mess with it without notice.  Makes the x-stat-N effectively
>> useless for anything but experimenting.  Which is exactly the point of
>> naming them x-.
>
> I must be missing something...  QEMU has no business with the number at
> all unless it recognizes it; in that case it only replaces the dumb
> x-stat-N label with the one it knows.  How can it "randomly mess with
> it"?
>
> Let's get this straight: the only thing QEMU does with balloon stats is
> marshalling them into json.  (For that matter, libvirt only unmarshalls
> them back into an array of (int tag, long value) pairs so is similar.)
> Basically QEMU only plays the role of transport for memory stats between
> the guest driver and the management layer; I'm not even sure why it has
> to know what the individual fields mean.  What this patch proposes is
> essentially to make QEMU not stand in the way of the data it transports;
> it's only the endpoints' responsibility to agree on the interpretation
> of the contents.

If you want to propose a stable interface, don't use the x- prefix.
That's for unstable stuff.  x- like experimental.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Roman Kagan]

On Thu, Feb 25, 2016 at 12:58:08PM +0100, Markus Armbruster wrote:
> Roman Kagan <rkagan@virtuozzo.com> writes:
> > On Thu, Feb 25, 2016 at 11:46:59AM +0100, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> > On Thu, Feb 25, 2016 at 12:30:21PM +0300, Roman Kagan wrote:
> >> >> The whole point is that there are several participants in the process,
> >> >> with independent release cycles and policies, but with a common
> >> >> "registry" of supported stats (with the master copy being in the kernel,
> >> >> right?).
> >> >
> >> > For most devices it's the virtio spec.
> >> >
> >> >> Once a counter is accepted there, you can start shipping the
> >> >> guest driver providing it, and you don't have to wait until qemu catches
> >> >> up: your management level can read "x-stat-NEW_NUMBER" *or* "new_name",
> >> >> as both NEW_NUMBER and new_name are now allocated for that new counter.
> >> >> 
> >> >> So yes, people are planning to use it, in particluar, before it's merged
> >> >> into qemu proper, but I don't see how that creates any extra maintenance
> >> >> burden on qemu side.  Anybody using x- is on their own; the scheme I
> >> >> sketched seems reasonably safe but is the headache of that management
> >> >> software anyway.
> >> >> 
> >> >> Roman.
> >> >
> >> > Basically if you do this hack QEMU must not reuse the x-stat-NEW_NUMBER
> >> > ever, otherwise management handling it will intepret it
> >> > as legacy QEMU and will break.
> >> 
> >> No, QEMU should aggressively reuse the number part.  Heck, it's free to
> >> randomly mess with it without notice.  Makes the x-stat-N effectively
> >> useless for anything but experimenting.  Which is exactly the point of
> >> naming them x-.
> >
> > I must be missing something...  QEMU has no business with the number at
> > all unless it recognizes it; in that case it only replaces the dumb
> > x-stat-N label with the one it knows.  How can it "randomly mess with
> > it"?
> >
> > Let's get this straight: the only thing QEMU does with balloon stats is
> > marshalling them into json.  (For that matter, libvirt only unmarshalls
> > them back into an array of (int tag, long value) pairs so is similar.)
> > Basically QEMU only plays the role of transport for memory stats between
> > the guest driver and the management layer; I'm not even sure why it has
> > to know what the individual fields mean.  What this patch proposes is
> > essentially to make QEMU not stand in the way of the data it transports;
> > it's only the endpoints' responsibility to agree on the interpretation
> > of the contents.
> 
> If you want to propose a stable interface, don't use the x- prefix.
> That's for unstable stuff.  x- like experimental.

We wanted to remain compatible with the existing query-balloon, which
was already designed with named fields.

Do you think we'd better introduce instead a new monitor command that
would make QEMU just consistently marshall whatever the guest balloon
sent without interpreting, e.g. query-balloon-raw?  Or just drop x- and
declare that all new fields in balloon stats will have stat-N names when
marshalled by QEMU?

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Markus Armbruster]

Roman Kagan <rkagan@virtuozzo.com> writes:

> On Thu, Feb 25, 2016 at 12:58:08PM +0100, Markus Armbruster wrote:
>> Roman Kagan <rkagan@virtuozzo.com> writes:
>> > On Thu, Feb 25, 2016 at 11:46:59AM +0100, Markus Armbruster wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >> > On Thu, Feb 25, 2016 at 12:30:21PM +0300, Roman Kagan wrote:
>> >> >> The whole point is that there are several participants in the process,
>> >> >> with independent release cycles and policies, but with a common
>> >> >> "registry" of supported stats (with the master copy being in the kernel,
>> >> >> right?).
>> >> >
>> >> > For most devices it's the virtio spec.
>> >> >
>> >> >> Once a counter is accepted there, you can start shipping the
>> >> >> guest driver providing it, and you don't have to wait until qemu catches
>> >> >> up: your management level can read "x-stat-NEW_NUMBER" *or* "new_name",
>> >> >> as both NEW_NUMBER and new_name are now allocated for that new counter.
>> >> >> 
>> >> >> So yes, people are planning to use it, in particluar, before it's merged
>> >> >> into qemu proper, but I don't see how that creates any extra maintenance
>> >> >> burden on qemu side.  Anybody using x- is on their own; the scheme I
>> >> >> sketched seems reasonably safe but is the headache of that management
>> >> >> software anyway.
>> >> >> 
>> >> >> Roman.
>> >> >
>> >> > Basically if you do this hack QEMU must not reuse the x-stat-NEW_NUMBER
>> >> > ever, otherwise management handling it will intepret it
>> >> > as legacy QEMU and will break.
>> >> 
>> >> No, QEMU should aggressively reuse the number part.  Heck, it's free to
>> >> randomly mess with it without notice.  Makes the x-stat-N effectively
>> >> useless for anything but experimenting.  Which is exactly the point of
>> >> naming them x-.
>> >
>> > I must be missing something...  QEMU has no business with the number at
>> > all unless it recognizes it; in that case it only replaces the dumb
>> > x-stat-N label with the one it knows.  How can it "randomly mess with
>> > it"?
>> >
>> > Let's get this straight: the only thing QEMU does with balloon stats is
>> > marshalling them into json.  (For that matter, libvirt only unmarshalls
>> > them back into an array of (int tag, long value) pairs so is similar.)
>> > Basically QEMU only plays the role of transport for memory stats between
>> > the guest driver and the management layer; I'm not even sure why it has
>> > to know what the individual fields mean.  What this patch proposes is
>> > essentially to make QEMU not stand in the way of the data it transports;
>> > it's only the endpoints' responsibility to agree on the interpretation
>> > of the contents.
>> 
>> If you want to propose a stable interface, don't use the x- prefix.
>> That's for unstable stuff.  x- like experimental.
>
> We wanted to remain compatible with the existing query-balloon, which
> was already designed with named fields.
>
> Do you think we'd better introduce instead a new monitor command that
> would make QEMU just consistently marshall whatever the guest balloon
> sent without interpreting, e.g. query-balloon-raw?  Or just drop x- and
> declare that all new fields in balloon stats will have stat-N names when
> marshalled by QEMU?

Now I'm confused.  According to virtio-balloon-stats.txt, they're
exposed as virtio-balloon properties in QOM.  I don't understand how
these are related to query-balloon, nor what a new monitor command
query-balloon-raw would be good for.

You seem to propose adding stats unknown to QEMU with numeric names.  If
there's an authority mapping numbers to meaning, the numbers so mapped
would be suitable as stable interface.  However, for the numbers QEMU
knows to be mapped, it surely knows names, too, so limiting the thing to
such numbers would be pointless.

The only way you can make numbers QEMU doesn't know a stable interface
is declaring the interface a mere transport between guest and management
application.  That's a whole different can of worms.  I have no opinion
whether opening it here is a good idea.

Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Roman Kagan]

On Thu, Feb 25, 2016 at 03:10:09PM +0100, Markus Armbruster wrote:
> Roman Kagan <rkagan@virtuozzo.com> writes:
> Now I'm confused.  According to virtio-balloon-stats.txt, they're
> exposed as virtio-balloon properties in QOM.  I don't understand how
> these are related to query-balloon, nor what a new monitor command
> query-balloon-raw would be good for.

Oops, sorry, you're right, I mixed it all up: query-balloon only reports
how big the balloon is currently inflated, while the stats are indeed
exposed as object properties.  So query-balloon-raw makes no sense.

> You seem to propose adding stats unknown to QEMU with numeric names.  If
> there's an authority mapping numbers to meaning, the numbers so mapped
> would be suitable as stable interface.  However, for the numbers QEMU
> knows to be mapped, it surely knows names, too, so limiting the thing to
> such numbers would be pointless.
> 
> The only way you can make numbers QEMU doesn't know a stable interface
> is declaring the interface a mere transport between guest and management
> application.

But that's what it currently is.

Roman.

[Qemu-devel] [PATCH v3 0/2] virtio-balloon: improve balloon statistics

[Denis V. Lunev]

New counter from the Linux kernel + generic framework to pass currently
unknown counters via QMP for debug purposes.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Igor Redko <redkoi@virtuozzo.com>
CC: Michael S. Tsirkin <mst@redhat.com>

Changes from v2:
- put "export unknown counters" code under ifdef

Changes from v1:
- removed !err in patch 1 by suggestion from Eric

[Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Denis V. Lunev]

From: Igor Redko <redkoi@virtuozzo.com>

We are making experiments with different autoballooning strategies
based on the guest behavior. Thus we need to experiment with different
guest statistics. For now every counter change requires QEMU recompilation
and dances with Libvirt.

This patch introduces transport for unrecognized counters in virtio-balloon.
This transport can be used for measuring benefits from using new
balloon counters, before submitting any patches. Current alternative
is 'guest-exec' transport which isn't made for such delicate matters
and can influence test results.

Originally all counters with tag >= VIRTIO_BALLOON_S_NR were ignored.
Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters from the
queue and pass unrecognized ones with the following names: 'x-stat-XXXX',
where XXXX is a tag number in hex. Defined counters are reported with their
regular names.

Signed-off-by: Igor Redko <redkoi@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 configure                          | 12 ++++++++++++
 hw/virtio/virtio-balloon.c         | 32 ++++++++++++++++++++++++++------
 include/hw/virtio/virtio-balloon.h |  3 ++-
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/configure b/configure
index 0aa249b..a51b85c 100755
--- a/configure
+++ b/configure
@@ -314,6 +314,7 @@ vhdx=""
 numa=""
 tcmalloc="no"
 jemalloc="no"
+unknown_balloon_stats="no"
 
 # parse CC options first
 for opt do
@@ -1141,6 +1142,10 @@ for opt do
   ;;
   --enable-jemalloc) jemalloc="yes"
   ;;
+  --enable-unknown-balloon-stats) unknown_balloon_stats="yes"
+  ;;
+  --disable-unknown-balloon-stats) unknown_balloon_stats="no"
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1363,6 +1368,8 @@ disabled with --disable-FEATURE, default is enabled if available:
   numa            libnuma support
   tcmalloc        tcmalloc support
   jemalloc        jemalloc support
+  unknown-balloon-stats  report unknown balloon statistics counters
+  ;;
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -4776,6 +4783,7 @@ echo "bzip2 support     $bzip2"
 echo "NUMA host support $numa"
 echo "tcmalloc support  $tcmalloc"
 echo "jemalloc support  $jemalloc"
+echo "unknown balloon stat counters support  $unknown_balloon_stats"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -5324,6 +5332,10 @@ if test "$rdma" = "yes" ; then
   echo "CONFIG_RDMA=y" >> $config_host_mak
 fi
 
+if test "$unknown_balloon_stats" = "yes" ; then
+  echo "CONFIG_UNKNOWN_BALLOON_STATS=y" >> $config_host_mak
+fi
+
 # Hold two types of flag:
 #   CONFIG_THREAD_SETNAME_BYTHREAD  - we've got a way of setting the name on
 #                                     a thread we have a handle to
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a382f43..6629145 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -66,8 +66,7 @@ static const char *balloon_stat_names[] = {
  */
 static inline void reset_stats(VirtIOBalloon *dev)
 {
-    int i;
-    for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
+    dev->stats_cnt = 0;
 }
 
 static bool balloon_stats_supported(const VirtIOBalloon *s)
@@ -133,12 +132,22 @@ static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
     if (err) {
         goto out_end;
     }
-    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
-        visit_type_uint64(v, balloon_stat_names[i], &s->stats[i], &err);
+    for (i = 0; i < s->stats_cnt; i++) {
+        if (s->stats[i].tag < VIRTIO_BALLOON_S_NR) {
+            visit_type_uint64(v, balloon_stat_names[s->stats[i].tag],
+                              &s->stats[i].val, &err);
+        } else {
+#if defined(CONFIG_UNKNOWN_BALLOON_STATS)
+            gchar *str = g_strdup_printf("x-stat-%04x", s->stats[i].tag);
+            visit_type_uint64(v, str, &s->stats[i].val, &err);
+            g_free(str);
+#endif
+        }
         if (err) {
             break;
         }
     }
+
     error_propagate(errp, err);
     err = NULL;
     visit_end_struct(v, &err);
@@ -273,10 +282,21 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
            == sizeof(stat)) {
         uint16_t tag = virtio_tswap16(vdev, stat.tag);
         uint64_t val = virtio_tswap64(vdev, stat.val);
+        int i;
 
         offset += sizeof(stat);
-        if (tag < VIRTIO_BALLOON_S_NR)
-            s->stats[tag] = val;
+        for (i = 0; i < s->stats_cnt; i++) {
+            if (s->stats[i].tag == tag) {
+                break;
+            }
+        }
+        if (i < ARRAY_SIZE(s->stats)) {
+            s->stats[i].tag = tag;
+            s->stats[i].val = val;
+            if (s->stats_cnt <= i) {
+                s->stats_cnt = i + 1;
+            }
+        }
     }
     s->stats_vq_offset = offset;
 
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 35f62ac..5c8730e 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -36,7 +36,8 @@ typedef struct VirtIOBalloon {
     VirtQueue *ivq, *dvq, *svq;
     uint32_t num_pages;
     uint32_t actual;
-    uint64_t stats[VIRTIO_BALLOON_S_NR];
+    VirtIOBalloonStatModern stats[VIRTIO_BALLOON_S_NR + 32];
+    uint16_t stats_cnt;
     VirtQueueElement *stats_vq_elem;
     size_t stats_vq_offset;
     QEMUTimer *stats_timer;
-- 
2.1.4