[PATCH v2 0/2] KVM: s390: add counters for vsie performance

[PATCH v2 0/2] KVM: s390: add counters for vsie performance

[Nico Boehr]

v2:
---
* also count shadowing of pages (Janosch)
* fix naming of counters (Janosch)
* mention shadowing of multiple levels is counted in each level (Claudio)
* fix inaccuate commit description regarding gmap notifier (Claudio)

When running a guest-3 via VSIE, guest-1 needs to shadow the page table
structures of guest-2.

To reflect changes of the guest-2 in the _shadowed_ page table structures,
the _shadowing_ sturctures sometimes need to be rebuilt. Since this is a
costly operation, it should be avoided whenever possible.

This series adds kvm stat counters to count the number of shadow gmaps
created and a tracepoint whenever something is unshadowed. This is a first
step to try and improve VSIE performance.

Please note that "KVM: s390: add tracepoint in gmap notifier" has some
checkpatch --strict findings. I did not fix these since the tracepoint
definition would then look completely different from all the other
tracepoints in arch/s390/kvm/trace-s390.h. If you want me to fix that,
please let me know.

While developing this, a question regarding the stat counters came up:
there's usually no locking involved when the stat counters are incremented.
On s390, GCC accidentally seems to do the right thing(TM) most of the time
by generating a agsi instruction (which should be atomic given proper
alignment). However, it's not guaranteed, so would we rather want to go
with an atomic for the stat counters to avoid losing events? Or do we just
accept the fact that we might loose events sometimes? Is there anything
that speaks against having an atomic in kvm_stat?

Nico Boehr (2):
  KVM: s390: add stat counter for shadow gmap events
  KVM: s390: add tracepoint in gmap notifier

 arch/s390/include/asm/kvm_host.h |  6 ++++++
 arch/s390/kvm/gaccess.c          |  7 +++++++
 arch/s390/kvm/kvm-s390.c         | 10 +++++++++-
 arch/s390/kvm/trace-s390.h       | 23 +++++++++++++++++++++++
 arch/s390/kvm/vsie.c             |  1 +
 5 files changed, 46 insertions(+), 1 deletion(-)

-- 
2.39.1

[PATCH v2 1/2] KVM: s390: add stat counter for shadow gmap events

[Nico Boehr]

The shadow gmap tracks memory of nested guests (guest-3). In certain
scenarios, the shadow gmap needs to be rebuilt, which is a costly operation
since it involves a SIE exit into guest-1 for every entry in the respective
shadow level.

Add kvm stat counters when new shadow structures are created at various
levels. Also add a counter gmap_shadow_acquire when a completely fresh
shadow gmap is created.

Note that when several levels are shadowed at once, counters on all
affected levels will be increased.

Also note that not all page table levels need to be present and a ASCE
can directly point to e.g. a segment table. In this case, a new segment
table will always be equivalent to a new shadow gmap and hence will be
counted as gmap_shadow_acquire and not as gmap_shadow_segment.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 6 ++++++
 arch/s390/kvm/gaccess.c          | 7 +++++++
 arch/s390/kvm/kvm-s390.c         | 8 +++++++-
 arch/s390/kvm/vsie.c             | 1 +
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 2bbc3d54959d..d35e03e82d3d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -777,6 +777,12 @@ struct kvm_vm_stat {
 	u64 inject_service_signal;
 	u64 inject_virtio;
 	u64 aen_forward;
+	u64 gmap_shadow_acquire;
+	u64 gmap_shadow_r1_te;
+	u64 gmap_shadow_r2_te;
+	u64 gmap_shadow_r3_te;
+	u64 gmap_shadow_sg_te;
+	u64 gmap_shadow_pg_te;
 };
 
 struct kvm_arch_memory_slot {
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 3eb85f254881..6f4292ad0e4a 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -1382,6 +1382,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
 				  unsigned long *pgt, int *dat_protection,
 				  int *fake)
 {
+	struct kvm *kvm;
 	struct gmap *parent;
 	union asce asce;
 	union vaddress vaddr;
@@ -1390,6 +1391,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
 
 	*fake = 0;
 	*dat_protection = 0;
+	kvm = sg->private;
 	parent = sg->parent;
 	vaddr.addr = saddr;
 	asce.val = sg->orig_asce;
@@ -1450,6 +1452,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
 		rc = gmap_shadow_r2t(sg, saddr, rfte.val, *fake);
 		if (rc)
 			return rc;
+		kvm->stat.gmap_shadow_r1_te++;
 	}
 		fallthrough;
 	case ASCE_TYPE_REGION2: {
@@ -1478,6 +1481,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
 		rc = gmap_shadow_r3t(sg, saddr, rste.val, *fake);
 		if (rc)
 			return rc;
+		kvm->stat.gmap_shadow_r2_te++;
 	}
 		fallthrough;
 	case ASCE_TYPE_REGION3: {
@@ -1515,6 +1519,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
 		rc = gmap_shadow_sgt(sg, saddr, rtte.val, *fake);
 		if (rc)
 			return rc;
+		kvm->stat.gmap_shadow_r3_te++;
 	}
 		fallthrough;
 	case ASCE_TYPE_SEGMENT: {
@@ -1548,6 +1553,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
 		rc = gmap_shadow_pgt(sg, saddr, ste.val, *fake);
 		if (rc)
 			return rc;
+		kvm->stat.gmap_shadow_sg_te++;
 	}
 	}
 	/* Return the parent address of the page table */
