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

["Denis V. Lunev" <den-lists@virtuozzo.com>]

On 03/16/2016 01:37 PM, Michael S. Tsirkin wrote:
> 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.
>
how could we start the discussion?