diff mbox series

[v2,01/23] KVM: arm64: Add tracepoints + stats for LPI cache effectiveness

Message ID 20240213093250.3960069-2-oliver.upton@linux.dev (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Improvements to LPI injection | expand

Commit Message

Oliver Upton Feb. 13, 2024, 9:32 a.m. UTC
LPI translation and injection has been shown to have a significant
impact on the performance of VM workloads, so it probably makes sense to
add some signals in this area.

Introduce the concept of a KVM tracepoint that associates with a VM
stat and use it for the LPI translation cache tracepoints. It isn't too
uncommon for a kernel hacker to attach to tracepoints, while at the same
time userspace may open a 'binary stats' FD to peek at the corresponding
VM stats.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_host.h |  3 ++
 arch/arm64/kvm/guest.c            |  5 ++-
 arch/arm64/kvm/vgic/trace.h       | 66 +++++++++++++++++++++++++++++++
 arch/arm64/kvm/vgic/vgic-its.c    | 14 ++++++-
 include/linux/kvm_host.h          |  4 ++
 5 files changed, 89 insertions(+), 3 deletions(-)

Comments

Marc Zyngier Feb. 14, 2024, 3:55 p.m. UTC | #1
On Tue, 13 Feb 2024 09:32:38 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> LPI translation and injection has been shown to have a significant
> impact on the performance of VM workloads, so it probably makes sense to
> add some signals in this area.
> 
> Introduce the concept of a KVM tracepoint that associates with a VM
> stat and use it for the LPI translation cache tracepoints. It isn't too
> uncommon for a kernel hacker to attach to tracepoints, while at the same
> time userspace may open a 'binary stats' FD to peek at the corresponding
> VM stats.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h |  3 ++
>  arch/arm64/kvm/guest.c            |  5 ++-
>  arch/arm64/kvm/vgic/trace.h       | 66 +++++++++++++++++++++++++++++++
>  arch/arm64/kvm/vgic/vgic-its.c    | 14 ++++++-
>  include/linux/kvm_host.h          |  4 ++
>  5 files changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 21c57b812569..6f88b76373a5 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -966,6 +966,9 @@ static inline bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
>  
>  struct kvm_vm_stat {
>  	struct kvm_vm_stat_generic generic;
> +	u64 vgic_its_trans_cache_hit;
> +	u64 vgic_its_trans_cache_miss;
> +	u64 vgic_its_trans_cache_victim;
>  };
>  
>  struct kvm_vcpu_stat {
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index aaf1d4939739..354d67251fc2 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -30,7 +30,10 @@
>  #include "trace.h"
>  
>  const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
> -	KVM_GENERIC_VM_STATS()
> +	KVM_GENERIC_VM_STATS(),
> +	STATS_DESC_COUNTER(VM, vgic_its_trans_cache_hit),
> +	STATS_DESC_COUNTER(VM, vgic_its_trans_cache_miss),
> +	STATS_DESC_COUNTER(VM, vgic_its_trans_cache_victim)
>  };

We've talked about this offline, but I thought I'd make my position
public on these.

I've very concerned that exposing these statistic, however useful they
may be at a given point in time, will eventually become all wrong
and/or misleading.

Case in point, our discussion about this very series, where we landed
on a potential reimplementation of the translation cache as a per-ITS
xarray. If this comes to fruition, these stats will probably be
totally useless (hit monotonically increasing at the rate of 1 per
interrupt, miss being 0 or 1, victim being stuck to 0).

I'm *not* saying that such stats are totally useless. Just that they
are, by definition, tied to a kernel-side implementation choice, which
will evolve. The trouble is that they create an ABI that we need to
support forever.

I wish we could work on something that would allow such statistics to
be *extracted* (as opposed to published). Something like this as a
discussion subject for a future KVM Forum or LPC uConf would get my
full backing (and I'm pretty sure the networking folks have solved
that problem a long time ago).

Thanks,

	M.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 21c57b812569..6f88b76373a5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -966,6 +966,9 @@  static inline bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
 
 struct kvm_vm_stat {
 	struct kvm_vm_stat_generic generic;
+	u64 vgic_its_trans_cache_hit;
+	u64 vgic_its_trans_cache_miss;
+	u64 vgic_its_trans_cache_victim;
 };
 
 struct kvm_vcpu_stat {
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index aaf1d4939739..354d67251fc2 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -30,7 +30,10 @@ 
 #include "trace.h"
 
 const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
-	KVM_GENERIC_VM_STATS()
+	KVM_GENERIC_VM_STATS(),
+	STATS_DESC_COUNTER(VM, vgic_its_trans_cache_hit),
+	STATS_DESC_COUNTER(VM, vgic_its_trans_cache_miss),
+	STATS_DESC_COUNTER(VM, vgic_its_trans_cache_victim)
 };
 
 const struct kvm_stats_header kvm_vm_stats_header = {
diff --git a/arch/arm64/kvm/vgic/trace.h b/arch/arm64/kvm/vgic/trace.h
index 83c64401a7fc..ff6423f22c91 100644
--- a/arch/arm64/kvm/vgic/trace.h
+++ b/arch/arm64/kvm/vgic/trace.h
@@ -27,6 +27,72 @@  TRACE_EVENT(vgic_update_irq_pending,
 		  __entry->vcpu_id, __entry->irq, __entry->level)
 );
 
+TRACE_EVENT(vgic_its_trans_cache_hit,
+	TP_PROTO(__u64 db_addr, __u32 device_id, __u32 event_id, __u32 intid),
+	TP_ARGS(db_addr, device_id, event_id, intid),
+
+	TP_STRUCT__entry(
+		__field(	__u64,		db_addr		)
+		__field(	__u32,		device_id	)
+		__field(	__u32,		event_id	)
+		__field(	__u32,		intid		)
+	),
+
+	TP_fast_assign(
+		__entry->db_addr	= db_addr;
+		__entry->device_id	= device_id;
+		__entry->event_id	= event_id;
+		__entry->intid		= intid;
+	),
+
+	TP_printk("DB: %016llx, device_id %u, event_id %u, intid %u",
+                  __entry->db_addr, __entry->device_id, __entry->event_id,
+                  __entry->intid)
+);
+
+TRACE_EVENT(vgic_its_trans_cache_miss,
+	TP_PROTO(__u64 db_addr, __u32 device_id, __u32 event_id),
+	TP_ARGS(db_addr, device_id, event_id),
+
+	TP_STRUCT__entry(
+		__field(	__u64,		db_addr		)
+		__field(	__u32,		device_id	)
+		__field(	__u32,		event_id	)
+	),
+
+	TP_fast_assign(
+		__entry->db_addr	= db_addr;
+		__entry->device_id	= device_id;
+		__entry->event_id	= event_id;
+	),
+
+	TP_printk("DB: %016llx, device_id %u, event_id %u",
+                  __entry->db_addr, __entry->device_id, __entry->event_id)
+);
+
+TRACE_EVENT(vgic_its_trans_cache_victim,
+	TP_PROTO(__u64 db_addr, __u32 device_id, __u32 event_id, __u32 intid),
+	TP_ARGS(db_addr, device_id, event_id, intid),
+
+	TP_STRUCT__entry(
+		__field(	__u64,		db_addr		)
+		__field(	__u32,		device_id	)
+		__field(	__u32,		event_id	)
+		__field(	__u32,		intid		)
+	),
+
+	TP_fast_assign(
+		__entry->db_addr	= db_addr;
+		__entry->device_id	= device_id;
+		__entry->event_id	= event_id;
+		__entry->intid		= intid;
+	),
+
+	TP_printk("DB: %016llx, device_id %u, event_id %u, intid %u",
+                  __entry->db_addr, __entry->device_id, __entry->event_id,
+                  __entry->intid)
+);
+
 #endif /* _TRACE_VGIC_H */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index e2764d0ffa9f..59179268ac2d 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -20,6 +20,7 @@ 
 #include <asm/kvm_arm.h>
 #include <asm/kvm_mmu.h>
 
+#include "trace.h"
 #include "vgic.h"
 #include "vgic-mmio.h"
 
@@ -636,8 +637,11 @@  static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
 	 * to the interrupt, so drop the potential reference on what
 	 * was in the cache, and increment it on the new interrupt.
 	 */
-	if (cte->irq)
+	if (cte->irq) {
+		KVM_VM_TRACE_EVENT(kvm, vgic_its_trans_cache_victim, cte->db,
+				   cte->devid, cte->eventid, cte->irq->intid);
 		__vgic_put_lpi_locked(kvm, cte->irq);
+	}
 
 	vgic_get_irq_kref(irq);
 
@@ -767,8 +771,14 @@  int vgic_its_inject_cached_translation(struct kvm *kvm, struct kvm_msi *msi)
 
 	db = (u64)msi->address_hi << 32 | msi->address_lo;
 	irq = vgic_its_check_cache(kvm, db, msi->devid, msi->data);
-	if (!irq)
+	if (!irq) {
+		KVM_VM_TRACE_EVENT(kvm, vgic_its_trans_cache_miss, db, msi->devid,
+				   msi->data);
 		return -EWOULDBLOCK;
+	}
+
+	KVM_VM_TRACE_EVENT(kvm, vgic_its_trans_cache_hit, db, msi->devid,
+			   msi->data, irq->intid);
 
 	raw_spin_lock_irqsave(&irq->irq_lock, flags);
 	irq->pending_latch = true;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 7e7fd25b09b3..846b447b6798 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1927,6 +1927,10 @@  struct _kvm_stats_desc {
 			HALT_POLL_HIST_COUNT),				       \
 	STATS_DESC_IBOOLEAN(VCPU_GENERIC, blocking)
 
+#define KVM_VM_TRACE_EVENT(vm, event, ...)					\
+	((vm)->stat.event)++; trace_## event(__VA_ARGS__)
+
+
 extern struct dentry *kvm_debugfs_dir;
 
 ssize_t kvm_stats_read(char *id, const struct kvm_stats_header *header,