| Message ID | 20210319161711.24972-3-yoan.picchi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: add more event counters for kvm_stat | expand |
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.
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), VCPU_STAT("exits", exits), VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns), VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns), diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 3996b28da..55d7fe63b 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -80,6 +80,17 @@ static bool memslot_is_logging(struct kvm_memory_slot *memslot) */ void kvm_flush_remote_tlbs(struct kvm *kvm) { + ulong old, new; + /* + * This is not always called in thread safe code so we need an atomic add + * or a lock. The two would pollute otherwise clean code for just a counter + * so we use an older atomic primitive that work on the counter data type. + */ + do { + old = kvm->stat.remote_tlb_flush; + new = old + 1; + } while (cmpxchg(&(kvm->stat.remote_tlb_flush), old, new) != old); + kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu); }
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. 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) 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. 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(+)