[PATCH 0/7] KVM: arm64: add more event counters for kvm_stat

Hi all,

As mentioned in the KVM forum talk from 2019 
(https://kvmforum2019.sched.com/event/Tmwf/kvmstat-and-beyond-past-present-and-future-of-performance-monitoring-christian-borntrager-ibm page 10),
there is few event counters for kvm_stat in the arm64 version of kvm when
you compare it to something like the x86 version.
Those counters are used in kvm_stat by kernel/driver developers to
have a rough idea of the impact of their patches on the general performance.
An example would be to make sure a patch don't increase to much the amount
of interruptions. Those patches aim to add more counters to make the use of
kvm_stat more relevant when measuring performance impact.

I am new in working on kernel-related things and I am learning kvm as I go.
Some of the counter I added early (memory_slot_unmaped, stage2_unmap_vm)
no longer seems relevant because while they do interesting things, they
happens in very specific scenarios. Instead of just deleting them, I prefer
to ask weither a little-used counter or no counter is the best.
I can also use some suggestion on how to test those counters as some like 
remote_tlb_flush which mostly happen when fixing up a race condition; or
what uncovered event could be worth adding in a future patch set.

Yoan Picchi (7):
  KVM: arm64: Add two page mapping counters for kvm_stat
  KVM: arm64: Add remote_tlb_flush counter for kvm_stat
  KVM: arm64: Add cached_page_invalidated counter for kvm_stat
  KVM: arm64: Add flush_all_cache_lines counter for kvm_stat
  KVM: arm64: Add memory_slot_unmaped counter for kvm_stat
  KVM: arm64: Add stage2_unmap_vm counter for kvm_stat
  KVM: arm64: Add irq_inject counter for kvm_stat

 arch/arm64/include/asm/kvm_host.h |  8 ++++++++
 arch/arm64/kvm/arm.c              |  2 ++
 arch/arm64/kvm/guest.c            |  9 +++++++++
 arch/arm64/kvm/mmu.c              | 27 ++++++++++++++++++++++++++-
 arch/arm64/kvm/vgic/vgic.c        |  2 ++
 5 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.17.1


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

Add a counter for when a regular page is mapped, and another
for when a huge page is mapped.

Signed-off-by: Yoan Picchi <redacted>
---
 arch/arm64/include/asm/kvm_host.h | 2 ++
 arch/arm64/kvm/guest.c            | 2 ++
 arch/arm64/kvm/mmu.c              | 7 +++++++
 3 files changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3d10e6527..863603285 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -561,6 +561,8 @@ struct kvm_vcpu_stat {
 	u64 wfi_exit_stat;
 	u64 mmio_exit_user;
 	u64 mmio_exit_kernel;
+	u64 regular_page_mapped;
+	u64 huge_page_mapped;
 	u64 exits;
 };
 

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 9bbd30e62..14b15fb8f 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -38,6 +38,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	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("regular_page_mapped", regular_page_mapped),
+	VCPU_STAT("huge_page_mapped", huge_page_mapped),
 	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 77cb2d28f..3996b28da 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c

@@ -914,6 +914,13 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		mark_page_dirty(kvm, gfn);
 	}
 
+	if (ret >= 0) {
+		if (vma_pagesize == PAGE_SIZE) {
+			vcpu->stat.regular_page_mapped++;
+		} else {
+			vcpu->stat.huge_page_mapped++;
+		}
+	}
 out_unlock:
 	spin_unlock(&kvm->mmu_lock);
 	kvm_set_pfn_accessed(pfn);

-- 
2.17.1


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

Hi Yoan,

On 2021/3/20 0:17, Yoan Picchi wrote:

This looks too simple, as we don't always map a new regular_page or huge_page here.
We may just relax permission or change mapping size, etc. If we mix these operations,
the stat result seems not very valuable...

Thanks,
Keqian

 out_unlock:
 	spin_unlock(&kvm->mmu_lock);
 	kvm_set_pfn_accessed(pfn);

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

On Tue, 23 Mar 2021 11:52:38 +0000,
Keqian Zhu [off-list ref] wrote:

Hi Yoan,

On 2021/3/20 0:17, Yoan Picchi wrote:

quoted

Add a counter for when a regular page is mapped, and another
for when a huge page is mapped.

Signed-off-by: Yoan Picchi <redacted>
---
 arch/arm64/include/asm/kvm_host.h | 2 ++
 arch/arm64/kvm/guest.c            | 2 ++
 arch/arm64/kvm/mmu.c              | 7 +++++++
 3 files changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3d10e6527..863603285 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -561,6 +561,8 @@ struct kvm_vcpu_stat {
 	u64 wfi_exit_stat;
 	u64 mmio_exit_user;
 	u64 mmio_exit_kernel;
+	u64 regular_page_mapped;
+	u64 huge_page_mapped;
 	u64 exits;
 };
 

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 9bbd30e62..14b15fb8f 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -38,6 +38,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	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("regular_page_mapped", regular_page_mapped),
+	VCPU_STAT("huge_page_mapped", huge_page_mapped),
 	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 77cb2d28f..3996b28da 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c

@@ -914,6 +914,13 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		mark_page_dirty(kvm, gfn);
 	}
 
+	if (ret >= 0) {
+		if (vma_pagesize == PAGE_SIZE) {
+			vcpu->stat.regular_page_mapped++;
+		} else {
+			vcpu->stat.huge_page_mapped++;
+		}
+	}

This looks too simple, as we don't always map a new regular_page or
huge_page here.  We may just relax permission or change mapping
size, etc. If we mix these operations, the stat result seems not
very valuable...

I can only agree. There is no such thing as a *huge page*. We have a
whole gamut of block mappings depending on the base granule size, and
we even have concatenation of pages/blocks (not yet implemented at S2,
but there is hope...).

What is this trying to express?

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

Add some counter for when a dpage get invalidated. The counter isn't
in the function that actually do it though because it doesn't have
either a kvm or vcpu argument, so we would have no way to access the
counters. For this reason, the counter have been added to the calling
functions instead.

Signed-off-by: Yoan Picchi <redacted>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/guest.c            | 1 +
 arch/arm64/kvm/mmu.c              | 5 ++++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 863603285..3609aa89d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -547,6 +547,7 @@ static inline bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
 
 struct kvm_vm_stat {
 	ulong remote_tlb_flush;
+	ulong cached_page_invalidated;
 };
 
 struct kvm_vcpu_stat {

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 1029976ca..f6b1f0b63 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -41,6 +41,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	VCPU_STAT("regular_page_mapped", regular_page_mapped),
 	VCPU_STAT("huge_page_mapped", huge_page_mapped),
 	VM_STAT("remote_tlb_flush", remote_tlb_flush),
+	VM_STAT("cached_page_invalidated", cached_page_invalidated),
 	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 55d7fe63b..d6ddf5ab8 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c

@@ -893,8 +893,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	if (writable)
 		prot |= KVM_PGTABLE_PROT_W;
 
-	if (fault_status != FSC_PERM && !device)
+	if (fault_status != FSC_PERM && !device) {
 		clean_dcache_guest_page(pfn, vma_pagesize);
+		kvm->stat.cached_page_invalidated++;
+	}
 
 	if (exec_fault) {
 		prot |= KVM_PGTABLE_PROT_X;

@@ -1166,6 +1168,7 @@ int kvm_set_spte_hva(struct kvm *kvm, unsigned long hva, pte_t pte)
 	 * just like a translation fault and clean the cache to the PoC.
 	 */
 	clean_dcache_guest_page(pfn, PAGE_SIZE);
+	kvm->stat.cached_page_invalidated++;
 	handle_hva_to_gpa(kvm, hva, end, &kvm_set_spte_handler, &pfn);
 	return 0;
 }

-- 
2.17.1


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

On Fri, 19 Mar 2021 16:17:07 +0000,
Yoan Picchi [off-list ref] wrote:

Given that PoC flushing is only done on translation fault, what are
the odds that this would report a different number than that of
translation faults (assuming it was actually implemented)?

It is also interesting that you attribute the same cost to flushing a
4kB page or a 1GB block. And what does this mean when either FWB or
IDC are available?

	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

Add a counter that triggers when all the cache are flushed. This happens
when toggling caching for instance.

Signed-off-by: Yoan Picchi <redacted>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/guest.c            | 1 +
 arch/arm64/kvm/mmu.c              | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3609aa89d..e6967951f 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -547,6 +547,7 @@ static inline bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
 
 struct kvm_vm_stat {
 	ulong remote_tlb_flush;
+	ulong flush_all_cache_lines;
 	ulong cached_page_invalidated;
 };
 

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index f6b1f0b63..d948c3bd2 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -41,6 +41,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	VCPU_STAT("regular_page_mapped", regular_page_mapped),
 	VCPU_STAT("huge_page_mapped", huge_page_mapped),
 	VM_STAT("remote_tlb_flush", remote_tlb_flush),
+	VM_STAT("flush_all_cache_lines", flush_all_cache_lines),
 	VM_STAT("cached_page_invalidated", cached_page_invalidated),
 	VCPU_STAT("exits", exits),
 	VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns),

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index d6ddf5ab8..985b048df 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c

@@ -181,6 +181,7 @@ static void stage2_flush_vm(struct kvm *kvm)
 	kvm_for_each_memslot(memslot, slots)
 		stage2_flush_memslot(kvm, memslot);
 
+	kvm->stat.flush_all_cache_lines++;
 	spin_unlock(&kvm->mmu_lock);
 	srcu_read_unlock(&kvm->srcu, idx);
 }

-- 
2.17.1


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

On Fri, 19 Mar 2021 16:17:08 +0000,
Yoan Picchi [off-list ref] wrote:

Add a counter that triggers when all the cache are flushed. This happens
when toggling caching for instance.

Which cache?

What additional information does this give over the other d-side flush
counter?

	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

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 <redacted>
---
 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),
 	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);
 }
 

-- 
2.17.1


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

On Fri, 19 Mar 2021 16:17:06 +0000,
Yoan Picchi [off-list ref] 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?

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

Add a counter for when the one want to unmap all the ram of a VM.
This mostly happens when one restart a VM so we make sure to clear
the ram and free the memory for other VMs.

Signed-off-by: Yoan Picchi <redacted>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/guest.c            | 1 +
 arch/arm64/kvm/mmu.c              | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 76c81aa79..fa59b669c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -549,6 +549,7 @@ struct kvm_vm_stat {
 	ulong remote_tlb_flush;
 	ulong flush_all_cache_lines;
 	ulong memory_slot_unmaped;
+	ulong stage2_unmap_vm;
 	ulong cached_page_invalidated;
 };
 

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index cd227136e..129c0d53d 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -43,6 +43,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	VM_STAT("remote_tlb_flush", remote_tlb_flush),
 	VM_STAT("flush_all_cache_lines", flush_all_cache_lines),
 	VM_STAT("memory_slot_unmaped", memory_slot_unmaped),
+	VM_STAT("stage2_unmap_vm", stage2_unmap_vm),
 	VM_STAT("cached_page_invalidated", cached_page_invalidated),
 	VCPU_STAT("exits", exits),
 	VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns),

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 1e8aeafc2..6d150a785 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c

@@ -471,6 +471,8 @@ void stage2_unmap_vm(struct kvm *kvm)
 	mmap_read_lock(current->mm);
 	spin_lock(&kvm->mmu_lock);
 
+	kvm->stat.stage2_unmap_vm++;
+
 	slots = kvm_memslots(kvm);
 	kvm_for_each_memslot(memslot, slots)
 		stage2_unmap_memslot(kvm, memslot);

-- 
2.17.1


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

On Fri, 19 Mar 2021 16:17:10 +0000,
Yoan Picchi [off-list ref] wrote:

Add a counter for when the one want to unmap all the ram of a VM.
This mostly happens when one restart a VM so we make sure to clear
the ram and free the memory for other VMs.

