Re: [PATCH 2/7] KVM: arm64: Add remote_tlb_flush counter for kvm_stat

[Marc Zyngier <maz@kernel.org>]

On Fri, 19 Mar 2021 16:17:06 +0000,
Yoan Picchi <yoan.picchi@arm.com> wrote:
> 
> Add a counter for remote tlb flushes.
> I think flushing the tlb is important enough of a thing so that one using
> kvm_stat should be aware if their code is trigering several flushes.
> Beside the event is recorded in x86 and ppc as well so there might be
> even more reasons that I can't think of.

And does this stat mean the same thing across architectures?

> Looking at where this is called, it mostly happen when someone is
> updating the dirty pages while we are doing some operation on them
> (like enabling dirty pages logging)

How about swapping, KSM, VM teardown?

> 
> There's one catch though, it is not always thread safe. Sometime it is
> called under some lock, some other time it isn't.
> We can't change the counter to an atomic_t as all the counters are
> unsigned. We shouldn't add a lock as this could be adding a lock
> (say, to kvm->arch) for a very minor thing and I would rather not pollute
> anything without a better reason. That's why I ended up using cmpxchg
> which according to LWN (https://lwn.net/Articles/695257/) is an old way
> to do without atomic. It's less efficient than an atomic increment, but
> this should happen very rarely anyway and is stil better than a lock.

Are you actually worried about this stat being exact?

> Signed-off-by: Yoan Picchi <yoan.picchi@arm.com>
> ---
>  arch/arm64/kvm/guest.c |  1 +
>  arch/arm64/kvm/mmu.c   | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 14b15fb8f..1029976ca 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -40,6 +40,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  	VCPU_STAT("mmio_exit_kernel", mmio_exit_kernel),
>  	VCPU_STAT("regular_page_mapped", regular_page_mapped),
>  	VCPU_STAT("huge_page_mapped", huge_page_mapped),
> +	VM_STAT("remote_tlb_flush", remote_tlb_flush),

Two things:

- having a VM_STAT stuck in the middle on a set of per-vcpu stats
  isn't great

- what does "remote TLB flush" means for the ARM architecture, which
  uses broadcast invalidation?  Slapping foreign concepts in the
  architecture code doesn't strike me as the best course of action

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel