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 <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Michael S. Tsirkin <address@hidden>

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