Again: unmapping memory from a VM doesn't make it free for anyone
else.

	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

Add a counter for when a memory slot is unmapped, meaning that all
memory belonging to a specific VM is made available to be mapped for
anoother VM. A memory slot is the memory that can be used to hold a
guest's pages and can be made available by Qemu for instance. For
now the memory slot are only unmapped when a vm restarts.

Signed-off-by: Yoan Picchi <redacted>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/guest.c            | 1 +
 arch/arm64/kvm/mmu.c              | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index e6967951f..76c81aa79 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -548,6 +548,7 @@ static inline bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
 struct kvm_vm_stat {
 	ulong remote_tlb_flush;
 	ulong flush_all_cache_lines;
+	ulong memory_slot_unmaped;
 	ulong cached_page_invalidated;
 };
 

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index d948c3bd2..cd227136e 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -42,6 +42,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	VCPU_STAT("huge_page_mapped", huge_page_mapped),
 	VM_STAT("remote_tlb_flush", remote_tlb_flush),
 	VM_STAT("flush_all_cache_lines", flush_all_cache_lines),
+	VM_STAT("memory_slot_unmaped", memory_slot_unmaped),
 	VM_STAT("cached_page_invalidated", cached_page_invalidated),
 	VCPU_STAT("exits", exits),
 	VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns),

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 985b048df..1e8aeafc2 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c

