Re: [RFC PATCH 1/4] KVM: stats: Separate statistics name strings from debugfs code

[Marc Zyngier <maz@kernel.org>]

Hi Jing,

On Wed, 10 Mar 2021 00:30:21 +0000,
Jing Zhang <jingzhangos@google.com> wrote:
> 
> Prepare the statistics name strings for supporting binary format
> aggregated statistics data retrieval.
> 
> No functional change intended.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/kvm/guest.c    |  47 ++++--
>  arch/mips/kvm/mips.c      | 114 ++++++++++----
>  arch/powerpc/kvm/book3s.c | 107 +++++++++----
>  arch/powerpc/kvm/booke.c  |  84 +++++++---
>  arch/s390/kvm/kvm-s390.c  | 320 ++++++++++++++++++++++++++------------
>  arch/x86/kvm/x86.c        | 127 ++++++++++-----
>  include/linux/kvm_host.h  |  31 +++-
>  7 files changed, 589 insertions(+), 241 deletions(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 9bbd30e62799..fb3aafe76b52 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -28,19 +28,42 @@
>  
>  #include "trace.h"
>  
> +const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN] = {
> +	"remote_tlb_flush",
> +};
> +static_assert(sizeof(kvm_vm_stat_strings) ==
> +		VM_STAT_COUNT * KVM_STATS_NAME_LEN);
> +
> +const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN] = {
> +	"halt_successful_poll",
> +	"halt_attempted_poll",
> +	"halt_poll_success_ns",
> +	"halt_poll_fail_ns",
> +	"halt_poll_invalid",
> +	"halt_wakeup",
> +	"hvc_exit_stat",
> +	"wfe_exit_stat",
> +	"wfi_exit_stat",
> +	"mmio_exit_user",
> +	"mmio_exit_kernel",
> +	"exits",
> +};
> +static_assert(sizeof(kvm_vcpu_stat_strings) ==
> +		VCPU_STAT_COUNT * KVM_STATS_NAME_LEN);
> +
>  struct kvm_stats_debugfs_item debugfs_entries[] = {
> -	VCPU_STAT("halt_successful_poll", halt_successful_poll),
> -	VCPU_STAT("halt_attempted_poll", halt_attempted_poll),
> -	VCPU_STAT("halt_poll_invalid", halt_poll_invalid),
> -	VCPU_STAT("halt_wakeup", halt_wakeup),
> -	VCPU_STAT("hvc_exit_stat", hvc_exit_stat),
> -	VCPU_STAT("wfe_exit_stat", wfe_exit_stat),
> -	VCPU_STAT("wfi_exit_stat", wfi_exit_stat),
> -	VCPU_STAT("mmio_exit_user", mmio_exit_user),
> -	VCPU_STAT("mmio_exit_kernel", mmio_exit_kernel),
> -	VCPU_STAT("exits", exits),
> -	VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns),
> -	VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns),
> +	VCPU_STAT(halt_successful_poll),
> +	VCPU_STAT(halt_attempted_poll),
> +	VCPU_STAT(halt_poll_invalid),
> +	VCPU_STAT(halt_wakeup),
> +	VCPU_STAT(hvc_exit_stat),
> +	VCPU_STAT(wfe_exit_stat),
> +	VCPU_STAT(wfi_exit_stat),
> +	VCPU_STAT(mmio_exit_user),
> +	VCPU_STAT(mmio_exit_kernel),
> +	VCPU_STAT(exits),
> +	VCPU_STAT(halt_poll_success_ns),
> +	VCPU_STAT(halt_poll_fail_ns),

So we now have two arrays that can easily deviate in their order,
whilst we didn't have that risk before. What is the advantage of doing
this? The commit message doesn't really say...

[...]

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1b65e7204344..1ea297458306 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1162,6 +1162,18 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa)
>  	return kvm_is_error_hva(hva);
>  }
>  
> +#define VM_STAT_COUNT		(sizeof(struct kvm_vm_stat)/sizeof(ulong))
> +#define VCPU_STAT_COUNT		(sizeof(struct kvm_vcpu_stat)/sizeof(u64))
> +#define KVM_STATS_NAME_LEN	32
> +
> +/* Make sure it is synced with fields in struct kvm_vm_stat. */
> +extern const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN];
> +/* Make sure it is synced with fields in struct kvm_vcpu_stat. */
> +extern const char kvm_vcpu_stat_strings[][KVM_STATS_NAME_LEN];
> +
> +#define VM_STAT_NAME(id)        (kvm_vm_stat_strings[id])
> +#define VCPU_STAT_NAME(id)      (kvm_vcpu_stat_strings[id])
> +
>  enum kvm_stat_kind {
>  	KVM_STAT_VM,
>  	KVM_STAT_VCPU,
> @@ -1182,10 +1194,21 @@ struct kvm_stats_debugfs_item {
>  #define KVM_DBGFS_GET_MODE(dbgfs_item)                                         \
>  	((dbgfs_item)->mode ? (dbgfs_item)->mode : 0644)
>  
> -#define VM_STAT(n, x, ...) 							\
> -	{ n, offsetof(struct kvm, stat.x), KVM_STAT_VM, ## __VA_ARGS__ }
> -#define VCPU_STAT(n, x, ...)							\
> -	{ n, offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU, ## __VA_ARGS__ }
> +#define VM_STAT(x, ...)                                                        \
> +	{                                                                      \
> +		VM_STAT_NAME(offsetof(struct kvm_vm_stat, x)/sizeof(ulong)),   \
> +		offsetof(struct kvm, stat.x),                                  \
> +		KVM_STAT_VM,                                                   \
> +		## __VA_ARGS__                                                 \
> +	}
> +
> +#define VCPU_STAT(x, ...)                                                      \
> +	{                                                                      \
> +		VCPU_STAT_NAME(offsetof(struct kvm_vcpu_stat, x)/sizeof(u64)), \
> +		offsetof(struct kvm_vcpu, stat.x),                             \
> +		KVM_STAT_VCPU,                                                 \
> +		## __VA_ARGS__                                                 \
> +	}

Is there any reason why we want to keep kvm_vm_stat populated with
ulong, while kvm_vcpu_stat is populated with u64? I have the feeling
that this is a fairly pointless difference, and that some of the
macros could be unified.

Also, using names initialisers would help with the readability of the
macros.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.