@@ -1618,6 +1624,7 @@ int kvm_s390_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg,
 	pte.p |= dat_protection;
 	if (!rc)
 		rc = gmap_shadow_page(sg, saddr, __pte(pte.val));
+	vcpu->kvm->stat.gmap_shadow_pg_te++;
 	ipte_unlock(vcpu->kvm);
 	mmap_read_unlock(sg->mm);
 	return rc;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 17b81659cdb2..ded4149e145b 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -66,7 +66,13 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
 	STATS_DESC_COUNTER(VM, inject_pfault_done),
 	STATS_DESC_COUNTER(VM, inject_service_signal),
 	STATS_DESC_COUNTER(VM, inject_virtio),
-	STATS_DESC_COUNTER(VM, aen_forward)
+	STATS_DESC_COUNTER(VM, aen_forward),
+	STATS_DESC_COUNTER(VM, gmap_shadow_acquire),
+	STATS_DESC_COUNTER(VM, gmap_shadow_r1_te),
+	STATS_DESC_COUNTER(VM, gmap_shadow_r2_te),
+	STATS_DESC_COUNTER(VM, gmap_shadow_r3_te),
+	STATS_DESC_COUNTER(VM, gmap_shadow_sg_te),
+	STATS_DESC_COUNTER(VM, gmap_shadow_pg_te),
 };
 
 const struct kvm_stats_header kvm_vm_stats_header = {
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 8d6b765abf29..beb3be037722 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -1221,6 +1221,7 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
 	if (IS_ERR(gmap))
 		return PTR_ERR(gmap);
 	gmap->private = vcpu->kvm;
+	vcpu->kvm->stat.gmap_shadow_acquire++;
 	WRITE_ONCE(vsie_page->gmap, gmap);
 	return 0;
 }
-- 
2.39.1

[PATCH v2 2/2] KVM: s390: add tracepoint in gmap notifier

[Nico Boehr]

The gmap notifier is called for changes in table entries with the
notifier bit set. To diagnose performance issues, it can be useful to
see what causes certain changes in the gmap.

Hence, add a tracepoint in the gmap notifier.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c   |  2 ++
 arch/s390/kvm/trace-s390.h | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ded4149e145b..e8476c023b07 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3982,6 +3982,8 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
 	unsigned long prefix;
 	unsigned long i;
 
+	trace_kvm_s390_gmap_notifier(start, end, gmap_is_shadow(gmap));
+
 	if (gmap_is_shadow(gmap))
 		return;
 	if (start >= 1UL << 31)
diff --git a/arch/s390/kvm/trace-s390.h b/arch/s390/kvm/trace-s390.h
index 6f0209d45164..5dabd0b64d6e 100644
--- a/arch/s390/kvm/trace-s390.h
+++ b/arch/s390/kvm/trace-s390.h
@@ -333,6 +333,29 @@ TRACE_EVENT(kvm_s390_airq_suppressed,
 		      __entry->id, __entry->isc)
 	);
 
