Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <90e2ebcd-f411-8d44-bf44-5df80fb59b51@linux.ibm.com>
Date:   Mon, 18 Jul 2022 11:34:08 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH] kvm: stats: tell userspace which values are boolean
Content-Language: en-US
To:     Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     Amneesh Singh <natto@weirdnatto.in>
References: <20220714120330.1410308-1-pbonzini@redhat.com>
From:   Janis Schoetterl-Glausch <scgl@linux.ibm.com>
In-Reply-To: <20220714120330.1410308-1-pbonzini@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/14/22 14:03, Paolo Bonzini wrote:
> Some of the statistics values exported by KVM are always only 0 or 1.
> It can be useful to export this fact to userspace so that it can track
> them specially (for example by polling the value every now and then to
> compute a % of time spent in a specific state).
> 
> Therefore, add "boolean value" as a new "unit".  While it is not exactly
> a unit, it walks and quacks like one.  In particular, using the type
> would be wrong because boolean values could be instantaneous or peak
> values (e.g. "is the rmap allocated?") or even two-bucket histograms
> (e.g. "number of posted vs. non-posted interrupt injections").
> 
> Suggested-by: Amneesh Singh <natto@weirdnatto.in>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  Documentation/virt/kvm/api.rst |  6 ++++++
>  arch/x86/kvm/x86.c             |  2 +-
>  include/linux/kvm_host.h       | 11 ++++++++++-
>  include/uapi/linux/kvm.h       |  1 +
>  4 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 11e00a46c610..48bf6e49a7de 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5657,6 +5657,7 @@ by a string of size ``name_size``.
>  	#define KVM_STATS_UNIT_BYTES		(0x1 << KVM_STATS_UNIT_SHIFT)
>  	#define KVM_STATS_UNIT_SECONDS		(0x2 << KVM_STATS_UNIT_SHIFT)
>  	#define KVM_STATS_UNIT_CYCLES		(0x3 << KVM_STATS_UNIT_SHIFT)
> +	#define KVM_STATS_UNIT_BOOLEAN		(0x4 << KVM_STATS_UNIT_SHIFT)
>  	#define KVM_STATS_UNIT_MAX		KVM_STATS_UNIT_CYCLES
> 
>  	#define KVM_STATS_BASE_SHIFT		8

[...]

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 5088bd9f1922..811897dadcae 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -2083,6 +2083,7 @@ struct kvm_stats_header {
>  #define KVM_STATS_UNIT_BYTES		(0x1 << KVM_STATS_UNIT_SHIFT)
>  #define KVM_STATS_UNIT_SECONDS		(0x2 << KVM_STATS_UNIT_SHIFT)
>  #define KVM_STATS_UNIT_CYCLES		(0x3 << KVM_STATS_UNIT_SHIFT)
> +#define KVM_STATS_UNIT_BOOLEAN		(0x4 << KVM_STATS_UNIT_SHIFT)
>  #define KVM_STATS_UNIT_MAX		KVM_STATS_UNIT_CYCLES

Shouldn't KVM_STATS_UNIT_MAX be KVM_STATS_UNIT_BOOLEAN then?

The selftest has:
	TEST_ASSERT((pdesc->flags & KVM_STATS_UNIT_MASK)
                                <= KVM_STATS_UNIT_MAX, "Unknown KVM stats unit");
> 
>  #define KVM_STATS_BASE_SHIFT		8