Re: [PATCH] KVM: arm64: Add some statistics under kvm debugfs

[Marc Zyngier]

On Thu, 25 Jul 2024 04:27:38 +0100,
jianyong.wu@xxxxxxxxxxx wrote:
> 
> From: Jianyong Wu <jianyong.wu@xxxxxxxxxxx>
> 
> There are some statistics missing from kvm debugfs on arm64. For
> example: irq_exits number, irq_injection number and so on. But it's
> useful to monitoring kvm performance, so, add them here.
> 
> Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxxxxxx>
> ---
>  arch/arm64/include/asm/kvm_host.h | 3 +++
>  arch/arm64/kvm/guest.c            | 3 +++
>  arch/arm64/kvm/handle_exit.c      | 1 +
>  arch/arm64/kvm/sys_regs.c         | 1 +
>  arch/arm64/kvm/vgic/vgic.c        | 1 +
>  5 files changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 9e8a496fb284..1e20319977b3 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1024,6 +1024,9 @@ struct kvm_vcpu_stat {
>  	u64 mmio_exit_user;
>  	u64 mmio_exit_kernel;
>  	u64 signal_exits;
> +	u64 irq_exits;
> +	u64 irq_injections;
> +	u64 handle_sys_reg_exits;
>  	u64 exits;
>  };
>  
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index e2f762d959bb..a3783388a802 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -50,6 +50,9 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
>  	STATS_DESC_COUNTER(VCPU, mmio_exit_user),
>  	STATS_DESC_COUNTER(VCPU, mmio_exit_kernel),
>  	STATS_DESC_COUNTER(VCPU, signal_exits),
> +	STATS_DESC_COUNTER(VCPU, irq_exits),
> +	STATS_DESC_COUNTER(VCPU, irq_injections),
> +	STATS_DESC_COUNTER(VCPU, handle_sys_reg_exits),
>  	STATS_DESC_COUNTER(VCPU, exits)
>  };
>  
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 617ae6dea5d5..d69ab1207da4 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -335,6 +335,7 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
>  
>  	switch (exception_index) {
>  	case ARM_EXCEPTION_IRQ:
> +		++vcpu->stat.irq_exits;
>  		return 1;
>  	case ARM_EXCEPTION_EL1_SERROR:
>  		return 1;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index c9f4f387155f..2a6bfc673636 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -3568,6 +3568,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
>  	int Rt = kvm_vcpu_sys_get_rt(vcpu);
>  	int sr_idx;
>  
> +	++vcpu->stat.handle_sys_reg_exits;
>  	trace_kvm_handle_sys_reg(esr);
>  
>  	if (triage_sysreg_trap(vcpu, &sr_idx))
> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index 4ec93587c8cd..9f7371e01f35 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -454,6 +454,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
>  
>  	vgic_queue_irq_unlock(kvm, irq, flags);
>  	vgic_put_irq(kvm, irq);
> +	++vcpu->stat.irq_injections;

Like for most stats, I don't think this is warranted. Most of these
already have trace points associated to it, and you can readily
*count* those. These stats are an absolute waste of cycles and memory,
and I have suggested alternative approaches in the past (all of them
relying on the tracing infrastructure).

Thanks,

	M.

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