+/*
+ * Trace point for gmap notifier calls.
+ */
+TRACE_EVENT(kvm_s390_gmap_notifier,
+		TP_PROTO(unsigned long start, unsigned long end, unsigned int shadow),
+		TP_ARGS(start, end, shadow),
+
+		TP_STRUCT__entry(
+			__field(unsigned long, start)
+			__field(unsigned long, end)
+			__field(unsigned int, shadow)
+			),
+
+		TP_fast_assign(
+			__entry->start = start;
+			__entry->end = end;
+			__entry->shadow = shadow;
+			),
+
+		TP_printk("gmap notified (start:0x%lx end:0x%lx shadow:%d)",
+			__entry->start, __entry->end, __entry->shadow)
+	);
+
 
 #endif /* _TRACE_KVMS390_H */
 
-- 
2.39.1

Re: [PATCH v2 0/2] KVM: s390: add counters for vsie performance

[Nico Boehr]

Polite ping.

Quoting Nico Boehr (2023-05-10 14:18:20)
> v2:
> ---
> * also count shadowing of pages (Janosch)
> * fix naming of counters (Janosch)
> * mention shadowing of multiple levels is counted in each level (Claudio)
> * fix inaccuate commit description regarding gmap notifier (Claudio)
> 
> When running a guest-3 via VSIE, guest-1 needs to shadow the page table
> structures of guest-2.
> 
> To reflect changes of the guest-2 in the _shadowed_ page table structures,
> the _shadowing_ sturctures sometimes need to be rebuilt. Since this is a
> costly operation, it should be avoided whenever possible.
> 
> This series adds kvm stat counters to count the number of shadow gmaps
> created and a tracepoint whenever something is unshadowed. This is a first
> step to try and improve VSIE performance.
> 
> Please note that "KVM: s390: add tracepoint in gmap notifier" has some
> checkpatch --strict findings. I did not fix these since the tracepoint
> definition would then look completely different from all the other
> tracepoints in arch/s390/kvm/trace-s390.h. If you want me to fix that,
> please let me know.
> 
> While developing this, a question regarding the stat counters came up:
> there's usually no locking involved when the stat counters are incremented.
> On s390, GCC accidentally seems to do the right thing(TM) most of the time
> by generating a agsi instruction (which should be atomic given proper
> alignment). However, it's not guaranteed, so would we rather want to go
> with an atomic for the stat counters to avoid losing events? Or do we just
> accept the fact that we might loose events sometimes? Is there anything
> that speaks against having an atomic in kvm_stat?
> 
> Nico Boehr (2):
>   KVM: s390: add stat counter for shadow gmap events
>   KVM: s390: add tracepoint in gmap notifier
> 
>  arch/s390/include/asm/kvm_host.h |  6 ++++++
>  arch/s390/kvm/gaccess.c          |  7 +++++++
>  arch/s390/kvm/kvm-s390.c         | 10 +++++++++-
>  arch/s390/kvm/trace-s390.h       | 23 +++++++++++++++++++++++
>  arch/s390/kvm/vsie.c             |  1 +
>  5 files changed, 46 insertions(+), 1 deletion(-)
> 
> -- 
> 2.39.1
> 

Re: [PATCH v2 1/2] KVM: s390: add stat counter for shadow gmap events

[David Hildenbrand]

