From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-24.0 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7FD55C07E96 for ; Thu, 8 Jul 2021 21:45:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 620E161607 for ; Thu, 8 Jul 2021 21:45:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231402AbhGHVsS (ORCPT ); Thu, 8 Jul 2021 17:48:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33856 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231359AbhGHVsR (ORCPT ); Thu, 8 Jul 2021 17:48:17 -0400 Received: from mail-pf1-x432.google.com (mail-pf1-x432.google.com [IPv6:2607:f8b0:4864:20::432]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CC531C06175F for ; Thu, 8 Jul 2021 14:45:34 -0700 (PDT) Received: by mail-pf1-x432.google.com with SMTP id 21so6831256pfp.3 for ; Thu, 08 Jul 2021 14:45:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=W0ZQelC6T0ei1R0gY+iGoyAxrY/5z//ht72/j3Q3BDc=; b=r59rwp/3da+1B4EI9p+QoqOMrS7hLykjlBz/dFdOVbZhWK30g9hNFII0HpKfzD9lV7 eNFk9Iuq1k2bcC0jhiOzTsTXIRTo5T+Fq+ViF1kFicxVARj5sAB/csfjP4gjPe+ZQdNH kV1KvonAjoUdiqUR8OzUVAqNLLpDwL4nm9bzzI6pGMB5hSngHCdKLYMqRcVkge3YQqdT tzIpl95XsX4E5PJ1xN/zWEp+l54EyU01hiwB3bF/c/l63lSAHC6hjc2sfJ4RL5TGmmko aE9xBuazP92Cr2WAmCM75HpXWAPVi506V8APy4xDuVbeKGSxJcX9oOKtKPdDsBVDMzRx g7Gg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=W0ZQelC6T0ei1R0gY+iGoyAxrY/5z//ht72/j3Q3BDc=; b=GQ16cNLCo+wfYIyg4RiCR30/faCCSyY4vzF/rcYJs2NxWiwer3wq7gHrkR+eGuA2zQ XfG/S0XTfQ+zLsD3h3rhQjtuf7XDz7FL5tuKZxoyetAjNnKgF6T9X1hsGE7yAySI0CsT n0Igt7OyGXeS2u368BLB9kyW8ZZJCQRNwsNNqusB+tgJHOqfpRI9I6hPNYkvbn3Nvz/X qe//6sdcoutTfY188ZjKY4gfEtyo0Olw/GgHEtgKGsTaFsMifAhtOoNfspC07kUPOSnD 1FREQHrls4qmAqbn0fofWdBy0vnSIQsvIpsRJBepTgmI7rFGNkelDJdUdFkgT0ihZyBJ ozyg== X-Gm-Message-State: AOAM533SqWS76+kMnMCvn74eo51qv6miHR0F3NIz2o0kldkFDhfIN22V FcW40PUNYcEop+SFjHctVeaLbw== X-Google-Smtp-Source: ABdhPJxKDYV90AeVVXSjRpHC1ir4YB25j50/3SjrNhTypYVc8sv0SFi+loDMuMerrNVFb3hrezSRXg== X-Received: by 2002:a05:6a00:1481:b029:304:87ea:8dcc with SMTP id v1-20020a056a001481b029030487ea8dccmr33055736pfu.46.1625780734023; Thu, 08 Jul 2021 14:45:34 -0700 (PDT) Received: from google.com (254.80.82.34.bc.googleusercontent.com. [34.82.80.254]) by smtp.gmail.com with ESMTPSA id n23sm4204026pgv.76.2021.07.08.14.45.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Jul 2021 14:45:33 -0700 (PDT) Date: Thu, 8 Jul 2021 21:45:29 +0000 From: David Matlack To: Jing Zhang Cc: KVM , KVMPPC , Paolo Bonzini , Sean Christopherson , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes Subject: Re: [PATCH v1 1/4] KVM: stats: Support linear and logarithmic histogram statistics Message-ID: References: <20210706180350.2838127-1-jingzhangos@google.com> <20210706180350.2838127-2-jingzhangos@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Thu, Jul 08, 2021 at 04:40:16PM -0500, Jing Zhang wrote: > On Thu, Jul 8, 2021 at 4:01 PM David Matlack wrote: > > > > On Tue, Jul 06, 2021 at 06:03:47PM +0000, Jing Zhang wrote: > > > Add new types of KVM stats, linear and logarithmic histogram. > > > Histogram are very useful for observing the value distribution > > > of time or size related stats. > > > > > > Signed-off-by: Jing Zhang > > > --- > > > arch/arm64/kvm/guest.c | 4 --- > > > arch/mips/kvm/mips.c | 4 --- > > > arch/powerpc/kvm/book3s.c | 4 --- > > > arch/powerpc/kvm/booke.c | 4 --- > > > arch/s390/kvm/kvm-s390.c | 4 --- > > > arch/x86/kvm/x86.c | 4 --- > > > include/linux/kvm_host.h | 53 ++++++++++++++++++++++++++++----------- > > > include/linux/kvm_types.h | 16 ++++++++++++ > > > include/uapi/linux/kvm.h | 11 +++++--- > > > virt/kvm/binary_stats.c | 36 ++++++++++++++++++++++++++ > > > 10 files changed, 98 insertions(+), 42 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > > index 1512a8007a78..cb44d8756fa7 100644 > > > --- a/arch/arm64/kvm/guest.c > > > +++ b/arch/arm64/kvm/guest.c > > > @@ -31,8 +31,6 @@ > > > const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > KVM_GENERIC_VM_STATS() > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) == > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -52,8 +50,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, mmio_exit_kernel), > > > STATS_DESC_COUNTER(VCPU, exits) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) == > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > > > index af9dd029a4e1..75c6f264c626 100644 > > > --- a/arch/mips/kvm/mips.c > > > +++ b/arch/mips/kvm/mips.c > > > @@ -41,8 +41,6 @@ > > > const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > KVM_GENERIC_VM_STATS() > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) == > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -85,8 +83,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, vz_cpucfg_exits), > > > #endif > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) == > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c > > > index 79833f78d1da..5cc6e90095b0 100644 > > > --- a/arch/powerpc/kvm/book3s.c > > > +++ b/arch/powerpc/kvm/book3s.c > > > @@ -43,8 +43,6 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > STATS_DESC_ICOUNTER(VM, num_2M_pages), > > > STATS_DESC_ICOUNTER(VM, num_1G_pages) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) == > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -88,8 +86,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, pthru_host), > > > STATS_DESC_COUNTER(VCPU, pthru_bad_aff) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) == > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > > > index 551b30d84aee..5ed6c235e059 100644 > > > --- a/arch/powerpc/kvm/booke.c > > > +++ b/arch/powerpc/kvm/booke.c > > > @@ -41,8 +41,6 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > STATS_DESC_ICOUNTER(VM, num_2M_pages), > > > STATS_DESC_ICOUNTER(VM, num_1G_pages) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) == > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -79,8 +77,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, pthru_host), > > > STATS_DESC_COUNTER(VCPU, pthru_bad_aff) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) == > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > > index 1695f0ced5ba..7610d33d319b 100644 > > > --- a/arch/s390/kvm/kvm-s390.c > > > +++ b/arch/s390/kvm/kvm-s390.c > > > @@ -66,8 +66,6 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > STATS_DESC_COUNTER(VM, inject_service_signal), > > > STATS_DESC_COUNTER(VM, inject_virtio) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) == > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -174,8 +172,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, diagnose_other), > > > STATS_DESC_COUNTER(VCPU, pfault_sync) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) == > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index 8166ad113fb2..b94a80ad5b8d 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -239,8 +239,6 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > STATS_DESC_ICOUNTER(VM, nx_lpage_splits), > > > STATS_DESC_PCOUNTER(VM, max_mmu_page_hash_collisions) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) == > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -280,8 +278,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, directed_yield_successful), > > > STATS_DESC_ICOUNTER(VCPU, guest_mode) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) == > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > > index ae7735b490b4..356af173114d 100644 > > > --- a/include/linux/kvm_host.h > > > +++ b/include/linux/kvm_host.h > > > @@ -1273,56 +1273,66 @@ struct _kvm_stats_desc { > > > char name[KVM_STATS_NAME_SIZE]; > > > }; > > > > > > -#define STATS_DESC_COMMON(type, unit, base, exp) \ > > > +#define STATS_DESC_COMMON(type, unit, base, exp, sz, param) \ > > > .flags = type | unit | base | \ > > > BUILD_BUG_ON_ZERO(type & ~KVM_STATS_TYPE_MASK) | \ > > > BUILD_BUG_ON_ZERO(unit & ~KVM_STATS_UNIT_MASK) | \ > > > BUILD_BUG_ON_ZERO(base & ~KVM_STATS_BASE_MASK), \ > > > .exponent = exp, \ > > > - .size = 1 > > > + .size = sz, \ > > > + .hist_param = param > > > > > > -#define VM_GENERIC_STATS_DESC(stat, type, unit, base, exp) \ > > > +#define VM_GENERIC_STATS_DESC(stat, type, unit, base, exp, sz, param) \ > > > { \ > > > { \ > > > - STATS_DESC_COMMON(type, unit, base, exp), \ > > > + STATS_DESC_COMMON(type, unit, base, exp, sz, param), \ > > > .offset = offsetof(struct kvm_vm_stat, generic.stat) \ > > > }, \ > > > .name = #stat, \ > > > } > > > -#define VCPU_GENERIC_STATS_DESC(stat, type, unit, base, exp) \ > > > +#define VCPU_GENERIC_STATS_DESC(stat, type, unit, base, exp, sz, param) \ > > > { \ > > > { \ > > > - STATS_DESC_COMMON(type, unit, base, exp), \ > > > + STATS_DESC_COMMON(type, unit, base, exp, sz, param), \ > > > .offset = offsetof(struct kvm_vcpu_stat, generic.stat) \ > > > }, \ > > > .name = #stat, \ > > > } > > > -#define VM_STATS_DESC(stat, type, unit, base, exp) \ > > > +#define VM_STATS_DESC(stat, type, unit, base, exp, sz, param) \ > > > { \ > > > { \ > > > - STATS_DESC_COMMON(type, unit, base, exp), \ > > > + STATS_DESC_COMMON(type, unit, base, exp, sz, param), \ > > > .offset = offsetof(struct kvm_vm_stat, stat) \ > > > }, \ > > > .name = #stat, \ > > > } > > > -#define VCPU_STATS_DESC(stat, type, unit, base, exp) \ > > > +#define VCPU_STATS_DESC(stat, type, unit, base, exp, sz, param) \ > > > { \ > > > { \ > > > - STATS_DESC_COMMON(type, unit, base, exp), \ > > > + STATS_DESC_COMMON(type, unit, base, exp, sz, param), \ > > > .offset = offsetof(struct kvm_vcpu_stat, stat) \ > > > }, \ > > > .name = #stat, \ > > > } > > > /* SCOPE: VM, VM_GENERIC, VCPU, VCPU_GENERIC */ > > > -#define STATS_DESC(SCOPE, stat, type, unit, base, exp) \ > > > - SCOPE##_STATS_DESC(stat, type, unit, base, exp) > > > +#define STATS_DESC(SCOPE, stat, type, unit, base, exp, sz, param) \ > > > + SCOPE##_STATS_DESC(stat, type, unit, base, exp, sz, param) > > > > > > #define STATS_DESC_CUMULATIVE(SCOPE, name, unit, base, exponent) \ > > > - STATS_DESC(SCOPE, name, KVM_STATS_TYPE_CUMULATIVE, unit, base, exponent) > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_CUMULATIVE, \ > > > + unit, base, exponent, 1, 0) > > > #define STATS_DESC_INSTANT(SCOPE, name, unit, base, exponent) \ > > > - STATS_DESC(SCOPE, name, KVM_STATS_TYPE_INSTANT, unit, base, exponent) > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_INSTANT, \ > > > + unit, base, exponent, 1, 0) > > > #define STATS_DESC_PEAK(SCOPE, name, unit, base, exponent) \ > > > - STATS_DESC(SCOPE, name, KVM_STATS_TYPE_PEAK, unit, base, exponent) > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_PEAK, \ > > > + unit, base, exponent, 1, 0) > > > +#define STATS_DESC_LINEAR_HIST(SCOPE, name, unit, base, exponent, sz, param) \ > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_LINEAR_HIST, \ > > > + unit, base, exponent, sz, param) > > > +#define STATS_DESC_LOG_HIST(SCOPE, name, unit, base, exponent, sz, param) \ > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_LOG_HIST, \ > > > + unit, base, exponent, sz, param) > > > > > > /* Cumulative counter, read/write */ > > > #define STATS_DESC_COUNTER(SCOPE, name) \ > > > @@ -1341,6 +1351,14 @@ struct _kvm_stats_desc { > > > #define STATS_DESC_TIME_NSEC(SCOPE, name) \ > > > STATS_DESC_CUMULATIVE(SCOPE, name, KVM_STATS_UNIT_SECONDS, \ > > > KVM_STATS_BASE_POW10, -9) > > > +/* Linear histogram for time in nanosecond */ > > > +#define STATS_DESC_LINHIST_TIME_NSEC(SCOPE, name, sz, bucket_size) \ > > > + STATS_DESC_LINEAR_HIST(SCOPE, name, KVM_STATS_UNIT_SECONDS, \ > > > + KVM_STATS_BASE_POW10, -9, sz, bucket_size) > > > +/* Logarithmic histogram for time in nanosecond */ > > > +#define STATS_DESC_LOGHIST_TIME_NSEC(SCOPE, name, sz) \ > > > + STATS_DESC_LOG_HIST(SCOPE, name, KVM_STATS_UNIT_SECONDS, \ > > > + KVM_STATS_BASE_POW10, -9, sz, LOGHIST_BASE_2) > > > > > > #define KVM_GENERIC_VM_STATS() \ > > > STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush) > > > @@ -1354,10 +1372,15 @@ struct _kvm_stats_desc { > > > STATS_DESC_TIME_NSEC(VCPU_GENERIC, halt_poll_fail_ns) > > > > > > extern struct dentry *kvm_debugfs_dir; > > > + > > > ssize_t kvm_stats_read(char *id, const struct kvm_stats_header *header, > > > const struct _kvm_stats_desc *desc, > > > void *stats, size_t size_stats, > > > char __user *user_buffer, size_t size, loff_t *offset); > > > +void kvm_stats_linear_hist_update(u64 *data, size_t size, > > > + u64 value, size_t bucket_size); > > > +void kvm_stats_log_hist_update(u64 *data, size_t size, u64 value); > > > + > > > extern const struct kvm_stats_header kvm_vm_stats_header; > > > extern const struct _kvm_stats_desc kvm_vm_stats_desc[]; > > > extern const struct kvm_stats_header kvm_vcpu_stats_header; > > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h > > > index ed6a985c5680..cc88cd676775 100644 > > > --- a/include/linux/kvm_types.h > > > +++ b/include/linux/kvm_types.h > > > @@ -76,6 +76,22 @@ struct kvm_mmu_memory_cache { > > > }; > > > #endif > > > > > > +/* Constants used for histogram stats */ > > > +#define LINHIST_SIZE_SMALL 10 > > > +#define LINHIST_SIZE_MEDIUM 20 > > > +#define LINHIST_SIZE_LARGE 50 > > > +#define LINHIST_SIZE_XLARGE 100 > > > > nit: s/SIZE/BUCKET_COUNT/ > > > Sure, it makes more sense. > > > +#define LINHIST_BUCKET_SIZE_SMALL 10 > > > +#define LINHIST_BUCKET_SIZE_MEDIUM 100 > > > +#define LINHIST_BUCKET_SIZE_LARGE 1000 > > > +#define LINHIST_BUCKET_SIZE_XLARGE 10000 > > > + > > > +#define LOGHIST_SIZE_SMALL 8 > > > +#define LOGHIST_SIZE_MEDIUM 16 > > > +#define LOGHIST_SIZE_LARGE 32 > > > +#define LOGHIST_SIZE_XLARGE 64 > > > > Ditto here. > > > Will do. > > > +#define LOGHIST_BASE_2 2 > > > + > > > struct kvm_vm_stat_generic { > > > u64 remote_tlb_flush; > > > }; > > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > > > index 68c9e6d8bbda..ff34a471d9ef 100644 > > > --- a/include/uapi/linux/kvm.h > > > +++ b/include/uapi/linux/kvm.h > > > @@ -1963,7 +1963,9 @@ struct kvm_stats_header { > > > #define KVM_STATS_TYPE_CUMULATIVE (0x0 << KVM_STATS_TYPE_SHIFT) > > > #define KVM_STATS_TYPE_INSTANT (0x1 << KVM_STATS_TYPE_SHIFT) > > > #define KVM_STATS_TYPE_PEAK (0x2 << KVM_STATS_TYPE_SHIFT) > > > -#define KVM_STATS_TYPE_MAX KVM_STATS_TYPE_PEAK > > > +#define KVM_STATS_TYPE_LINEAR_HIST (0x3 << KVM_STATS_TYPE_SHIFT) > > > +#define KVM_STATS_TYPE_LOG_HIST (0x4 << KVM_STATS_TYPE_SHIFT) > > > +#define KVM_STATS_TYPE_MAX KVM_STATS_TYPE_LOG_HIST > > > > > > #define KVM_STATS_UNIT_SHIFT 4 > > > #define KVM_STATS_UNIT_MASK (0xF << KVM_STATS_UNIT_SHIFT) > > > @@ -1987,7 +1989,10 @@ struct kvm_stats_header { > > > * Every data item is of type __u64. > > > * @offset: The offset of the stats to the start of stat structure in > > > * struture kvm or kvm_vcpu. > > > - * @unused: Unused field for future usage. Always 0 for now. > > > + * @hist_param: A parameter value used for histogram stats. For linear > > > + * histogram stats, it indicates the size of the bucket; > > > + * For logarithmic histogram stats, it indicates the base > > > + * of the logarithm. Only base of 2 is supported. > > > * @name: The name string for the stats. Its size is indicated by the > > > * &kvm_stats_header->name_size. > > > */ > > > @@ -1996,7 +2001,7 @@ struct kvm_stats_desc { > > > __s16 exponent; > > > __u16 size; > > > __u32 offset; > > > - __u32 unused; > > > + __u32 hist_param; > > > > `hist_param` is vague. What about making this an anonymous union to make > > the dual meaning explicit? > > > > union { > > /* Only used for KVM_STATS_TYPE_LOG_HIST. */ > > __u32 base; > > /* Only used for KVM_STATS_TYPE_LINEAR_HIST. */ > > __u32 bucket_size; > > }; > > > > It may make the STATS_DESC code a bit more complicated but the rest of > > the code that uses it will be much more clear. > > > Since we only support base-2 log hist, maybe it is not necessary to > have the base field. > The reason to only support base-2 log hist is that its range is > already large enough for > any stats. > Will just change hist_param to bucket_size. Cool sounds good. We can always add a custom base in the future without breaking userspace by adding the anonymous union and guarding it behind the flag field. > > > char name[]; > > > }; > > > > > > diff --git a/virt/kvm/binary_stats.c b/virt/kvm/binary_stats.c > > > index e609d428811a..6eead6979a7f 100644 > > > --- a/virt/kvm/binary_stats.c > > > +++ b/virt/kvm/binary_stats.c > > > @@ -144,3 +144,39 @@ ssize_t kvm_stats_read(char *id, const struct kvm_stats_header *header, > > > *offset = pos; > > > return len; > > > } > > > + > > > +/** > > > + * kvm_stats_linear_hist_update() - Update bucket value for linear histogram > > > + * statistics data. > > > + * > > > + * @data: start address of the stats data > > > + * @size: the number of bucket of the stats data > > > + * @value: the new value used to update the linear histogram's bucket > > > + * @bucket_size: the size (width) of a bucket > > > + */ > > > +void kvm_stats_linear_hist_update(u64 *data, size_t size, > > > + u64 value, size_t bucket_size) > > > +{ > > > + size_t index = value / bucket_size; > > > + > > > + if (index >= size) > > > + index = size - 1; > > > > nit: It would be simpler to use max(). > > > > size_t index = max(value / bucket_size, size - 1); > > > Will do. > > > + ++data[index]; > > > +} > > > + > > > +/** > > > + * kvm_stats_log_hist_update() - Update bucket value for logarithmic histogram > > > + * statistics data. > > > + * > > > + * @data: start address of the stats data > > > + * @size: the number of bucket of the stats data > > > + * @value: the new value used to update the logarithmic histogram's bucket > > > + */ > > > +void kvm_stats_log_hist_update(u64 *data, size_t size, u64 value) > > > +{ > > > + size_t index = fls64(value); > > > + > > > + if (index >= size) > > > + index = size - 1; > > > > Ditto here about using max(). > > > Will do. > > > + ++data[index]; > > > +} > > > -- > > > 2.32.0.93.g670b81a890-goog > > > > Thanks, > Jing From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Matlack Date: Thu, 08 Jul 2021 21:45:29 +0000 Subject: Re: [PATCH v1 1/4] KVM: stats: Support linear and logarithmic histogram statistics Message-Id: List-Id: References: <20210706180350.2838127-1-jingzhangos@google.com> <20210706180350.2838127-2-jingzhangos@google.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Jing Zhang Cc: KVM , KVMPPC , Paolo Bonzini , Sean Christopherson , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes On Thu, Jul 08, 2021 at 04:40:16PM -0500, Jing Zhang wrote: > On Thu, Jul 8, 2021 at 4:01 PM David Matlack wrote: > > > > On Tue, Jul 06, 2021 at 06:03:47PM +0000, Jing Zhang wrote: > > > Add new types of KVM stats, linear and logarithmic histogram. > > > Histogram are very useful for observing the value distribution > > > of time or size related stats. > > > > > > Signed-off-by: Jing Zhang > > > --- > > > arch/arm64/kvm/guest.c | 4 --- > > > arch/mips/kvm/mips.c | 4 --- > > > arch/powerpc/kvm/book3s.c | 4 --- > > > arch/powerpc/kvm/booke.c | 4 --- > > > arch/s390/kvm/kvm-s390.c | 4 --- > > > arch/x86/kvm/x86.c | 4 --- > > > include/linux/kvm_host.h | 53 ++++++++++++++++++++++++++++----------- > > > include/linux/kvm_types.h | 16 ++++++++++++ > > > include/uapi/linux/kvm.h | 11 +++++--- > > > virt/kvm/binary_stats.c | 36 ++++++++++++++++++++++++++ > > > 10 files changed, 98 insertions(+), 42 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > > index 1512a8007a78..cb44d8756fa7 100644 > > > --- a/arch/arm64/kvm/guest.c > > > +++ b/arch/arm64/kvm/guest.c > > > @@ -31,8 +31,6 @@ > > > const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > KVM_GENERIC_VM_STATS() > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) = > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -52,8 +50,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, mmio_exit_kernel), > > > STATS_DESC_COUNTER(VCPU, exits) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) = > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > > > index af9dd029a4e1..75c6f264c626 100644 > > > --- a/arch/mips/kvm/mips.c > > > +++ b/arch/mips/kvm/mips.c > > > @@ -41,8 +41,6 @@ > > > const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > KVM_GENERIC_VM_STATS() > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) = > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -85,8 +83,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, vz_cpucfg_exits), > > > #endif > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) = > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c > > > index 79833f78d1da..5cc6e90095b0 100644 > > > --- a/arch/powerpc/kvm/book3s.c > > > +++ b/arch/powerpc/kvm/book3s.c > > > @@ -43,8 +43,6 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > STATS_DESC_ICOUNTER(VM, num_2M_pages), > > > STATS_DESC_ICOUNTER(VM, num_1G_pages) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) = > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -88,8 +86,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, pthru_host), > > > STATS_DESC_COUNTER(VCPU, pthru_bad_aff) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) = > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > > > index 551b30d84aee..5ed6c235e059 100644 > > > --- a/arch/powerpc/kvm/booke.c > > > +++ b/arch/powerpc/kvm/booke.c > > > @@ -41,8 +41,6 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > STATS_DESC_ICOUNTER(VM, num_2M_pages), > > > STATS_DESC_ICOUNTER(VM, num_1G_pages) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) = > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -79,8 +77,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, pthru_host), > > > STATS_DESC_COUNTER(VCPU, pthru_bad_aff) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) = > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > > index 1695f0ced5ba..7610d33d319b 100644 > > > --- a/arch/s390/kvm/kvm-s390.c > > > +++ b/arch/s390/kvm/kvm-s390.c > > > @@ -66,8 +66,6 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > STATS_DESC_COUNTER(VM, inject_service_signal), > > > STATS_DESC_COUNTER(VM, inject_virtio) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) = > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -174,8 +172,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, diagnose_other), > > > STATS_DESC_COUNTER(VCPU, pfault_sync) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) = > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index 8166ad113fb2..b94a80ad5b8d 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -239,8 +239,6 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = { > > > STATS_DESC_ICOUNTER(VM, nx_lpage_splits), > > > STATS_DESC_PCOUNTER(VM, max_mmu_page_hash_collisions) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vm_stats_desc) = > > > - sizeof(struct kvm_vm_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vm_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > @@ -280,8 +278,6 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = { > > > STATS_DESC_COUNTER(VCPU, directed_yield_successful), > > > STATS_DESC_ICOUNTER(VCPU, guest_mode) > > > }; > > > -static_assert(ARRAY_SIZE(kvm_vcpu_stats_desc) = > > > - sizeof(struct kvm_vcpu_stat) / sizeof(u64)); > > > > > > const struct kvm_stats_header kvm_vcpu_stats_header = { > > > .name_size = KVM_STATS_NAME_SIZE, > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > > index ae7735b490b4..356af173114d 100644 > > > --- a/include/linux/kvm_host.h > > > +++ b/include/linux/kvm_host.h > > > @@ -1273,56 +1273,66 @@ struct _kvm_stats_desc { > > > char name[KVM_STATS_NAME_SIZE]; > > > }; > > > > > > -#define STATS_DESC_COMMON(type, unit, base, exp) \ > > > +#define STATS_DESC_COMMON(type, unit, base, exp, sz, param) \ > > > .flags = type | unit | base | \ > > > BUILD_BUG_ON_ZERO(type & ~KVM_STATS_TYPE_MASK) | \ > > > BUILD_BUG_ON_ZERO(unit & ~KVM_STATS_UNIT_MASK) | \ > > > BUILD_BUG_ON_ZERO(base & ~KVM_STATS_BASE_MASK), \ > > > .exponent = exp, \ > > > - .size = 1 > > > + .size = sz, \ > > > + .hist_param = param > > > > > > -#define VM_GENERIC_STATS_DESC(stat, type, unit, base, exp) \ > > > +#define VM_GENERIC_STATS_DESC(stat, type, unit, base, exp, sz, param) \ > > > { \ > > > { \ > > > - STATS_DESC_COMMON(type, unit, base, exp), \ > > > + STATS_DESC_COMMON(type, unit, base, exp, sz, param), \ > > > .offset = offsetof(struct kvm_vm_stat, generic.stat) \ > > > }, \ > > > .name = #stat, \ > > > } > > > -#define VCPU_GENERIC_STATS_DESC(stat, type, unit, base, exp) \ > > > +#define VCPU_GENERIC_STATS_DESC(stat, type, unit, base, exp, sz, param) \ > > > { \ > > > { \ > > > - STATS_DESC_COMMON(type, unit, base, exp), \ > > > + STATS_DESC_COMMON(type, unit, base, exp, sz, param), \ > > > .offset = offsetof(struct kvm_vcpu_stat, generic.stat) \ > > > }, \ > > > .name = #stat, \ > > > } > > > -#define VM_STATS_DESC(stat, type, unit, base, exp) \ > > > +#define VM_STATS_DESC(stat, type, unit, base, exp, sz, param) \ > > > { \ > > > { \ > > > - STATS_DESC_COMMON(type, unit, base, exp), \ > > > + STATS_DESC_COMMON(type, unit, base, exp, sz, param), \ > > > .offset = offsetof(struct kvm_vm_stat, stat) \ > > > }, \ > > > .name = #stat, \ > > > } > > > -#define VCPU_STATS_DESC(stat, type, unit, base, exp) \ > > > +#define VCPU_STATS_DESC(stat, type, unit, base, exp, sz, param) \ > > > { \ > > > { \ > > > - STATS_DESC_COMMON(type, unit, base, exp), \ > > > + STATS_DESC_COMMON(type, unit, base, exp, sz, param), \ > > > .offset = offsetof(struct kvm_vcpu_stat, stat) \ > > > }, \ > > > .name = #stat, \ > > > } > > > /* SCOPE: VM, VM_GENERIC, VCPU, VCPU_GENERIC */ > > > -#define STATS_DESC(SCOPE, stat, type, unit, base, exp) \ > > > - SCOPE##_STATS_DESC(stat, type, unit, base, exp) > > > +#define STATS_DESC(SCOPE, stat, type, unit, base, exp, sz, param) \ > > > + SCOPE##_STATS_DESC(stat, type, unit, base, exp, sz, param) > > > > > > #define STATS_DESC_CUMULATIVE(SCOPE, name, unit, base, exponent) \ > > > - STATS_DESC(SCOPE, name, KVM_STATS_TYPE_CUMULATIVE, unit, base, exponent) > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_CUMULATIVE, \ > > > + unit, base, exponent, 1, 0) > > > #define STATS_DESC_INSTANT(SCOPE, name, unit, base, exponent) \ > > > - STATS_DESC(SCOPE, name, KVM_STATS_TYPE_INSTANT, unit, base, exponent) > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_INSTANT, \ > > > + unit, base, exponent, 1, 0) > > > #define STATS_DESC_PEAK(SCOPE, name, unit, base, exponent) \ > > > - STATS_DESC(SCOPE, name, KVM_STATS_TYPE_PEAK, unit, base, exponent) > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_PEAK, \ > > > + unit, base, exponent, 1, 0) > > > +#define STATS_DESC_LINEAR_HIST(SCOPE, name, unit, base, exponent, sz, param) \ > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_LINEAR_HIST, \ > > > + unit, base, exponent, sz, param) > > > +#define STATS_DESC_LOG_HIST(SCOPE, name, unit, base, exponent, sz, param) \ > > > + STATS_DESC(SCOPE, name, KVM_STATS_TYPE_LOG_HIST, \ > > > + unit, base, exponent, sz, param) > > > > > > /* Cumulative counter, read/write */ > > > #define STATS_DESC_COUNTER(SCOPE, name) \ > > > @@ -1341,6 +1351,14 @@ struct _kvm_stats_desc { > > > #define STATS_DESC_TIME_NSEC(SCOPE, name) \ > > > STATS_DESC_CUMULATIVE(SCOPE, name, KVM_STATS_UNIT_SECONDS, \ > > > KVM_STATS_BASE_POW10, -9) > > > +/* Linear histogram for time in nanosecond */ > > > +#define STATS_DESC_LINHIST_TIME_NSEC(SCOPE, name, sz, bucket_size) \ > > > + STATS_DESC_LINEAR_HIST(SCOPE, name, KVM_STATS_UNIT_SECONDS, \ > > > + KVM_STATS_BASE_POW10, -9, sz, bucket_size) > > > +/* Logarithmic histogram for time in nanosecond */ > > > +#define STATS_DESC_LOGHIST_TIME_NSEC(SCOPE, name, sz) \ > > > + STATS_DESC_LOG_HIST(SCOPE, name, KVM_STATS_UNIT_SECONDS, \ > > > + KVM_STATS_BASE_POW10, -9, sz, LOGHIST_BASE_2) > > > > > > #define KVM_GENERIC_VM_STATS() \ > > > STATS_DESC_COUNTER(VM_GENERIC, remote_tlb_flush) > > > @@ -1354,10 +1372,15 @@ struct _kvm_stats_desc { > > > STATS_DESC_TIME_NSEC(VCPU_GENERIC, halt_poll_fail_ns) > > > > > > extern struct dentry *kvm_debugfs_dir; > > > + > > > ssize_t kvm_stats_read(char *id, const struct kvm_stats_header *header, > > > const struct _kvm_stats_desc *desc, > > > void *stats, size_t size_stats, > > > char __user *user_buffer, size_t size, loff_t *offset); > > > +void kvm_stats_linear_hist_update(u64 *data, size_t size, > > > + u64 value, size_t bucket_size); > > > +void kvm_stats_log_hist_update(u64 *data, size_t size, u64 value); > > > + > > > extern const struct kvm_stats_header kvm_vm_stats_header; > > > extern const struct _kvm_stats_desc kvm_vm_stats_desc[]; > > > extern const struct kvm_stats_header kvm_vcpu_stats_header; > > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h > > > index ed6a985c5680..cc88cd676775 100644 > > > --- a/include/linux/kvm_types.h > > > +++ b/include/linux/kvm_types.h > > > @@ -76,6 +76,22 @@ struct kvm_mmu_memory_cache { > > > }; > > > #endif > > > > > > +/* Constants used for histogram stats */ > > > +#define LINHIST_SIZE_SMALL 10 > > > +#define LINHIST_SIZE_MEDIUM 20 > > > +#define LINHIST_SIZE_LARGE 50 > > > +#define LINHIST_SIZE_XLARGE 100 > > > > nit: s/SIZE/BUCKET_COUNT/ > > > Sure, it makes more sense. > > > +#define LINHIST_BUCKET_SIZE_SMALL 10 > > > +#define LINHIST_BUCKET_SIZE_MEDIUM 100 > > > +#define LINHIST_BUCKET_SIZE_LARGE 1000 > > > +#define LINHIST_BUCKET_SIZE_XLARGE 10000 > > > + > > > +#define LOGHIST_SIZE_SMALL 8 > > > +#define LOGHIST_SIZE_MEDIUM 16 > > > +#define LOGHIST_SIZE_LARGE 32 > > > +#define LOGHIST_SIZE_XLARGE 64 > > > > Ditto here. > > > Will do. > > > +#define LOGHIST_BASE_2 2 > > > + > > > struct kvm_vm_stat_generic { > > > u64 remote_tlb_flush; > > > }; > > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > > > index 68c9e6d8bbda..ff34a471d9ef 100644 > > > --- a/include/uapi/linux/kvm.h > > > +++ b/include/uapi/linux/kvm.h > > > @@ -1963,7 +1963,9 @@ struct kvm_stats_header { > > > #define KVM_STATS_TYPE_CUMULATIVE (0x0 << KVM_STATS_TYPE_SHIFT) > > > #define KVM_STATS_TYPE_INSTANT (0x1 << KVM_STATS_TYPE_SHIFT) > > > #define KVM_STATS_TYPE_PEAK (0x2 << KVM_STATS_TYPE_SHIFT) > > > -#define KVM_STATS_TYPE_MAX KVM_STATS_TYPE_PEAK > > > +#define KVM_STATS_TYPE_LINEAR_HIST (0x3 << KVM_STATS_TYPE_SHIFT) > > > +#define KVM_STATS_TYPE_LOG_HIST (0x4 << KVM_STATS_TYPE_SHIFT) > > > +#define KVM_STATS_TYPE_MAX KVM_STATS_TYPE_LOG_HIST > > > > > > #define KVM_STATS_UNIT_SHIFT 4 > > > #define KVM_STATS_UNIT_MASK (0xF << KVM_STATS_UNIT_SHIFT) > > > @@ -1987,7 +1989,10 @@ struct kvm_stats_header { > > > * Every data item is of type __u64. > > > * @offset: The offset of the stats to the start of stat structure in > > > * struture kvm or kvm_vcpu. > > > - * @unused: Unused field for future usage. Always 0 for now. > > > + * @hist_param: A parameter value used for histogram stats. For linear > > > + * histogram stats, it indicates the size of the bucket; > > > + * For logarithmic histogram stats, it indicates the base > > > + * of the logarithm. Only base of 2 is supported. > > > * @name: The name string for the stats. Its size is indicated by the > > > * &kvm_stats_header->name_size. > > > */ > > > @@ -1996,7 +2001,7 @@ struct kvm_stats_desc { > > > __s16 exponent; > > > __u16 size; > > > __u32 offset; > > > - __u32 unused; > > > + __u32 hist_param; > > > > `hist_param` is vague. What about making this an anonymous union to make > > the dual meaning explicit? > > > > union { > > /* Only used for KVM_STATS_TYPE_LOG_HIST. */ > > __u32 base; > > /* Only used for KVM_STATS_TYPE_LINEAR_HIST. */ > > __u32 bucket_size; > > }; > > > > It may make the STATS_DESC code a bit more complicated but the rest of > > the code that uses it will be much more clear. > > > Since we only support base-2 log hist, maybe it is not necessary to > have the base field. > The reason to only support base-2 log hist is that its range is > already large enough for > any stats. > Will just change hist_param to bucket_size. Cool sounds good. We can always add a custom base in the future without breaking userspace by adding the anonymous union and guarding it behind the flag field. > > > char name[]; > > > }; > > > > > > diff --git a/virt/kvm/binary_stats.c b/virt/kvm/binary_stats.c > > > index e609d428811a..6eead6979a7f 100644 > > > --- a/virt/kvm/binary_stats.c > > > +++ b/virt/kvm/binary_stats.c > > > @@ -144,3 +144,39 @@ ssize_t kvm_stats_read(char *id, const struct kvm_stats_header *header, > > > *offset = pos; > > > return len; > > > } > > > + > > > +/** > > > + * kvm_stats_linear_hist_update() - Update bucket value for linear histogram > > > + * statistics data. > > > + * > > > + * @data: start address of the stats data > > > + * @size: the number of bucket of the stats data > > > + * @value: the new value used to update the linear histogram's bucket > > > + * @bucket_size: the size (width) of a bucket > > > + */ > > > +void kvm_stats_linear_hist_update(u64 *data, size_t size, > > > + u64 value, size_t bucket_size) > > > +{ > > > + size_t index = value / bucket_size; > > > + > > > + if (index >= size) > > > + index = size - 1; > > > > nit: It would be simpler to use max(). > > > > size_t index = max(value / bucket_size, size - 1); > > > Will do. > > > + ++data[index]; > > > +} > > > + > > > +/** > > > + * kvm_stats_log_hist_update() - Update bucket value for logarithmic histogram > > > + * statistics data. > > > + * > > > + * @data: start address of the stats data > > > + * @size: the number of bucket of the stats data > > > + * @value: the new value used to update the logarithmic histogram's bucket > > > + */ > > > +void kvm_stats_log_hist_update(u64 *data, size_t size, u64 value) > > > +{ > > > + size_t index = fls64(value); > > > + > > > + if (index >= size) > > > + index = size - 1; > > > > Ditto here about using max(). > > > Will do. > > > + ++data[index]; > > > +} > > > -- > > > 2.32.0.93.g670b81a890-goog > > > > Thanks, > Jing