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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Mar 16, 2016 at 01:33:31PM +0300, Denis V. Lunev wrote:
> 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

Yes - I included it by mistake, and dropped in v2.
Let's discuss after 2.6.

-- 
MST