On 10.05.23 14:18, Nico Boehr wrote:
> The shadow gmap tracks memory of nested guests (guest-3). In certain
> scenarios, the shadow gmap needs to be rebuilt, which is a costly operation
> since it involves a SIE exit into guest-1 for every entry in the respective
> shadow level.
> 
> Add kvm stat counters when new shadow structures are created at various
> levels. Also add a counter gmap_shadow_acquire when a completely fresh
> shadow gmap is created.
> 
> Note that when several levels are shadowed at once, counters on all
> affected levels will be increased.
> 
> Also note that not all page table levels need to be present and a ASCE
> can directly point to e.g. a segment table. In this case, a new segment
> table will always be equivalent to a new shadow gmap and hence will be
> counted as gmap_shadow_acquire and not as gmap_shadow_segment.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h | 6 ++++++
>   arch/s390/kvm/gaccess.c          | 7 +++++++
>   arch/s390/kvm/kvm-s390.c         | 8 +++++++-
>   arch/s390/kvm/vsie.c             | 1 +
>   4 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 2bbc3d54959d..d35e03e82d3d 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -777,6 +777,12 @@ struct kvm_vm_stat {
>   	u64 inject_service_signal;
>   	u64 inject_virtio;
>   	u64 aen_forward;
> +	u64 gmap_shadow_acquire;
> +	u64 gmap_shadow_r1_te;
> +	u64 gmap_shadow_r2_te;
> +	u64 gmap_shadow_r3_te;
> +	u64 gmap_shadow_sg_te;
> +	u64 gmap_shadow_pg_te;

Is "te" supposed to stand for "table entry" ?

If so, I'd suggest to just call this gmap_shadow_pg_entry etc.

>   };
>   
>   struct kvm_arch_memory_slot {
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 3eb85f254881..6f4292ad0e4a 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1382,6 +1382,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
>   				  unsigned long *pgt, int *dat_protection,
>   				  int *fake)
>   {
> +	struct kvm *kvm;
>   	struct gmap *parent;
>   	union asce asce;
>   	union vaddress vaddr;
> @@ -1390,6 +1391,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
>   
>   	*fake = 0;
>   	*dat_protection = 0;
> +	kvm = sg->private;
>   	parent = sg->parent;
>   	vaddr.addr = saddr;
>   	asce.val = sg->orig_asce;
> @@ -1450,6 +1452,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
>   		rc = gmap_shadow_r2t(sg, saddr, rfte.val, *fake);
>   		if (rc)
>   			return rc;
> +		kvm->stat.gmap_shadow_r1_te++;
>   	}
>   		fallthrough;
>   	case ASCE_TYPE_REGION2: {
> @@ -1478,6 +1481,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
>   		rc = gmap_shadow_r3t(sg, saddr, rste.val, *fake);
>   		if (rc)
>   			return rc;
> +		kvm->stat.gmap_shadow_r2_te++;
>   	}
>   		fallthrough;
>   	case ASCE_TYPE_REGION3: {
> @@ -1515,6 +1519,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
>   		rc = gmap_shadow_sgt(sg, saddr, rtte.val, *fake);
>   		if (rc)
>   			return rc;
> +		kvm->stat.gmap_shadow_r3_te++;
>   	}
>   		fallthrough;
>   	case ASCE_TYPE_SEGMENT: {
> @@ -1548,6 +1553,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
>   		rc = gmap_shadow_pgt(sg, saddr, ste.val, *fake);
>   		if (rc)
>   			return rc;
> +		kvm->stat.gmap_shadow_sg_te++;
>   	}
>   	}
>   	/* Return the parent address of the page table */
> @@ -1618,6 +1624,7 @@ int kvm_s390_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg,
>   	pte.p |= dat_protection;
>   	if (!rc)
>   		rc = gmap_shadow_page(sg, saddr, __pte(pte.val));
> +	vcpu->kvm->stat.gmap_shadow_pg_te++;
>   	ipte_unlock(vcpu->kvm);
>   	mmap_read_unlock(sg->mm);
>   	return rc;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 17b81659cdb2..ded4149e145b 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -66,7 +66,13 @@ const struct _kvm_stats_desc kvm_vm_stats_desc[] = {
>   	STATS_DESC_COUNTER(VM, inject_pfault_done),
>   	STATS_DESC_COUNTER(VM, inject_service_signal),
>   	STATS_DESC_COUNTER(VM, inject_virtio),
> -	STATS_DESC_COUNTER(VM, aen_forward)
> +	STATS_DESC_COUNTER(VM, aen_forward),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_acquire),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_r1_te),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_r2_te),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_r3_te),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_sg_te),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_pg_te),
>   };
>   
>   const struct kvm_stats_header kvm_vm_stats_header = {
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 8d6b765abf29..beb3be037722 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -1221,6 +1221,7 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
>   	if (IS_ERR(gmap))
>   		return PTR_ERR(gmap);
>   	gmap->private = vcpu->kvm;
> +	vcpu->kvm->stat.gmap_shadow_acquire++;


Do you rather want to have events for

gmap_shadow_reuse (if gmap_shadow_valid() succeeded in that function)
gmap_shadow_create (if we have to create a new one via gmap_shadow)

?

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 2/2] KVM: s390: add tracepoint in gmap notifier

[David Hildenbrand]

