Re: [Qemu-devel] [PULL 08/16] virtio-balloon: export all balloon statistics

["Denis V. Lunev" <den@openvz.org>]

On 03/04/2016 10:49 AM, Michael S. Tsirkin 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   configure                          | 12 ++++++++++++
>   include/hw/virtio/virtio-balloon.h |  3 ++-
>   hw/virtio/virtio-balloon.c         | 32 ++++++++++++++++++++++++++------
>   3 files changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/configure b/configure
> index 0c0472a..767d96e 100755
> --- a/configure
> +++ b/configure
> @@ -315,6 +315,7 @@ vhdx=""
>   numa=""
>   tcmalloc="no"
>   jemalloc="no"
> +unknown_balloon_stats="no"
>   
>   # parse CC options first
>   for opt do
> @@ -1142,6 +1143,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"
> @@ -1364,6 +1369,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
> @@ -4790,6 +4797,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"
> @@ -5342,6 +5350,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/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;
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index e97d403..64367ac 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);
> @@ -282,10 +291,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;
>   
Michael,

what has happened with this patch?

I have seen pull request and this patch was coupled with
'available' modification. 'available' code was resent 11.03
and this one not.

Den