@@ -419,6 +419,7 @@ static void stage2_unmap_memslot(struct kvm *kvm,
 	phys_addr_t addr = memslot->base_gfn << PAGE_SHIFT;
 	phys_addr_t size = PAGE_SIZE * memslot->npages;
 	hva_t reg_end = hva + size;
+	kvm->stat.memory_slot_unmaped++;
 
 	/*
 	 * A memory region could potentially cover multiple VMAs, and any holes

-- 
2.17.1


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

On Fri, 19 Mar 2021 16:17:09 +0000,
Yoan Picchi [off-list ref] wrote:

Add a counter for when a memory slot is unmapped, meaning that all
memory belonging to a specific VM is made available to be mapped for
anoother VM.

No. It just means it is unmapped from the guest. The VMM still has the
pages.

Why is that a relevant thing to track? You even increment the counter
before even checking whether this will unmap anything... :-(

	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

Add a counter for interrupt injections. That is when kvm relay an
interrupt to the guest (for instance a timer, or a device interrupt
like from a network card)

Signed-off-by: Yoan Picchi <redacted>
---
 arch/arm64/include/asm/kvm_host.h | 2 ++
 arch/arm64/kvm/arm.c              | 2 ++
 arch/arm64/kvm/guest.c            | 2 ++
 arch/arm64/kvm/vgic/vgic.c        | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index fa59b669c..253acb8c2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -551,6 +551,7 @@ struct kvm_vm_stat {
 	ulong memory_slot_unmaped;
 	ulong stage2_unmap_vm;
 	ulong cached_page_invalidated;
+	ulong irq_inject;
 };
 
 struct kvm_vcpu_stat {

@@ -567,6 +568,7 @@ struct kvm_vcpu_stat {
 	u64 mmio_exit_kernel;
 	u64 regular_page_mapped;
 	u64 huge_page_mapped;
+	u64 irq_inject;
 	u64 exits;
 };
 

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index fc4c95dd2..841551f14 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c

@@ -895,6 +895,8 @@ static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
 	bool set;
 	unsigned long *hcr;
 
+	vcpu->stat.irq_inject++;
+
 	if (number == KVM_ARM_IRQ_CPU_IRQ)
 		bit_index = __ffs(HCR_VI);
 	else /* KVM_ARM_IRQ_CPU_FIQ */

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 129c0d53d..f663b03ae 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -45,6 +45,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
 	VM_STAT("memory_slot_unmaped", memory_slot_unmaped),
 	VM_STAT("stage2_unmap_vm", stage2_unmap_vm),
 	VM_STAT("cached_page_invalidated", cached_page_invalidated),
+	VM_STAT("irq_inject", irq_inject),
+	VCPU_STAT("irq_inject", irq_inject),
 	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/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 1c597c988..9e504243b 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c

@@ -458,6 +458,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 
 	raw_spin_lock_irqsave(&irq->irq_lock, flags);
 
+	kvm->stat.irq_inject++;
+
 	if (!vgic_validate_injection(irq, level, owner)) {
 		/* Nothing to see here, move along... */
 		raw_spin_unlock_irqrestore(&irq->irq_lock, flags);

-- 
2.17.1


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

On Fri, 19 Mar 2021 16:17:11 +0000,
Yoan Picchi [off-list ref] wrote:

So even if the injection failed, you report an injection? And what
about injection that occur via the MMIO interface? What about direct
injection? What about a level interrupt that is forever high?

	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

Hi Mark.

Thanks for all the reviews. I am a beginner and you gave me a lot to 
learn about.
I will reply to the other patch progressively once I understand better 
the issues.

On 23/03/2021 17:37, Marc Zyngier wrote:

On Fri, 19 Mar 2021 16:17:11 +0000,
Yoan Picchi [off-list ref] wrote:

quoted

Add a counter for interrupt injections. That is when kvm relay an
interrupt to the guest (for instance a timer, or a device interrupt
like from a network card)

Signed-off-by: Yoan Picchi <redacted>
---
  arch/arm64/include/asm/kvm_host.h | 2 ++
  arch/arm64/kvm/arm.c              | 2 ++
  arch/arm64/kvm/guest.c            | 2 ++
  arch/arm64/kvm/vgic/vgic.c        | 2 ++
  4 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index fa59b669c..253acb8c2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -551,6 +551,7 @@ struct kvm_vm_stat {
  	ulong memory_slot_unmaped;
  	ulong stage2_unmap_vm;
  	ulong cached_page_invalidated;
+	ulong irq_inject;
  };
  
  struct kvm_vcpu_stat {

@@ -567,6 +568,7 @@ struct kvm_vcpu_stat {
  	u64 mmio_exit_kernel;
  	u64 regular_page_mapped;
  	u64 huge_page_mapped;
+	u64 irq_inject;
  	u64 exits;
  };
  

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index fc4c95dd2..841551f14 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c

@@ -895,6 +895,8 @@ static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level)
  	bool set;
  	unsigned long *hcr;
  
+	vcpu->stat.irq_inject++;
+
  	if (number == KVM_ARM_IRQ_CPU_IRQ)
  		bit_index = __ffs(HCR_VI);
  	else /* KVM_ARM_IRQ_CPU_FIQ */

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 129c0d53d..f663b03ae 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -45,6 +45,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
  	VM_STAT("memory_slot_unmaped", memory_slot_unmaped),
  	VM_STAT("stage2_unmap_vm", stage2_unmap_vm),
  	VM_STAT("cached_page_invalidated", cached_page_invalidated),
+	VM_STAT("irq_inject", irq_inject),
+	VCPU_STAT("irq_inject", irq_inject),
  	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/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 1c597c988..9e504243b 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c

@@ -458,6 +458,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
  
  	raw_spin_lock_irqsave(&irq->irq_lock, flags);
  
+	kvm->stat.irq_inject++;
+
  	if (!vgic_validate_injection(irq, level, owner)) {

So even if the injection failed, you report an injection? And what
about injection that occur via the MMIO interface? What about direct
injection? What about a level interrupt that is forever high?

	M.

This one I actually started to fix this afternoon by moving the counter 
into vgic_queue_irq_unlock().
This way it is only incremented when the interrupt is inserted into a 
vcpu, and it also takes care of the
vgic_mmio injections. I also fixed the issue with the interrupt line so 
it only increment when the line
change of level.

I'm not sure about what you mean by direct injection yet though.

Kindly,
Yoan


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

On Tue, 23 Mar 2021 17:53:42 +0000,
Yoan Picchi [off-list ref] wrote:

Hi Mark.

s/k/c/, please!

Thanks for all the reviews. I am a beginner and you gave me a lot to
learn about.  I will reply to the other patch progressively once I
understand better the issues.

I think you should consider what I said in my reply to the cover
letter before going all out on every counter you have introduced in
this series.

[...]

quoted

quoted

diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 1c597c988..9e504243b 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c

@@ -458,6 +458,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
    	raw_spin_lock_irqsave(&irq->irq_lock, flags);
  +	kvm->stat.irq_inject++;
+
  	if (!vgic_validate_injection(irq, level, owner)) {

So even if the injection failed, you report an injection? And what
about injection that occur via the MMIO interface? What about direct
injection? What about a level interrupt that is forever high?

	M.

This one I actually started to fix this afternoon by moving the
counter into vgic_queue_irq_unlock().  This way it is only
incremented when the interrupt is inserted into a vcpu, and it also
takes care of the vgic_mmio injections. I also fixed the issue with
the interrupt line so it only increment when the line change of
level.

But if you do that, you start counting interrupts the guest itself
generates. What is the exact semantic of this counter? userspace
injected interrupts? Acked interrupts? Any interrupt?

Take my level interrupt example. The interrupt will be forever
pending, the guest will take as many interrupt as it can process, and
yet your counter will have been incremented *once*. What does your
counter mean then?

I'm not sure about what you mean by direct injection yet though.

GICv4.{0,1}, where an interrupt gets directly delivered to the guest
without (too much) SW intervention. With this, directly injected LPIs
will never result in the counter being incremented, and yet can pin
the guest to the ground under interrupt load.

Again, defining the exact behaviour of the counter would avoid me
ranting away...

	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

Hi Yoan,

On Fri, 19 Mar 2021 16:17:04 +0000,
Yoan Picchi [off-list ref] wrote:

Hi all,

As mentioned in the KVM forum talk from 2019
(https://kvmforum2019.sched.com/event/Tmwf/kvmstat-and-beyond-past-present-and-future-of-performance-monitoring-christian-borntrager-ibm
page 10), there is few event counters for kvm_stat in the arm64
version of kvm when you compare it to something like the x86
version.

Crucially, these differences exist because there is no universal
equivalence between architecture features. A TLB invalidation on x86
doesn't do the same thing as on PPC nor arm64.

Those counters are used in kvm_stat by kernel/driver developers to
have a rough idea of the impact of their patches on the general performance.
An example would be to make sure a patch don't increase to much the amount
of interruptions. Those patches aim to add more counters to make the use of
kvm_stat more relevant when measuring performance impact.

Adding more counters only make sense if the semantic of these counters
is clearly defined. In this series, you have sprayed a bunch of x++ in
random places, without defining what you were trying to count.

I am new in working on kernel-related things and I am learning kvm as I go.
Some of the counter I added early (memory_slot_unmaped, stage2_unmap_vm)
no longer seems relevant because while they do interesting things, they
happens in very specific scenarios. Instead of just deleting them, I prefer
to ask weither a little-used counter or no counter is the best.

It works the other way around: the onus is on you to explain *why* we
should even consider a counter or another.

May I suggest that you start by picking exactly *one* metric, work out
exactly what it is supposed to count, what significance it has at the
architectural level, what it actually brings to the user? Then try and
make a good job at implementing these semantics. You will learn about
the arm64 architecture and KVM in one swift go, one area at a time.

I can also use some suggestion on how to test those counters as some like
remote_tlb_flush which mostly happen when fixing up a race condition; or
what uncovered event could be worth adding in a future patch set.

remote_tlb_flush is a great example of something that really *doesn't*
make much sense on KVM/arm64. We don't deal with remote TLBs
independently of the local ones outside of vcpu migration (which you
don't cover either).

I can only urge you to focus on the architectural meaning of the
metric you picked, and see how it maps across the hypervisor.

Finally, there is the question of the general availability of these
counters. They live in debugfs, which isn't a proper userspace
ABI. There is work going on around making this a more palatable
interface, and I'd rather see where this is going before expanding the
number of counters.

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