On 10.05.23 14:18, Nico Boehr wrote:
> The gmap notifier is called for changes in table entries with the
> notifier bit set. To diagnose performance issues, it can be useful to
> see what causes certain changes in the gmap.
> 
> Hence, add a tracepoint in the gmap notifier.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c   |  2 ++
>   arch/s390/kvm/trace-s390.h | 23 +++++++++++++++++++++++
>   2 files changed, 25 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ded4149e145b..e8476c023b07 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3982,6 +3982,8 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
>   	unsigned long prefix;
>   	unsigned long i;
>   
> +	trace_kvm_s390_gmap_notifier(start, end, gmap_is_shadow(gmap));
> +
>   	if (gmap_is_shadow(gmap))
>   		return;
>   	if (start >= 1UL << 31)
> diff --git a/arch/s390/kvm/trace-s390.h b/arch/s390/kvm/trace-s390.h
> index 6f0209d45164..5dabd0b64d6e 100644
> --- a/arch/s390/kvm/trace-s390.h
> +++ b/arch/s390/kvm/trace-s390.h
> @@ -333,6 +333,29 @@ TRACE_EVENT(kvm_s390_airq_suppressed,
>   		      __entry->id, __entry->isc)
>   	);
>   
> +/*
> + * Trace point for gmap notifier calls.
> + */
> +TRACE_EVENT(kvm_s390_gmap_notifier,
> +		TP_PROTO(unsigned long start, unsigned long end, unsigned int shadow),
> +		TP_ARGS(start, end, shadow),
> +
> +		TP_STRUCT__entry(
> +			__field(unsigned long, start)
> +			__field(unsigned long, end)
> +			__field(unsigned int, shadow)
> +			),
> +
> +		TP_fast_assign(
> +			__entry->start = start;
> +			__entry->end = end;
> +			__entry->shadow = shadow;
> +			),
> +
> +		TP_printk("gmap notified (start:0x%lx end:0x%lx shadow:%d)",
> +			__entry->start, __entry->end, __entry->shadow)
> +	);
> +
>   
>   #endif /* _TRACE_KVMS390_H */
>   

In the context of vsie, I'd have thought you'd be tracing 
kvm_s390_vsie_gmap_notifier() instead.

This should work as well, as all notifier are called.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/2] KVM: s390: add stat counter for shadow gmap events

[Nico Boehr]

Quoting David Hildenbrand (2023-07-27 09:37:21)
[...]
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index 2bbc3d54959d..d35e03e82d3d 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -777,6 +777,12 @@ struct kvm_vm_stat {
> >       u64 inject_service_signal;
> >       u64 inject_virtio;
> >       u64 aen_forward;
> > +     u64 gmap_shadow_acquire;
> > +     u64 gmap_shadow_r1_te;
> > +     u64 gmap_shadow_r2_te;
> > +     u64 gmap_shadow_r3_te;
> > +     u64 gmap_shadow_sg_te;
> > +     u64 gmap_shadow_pg_te;
> 
> Is "te" supposed to stand for "table entry" ?

Yes.

> If so, I'd suggest to just call this gmap_shadow_pg_entry etc.

Janosch, since you suggested the current naming, are you OK with _entry?

[...]
> > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> > index 8d6b765abf29..beb3be037722 100644
> > --- a/arch/s390/kvm/vsie.c
> > +++ b/arch/s390/kvm/vsie.c
> > @@ -1221,6 +1221,7 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
> >       if (IS_ERR(gmap))
> >               return PTR_ERR(gmap);
> >       gmap->private = vcpu->kvm;
> > +     vcpu->kvm->stat.gmap_shadow_acquire++;
> 
> 
> Do you rather want to have events for
> 
> gmap_shadow_reuse (if gmap_shadow_valid() succeeded in that function)
> gmap_shadow_create (if we have to create a new one via gmap_shadow)
> 
> ?

Yeah, good suggestion. I'll add that.

Re: [PATCH v2 1/2] KVM: s390: add stat counter for shadow gmap events

[Janosch Frank]

On 8/1/23 13:48, Nico Boehr wrote:
> Quoting David Hildenbrand (2023-07-27 09:37:21)
> [...]
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index 2bbc3d54959d..d35e03e82d3d 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -777,6 +777,12 @@ struct kvm_vm_stat {
>>>        u64 inject_service_signal;
>>>        u64 inject_virtio;
>>>        u64 aen_forward;
>>> +     u64 gmap_shadow_acquire;
>>> +     u64 gmap_shadow_r1_te;
>>> +     u64 gmap_shadow_r2_te;
>>> +     u64 gmap_shadow_r3_te;
>>> +     u64 gmap_shadow_sg_te;
>>> +     u64 gmap_shadow_pg_te;
>>
>> Is "te" supposed to stand for "table entry" ?
> 
> Yes.
> 
>> If so, I'd suggest to just call this gmap_shadow_pg_entry etc.
> 
> Janosch, since you suggested the current naming, are you OK with _entry?

Sure

> 
> [...]
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index 8d6b765abf29..beb3be037722 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -1221,6 +1221,7 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
>>>        if (IS_ERR(gmap))
>>>                return PTR_ERR(gmap);
>>>        gmap->private = vcpu->kvm;
>>> +     vcpu->kvm->stat.gmap_shadow_acquire++;
>>
>>
>> Do you rather want to have events for
>>
>> gmap_shadow_reuse (if gmap_shadow_valid() succeeded in that function)
>> gmap_shadow_create (if we have to create a new one via gmap_shadow)
>>
>> ?
> 
> Yeah, good suggestion. I'll add that.

Re: [PATCH v2 2/2] KVM: s390: add tracepoint in gmap notifier

[Nico Boehr]

Quoting David Hildenbrand (2023-07-27 09:41:51)
> On 10.05.23 14:18, Nico Boehr wrote:
> > The gmap notifier is called for changes in table entries with the
> > notifier bit set. To diagnose performance issues, it can be useful to
> > see what causes certain changes in the gmap.
> > 
> > Hence, add a tracepoint in the gmap notifier.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > ---
> >   arch/s390/kvm/kvm-s390.c   |  2 ++
> >   arch/s390/kvm/trace-s390.h | 23 +++++++++++++++++++++++
> >   2 files changed, 25 insertions(+)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index ded4149e145b..e8476c023b07 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -3982,6 +3982,8 @@ static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
> >       unsigned long prefix;
> >       unsigned long i;
> >   
> > +     trace_kvm_s390_gmap_notifier(start, end, gmap_is_shadow(gmap));
> > +
> >       if (gmap_is_shadow(gmap))
> >               return;
> >       if (start >= 1UL << 31)
> > diff --git a/arch/s390/kvm/trace-s390.h b/arch/s390/kvm/trace-s390.h
> > index 6f0209d45164..5dabd0b64d6e 100644
> > --- a/arch/s390/kvm/trace-s390.h
> > +++ b/arch/s390/kvm/trace-s390.h
> > @@ -333,6 +333,29 @@ TRACE_EVENT(kvm_s390_airq_suppressed,
> >                     __entry->id, __entry->isc)
> >       );
> >   
> > +/*
> > + * Trace point for gmap notifier calls.
> > + */
> > +TRACE_EVENT(kvm_s390_gmap_notifier,
> > +             TP_PROTO(unsigned long start, unsigned long end, unsigned int shadow),
> > +             TP_ARGS(start, end, shadow),
> > +
> > +             TP_STRUCT__entry(
> > +                     __field(unsigned long, start)
> > +                     __field(unsigned long, end)
> > +                     __field(unsigned int, shadow)
> > +                     ),
> > +
> > +             TP_fast_assign(
> > +                     __entry->start = start;
> > +                     __entry->end = end;
> > +                     __entry->shadow = shadow;
> > +                     ),
> > +
> > +             TP_printk("gmap notified (start:0x%lx end:0x%lx shadow:%d)",
> > +                     __entry->start, __entry->end, __entry->shadow)
> > +     );
> > +
> >   
> >   #endif /* _TRACE_KVMS390_H */
> >   
> 
> In the context of vsie, I'd have thought you'd be tracing 
> kvm_s390_vsie_gmap_notifier() instead.

Right, I can change that if you / others have a preference for that.

Re: [PATCH v2 2/2] KVM: s390: add tracepoint in gmap notifier

[David Hildenbrand]

>>>    
>>>    #endif /* _TRACE_KVMS390_H */
>>>    
>>
>> In the context of vsie, I'd have thought you'd be tracing
>> kvm_s390_vsie_gmap_notifier() instead.
> 
> Right, I can change that if you / others have a preference for that.
> 

No strong opinion, just a thought.


-- 
Cheers,

David / dhildenb