[PATCH v1 0/3] KVM: s390: add counters for vsie performance

[PATCH v1 0/3] KVM: s390: add counters for vsie performance

[Nico Boehr]

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 (3):
  KVM: s390: fix space before open parenthesis
  KVM: s390: add stat counter for shadow gmap events
  KVM: s390: add tracepoint in gmap notifier

 arch/s390/include/asm/kvm_host.h |  7 ++++++-
 arch/s390/kvm/gaccess.c          |  6 ++++++
 arch/s390/kvm/kvm-s390.c         |  9 ++++++++-
 arch/s390/kvm/trace-s390.h       | 23 +++++++++++++++++++++++
 arch/s390/kvm/vsie.c             |  1 +
 5 files changed, 44 insertions(+), 2 deletions(-)

-- 
2.39.1

[PATCH v1 1/3] KVM: s390: fix space before open parenthesis

[Nico Boehr]

There should be a space before the open parenthesis, fix that while at
it.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 2bbc3d54959d..3c3fe45085ec 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -947,7 +947,7 @@ struct kvm_s390_pv {
 	struct mmu_notifier mmu_notifier;
 };
 
-struct kvm_arch{
+struct kvm_arch {
 	void *sca;
 	int use_esca;
 	rwlock_t sca_lock;
-- 
2.39.1

[PATCH v1 2/3] 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 there is no counter for the region first level. This is because
the region first level is the highest level and hence is never referenced
by another table. Creating a new region first table is therefore always
equivalent to a new shadow gmap and hence is counted as
gmap_shadow_acquire.

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 | 5 +++++
 arch/s390/kvm/gaccess.c          | 6 ++++++
 arch/s390/kvm/kvm-s390.c         | 7 ++++++-
 arch/s390/kvm/vsie.c             | 1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 3c3fe45085ec..7f70e3bbb44c 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -777,6 +777,11 @@ struct kvm_vm_stat {
 	u64 inject_service_signal;
 	u64 inject_virtio;
 	u64 aen_forward;
+	u64 gmap_shadow_acquire;
+	u64 gmap_shadow_r2;
+	u64 gmap_shadow_r3;
+	u64 gmap_shadow_segment;
+	u64 gmap_shadow_page;
 };
 
 struct kvm_arch_memory_slot {
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 3eb85f254881..8348a0095f3a 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_r2++;
 	}
 		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_r3++;
 	}
 		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_segment++;
 	}
 		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_page++;
 	}
 	}
 	/* Return the parent address of the page table */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 17b81659cdb2..b012645a5a7c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -66,7 +66,12 @@ 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_r2),
+	STATS_DESC_COUNTER(VM, gmap_shadow_r3),
+	STATS_DESC_COUNTER(VM, gmap_shadow_segment),
+	STATS_DESC_COUNTER(VM, gmap_shadow_page),
 };
 
 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 v1 3/3] KVM: s390: add tracepoint in gmap notifier

[Nico Boehr]

The gmap notifier is called whenever something in the gmap structures
changes. 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 b012645a5a7c..f66953bdabe4 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3981,6 +3981,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 v1 1/3] KVM: s390: fix space before open parenthesis

[Claudio Imbrenda]

On Tue,  9 May 2023 13:12:00 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> There should be a space before the open parenthesis, fix that while at
> it.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  arch/s390/include/asm/kvm_host.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 2bbc3d54959d..3c3fe45085ec 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -947,7 +947,7 @@ struct kvm_s390_pv {
>  	struct mmu_notifier mmu_notifier;
>  };
>  
> -struct kvm_arch{
> +struct kvm_arch {
>  	void *sca;
>  	int use_esca;
>  	rwlock_t sca_lock;

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

[Claudio Imbrenda]

On Tue,  9 May 2023 13:12:01 +0200
Nico Boehr <nrb@linux.ibm.com> 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 there is no counter for the region first level. This is because
> the region first level is the highest level and hence is never referenced
> by another table. Creating a new region first table is therefore always
> equivalent to a new shadow gmap and hence is counted as
> gmap_shadow_acquire.
> 
> 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>

looks good, see a nit below

> ---
>  arch/s390/include/asm/kvm_host.h | 5 +++++
>  arch/s390/kvm/gaccess.c          | 6 ++++++
>  arch/s390/kvm/kvm-s390.c         | 7 ++++++-
>  arch/s390/kvm/vsie.c             | 1 +
>  4 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 3c3fe45085ec..7f70e3bbb44c 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -777,6 +777,11 @@ struct kvm_vm_stat {
>  	u64 inject_service_signal;
>  	u64 inject_virtio;
>  	u64 aen_forward;
> +	u64 gmap_shadow_acquire;
> +	u64 gmap_shadow_r2;
> +	u64 gmap_shadow_r3;
> +	u64 gmap_shadow_segment;
> +	u64 gmap_shadow_page;
>  };
>  
>  struct kvm_arch_memory_slot {
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 3eb85f254881..8348a0095f3a 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_r2++;
>  	}
>  		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_r3++;
>  	}
>  		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_segment++;
>  	}
>  		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_page++;

do I understand correctly that, if several levels need to be shadowed
at the same time, you will increment every affected counter, and not
just the highest or lowest level?

if so, please add a brief explanation to the patch description

>  	}
>  	}
>  	/* Return the parent address of the page table */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 17b81659cdb2..b012645a5a7c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -66,7 +66,12 @@ 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_r2),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_r3),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_segment),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_page),
>  };
>  
>  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;
>  }

Re: [PATCH v1 3/3] KVM: s390: add tracepoint in gmap notifier

[Claudio Imbrenda]

On Tue,  9 May 2023 13:12:02 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> The gmap notifier is called whenever something in the gmap structures

this is a little bit too oversimplified; the gmap notifier is only
called for ptes (or pmds for hugetlbfs) that have the notifier bit set
(used for prefix or vsie)

> changes. 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>

apart for the above nit, looks quite straightforward

> ---
>  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 b012645a5a7c..f66953bdabe4 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3981,6 +3981,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 */
>  

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

[Janosch Frank]

On 5/9/23 13:12, 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 there is no counter for the region first level. This is because
> the region first level is the highest level and hence is never referenced
> by another table. Creating a new region first table is therefore always
> equivalent to a new shadow gmap and hence is counted as
> gmap_shadow_acquire.
> 
> 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 | 5 +++++
>   arch/s390/kvm/gaccess.c          | 6 ++++++
>   arch/s390/kvm/kvm-s390.c         | 7 ++++++-
>   arch/s390/kvm/vsie.c             | 1 +
>   4 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 3c3fe45085ec..7f70e3bbb44c 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -777,6 +777,11 @@ struct kvm_vm_stat {
>   	u64 inject_service_signal;
>   	u64 inject_virtio;
>   	u64 aen_forward;
> +	u64 gmap_shadow_acquire;
> +	u64 gmap_shadow_r2;
> +	u64 gmap_shadow_r3;
> +	u64 gmap_shadow_segment;
> +	u64 gmap_shadow_page;

This needs to be gmap_shadow_pgt and then we need a separate shadow page 
counter that's beeing incremented in kvm_s390_shadow_fault().


I'm wondering if we should name them after the entries to reduce 
confusion especially when we get huge pages in the future.

gmap_shadow_acquire
gmap_shadow_r1_te (ptr to r2 table)
gmap_shadow_r2_te (ptr to r3 table)
gmap_shadow_r3_te (ptr to segment table)
gmap_shadow_sg_te (ptr to page table)
gmap_shadow_pg_te (single page table entry)

>   };
>   
>   struct kvm_arch_memory_slot {
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 3eb85f254881..8348a0095f3a 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_r2++;
>   	}
>   		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_r3++;
>   	}
>   		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_segment++;
>   	}
>   		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_page++;
>   	}
>   	}
>   	/* Return the parent address of the page table */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 17b81659cdb2..b012645a5a7c 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -66,7 +66,12 @@ 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_r2),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_r3),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_segment),
> +	STATS_DESC_COUNTER(VM, gmap_shadow_page),
>   };
>   
>   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;
>   }

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

[Nico Boehr]

Quoting Claudio Imbrenda (2023-05-09 13:43:51)
[...]
> > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> > index 3eb85f254881..8348a0095f3a 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_r2++;
> >       }
> >               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_r3++;
> >       }
> >               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_segment++;
> >       }
> >               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_page++;
> 
> do I understand correctly that, if several levels need to be shadowed
> at the same time, you will increment every affected counter, and not
> just the highest or lowest level?
> 
> if so, please add a brief explanation to the patch description

Yes, that seemed like the simplest thing to do.

Will add a explanation.

Or should I add a flag and only increment the top level?

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

[Nico Boehr]

Quoting Janosch Frank (2023-05-09 13:59:46)
[...]
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index 3c3fe45085ec..7f70e3bbb44c 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -777,6 +777,11 @@ struct kvm_vm_stat {
> >       u64 inject_service_signal;
> >       u64 inject_virtio;
> >       u64 aen_forward;
> > +     u64 gmap_shadow_acquire;
> > +     u64 gmap_shadow_r2;
> > +     u64 gmap_shadow_r3;
> > +     u64 gmap_shadow_segment;
> > +     u64 gmap_shadow_page;
> 
> This needs to be gmap_shadow_pgt and then we need a separate shadow page 
> counter that's beeing incremented in kvm_s390_shadow_fault().
> 
> 
> I'm wondering if we should name them after the entries to reduce 
> confusion especially when we get huge pages in the future.
> 
> gmap_shadow_acquire
> gmap_shadow_r1_te (ptr to r2 table)
> gmap_shadow_r2_te (ptr to r3 table)
> gmap_shadow_r3_te (ptr to segment table)
> gmap_shadow_sg_te (ptr to page table)
> gmap_shadow_pg_te (single page table entry)

Yep, right, this was highly confusing to the point where I was also
confused by it. Will change that, thanks.

Re: [PATCH v1 3/3] KVM: s390: add tracepoint in gmap notifier

[Nico Boehr]

Quoting Claudio Imbrenda (2023-05-09 13:48:39)
> On Tue,  9 May 2023 13:12:02 +0200
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
> > The gmap notifier is called whenever something in the gmap structures
> 
> this is a little bit too oversimplified; the gmap notifier is only
> called for ptes (or pmds for hugetlbfs) that have the notifier bit set
> (used for prefix or vsie)

Yep true, I will adjust, thanks.

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

[Claudio Imbrenda]

On Tue, 09 May 2023 16:54:21 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Janosch Frank (2023-05-09 13:59:46)
> [...]
> > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > > index 3c3fe45085ec..7f70e3bbb44c 100644
> > > --- a/arch/s390/include/asm/kvm_host.h
> > > +++ b/arch/s390/include/asm/kvm_host.h
> > > @@ -777,6 +777,11 @@ struct kvm_vm_stat {
> > >       u64 inject_service_signal;
> > >       u64 inject_virtio;
> > >       u64 aen_forward;
> > > +     u64 gmap_shadow_acquire;
> > > +     u64 gmap_shadow_r2;
> > > +     u64 gmap_shadow_r3;
> > > +     u64 gmap_shadow_segment;
> > > +     u64 gmap_shadow_page;  
> > 
> > This needs to be gmap_shadow_pgt and then we need a separate shadow page 
> > counter that's beeing incremented in kvm_s390_shadow_fault().
> > 
> > 
> > I'm wondering if we should name them after the entries to reduce 
> > confusion especially when we get huge pages in the future.
> > 
> > gmap_shadow_acquire
> > gmap_shadow_r1_te (ptr to r2 table)
> > gmap_shadow_r2_te (ptr to r3 table)
> > gmap_shadow_r3_te (ptr to segment table)
> > gmap_shadow_sg_te (ptr to page table)
> > gmap_shadow_pg_te (single page table entry)  

but then why not calling them gmap_shadow_{pte,pmd,pud,p4d,pgd} ?

> 
> Yep, right, this was highly confusing to the point where I was also
> confused by it. Will change that, thanks.

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

[Claudio Imbrenda]

On Tue, 09 May 2023 16:53:13 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2023-05-09 13:43:51)
> [...]
> > > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> > > index 3eb85f254881..8348a0095f3a 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_r2++;
> > >       }
> > >               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_r3++;
> > >       }
> > >               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_segment++;
> > >       }
> > >               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_page++;  
> > 
> > do I understand correctly that, if several levels need to be shadowed
> > at the same time, you will increment every affected counter, and not
> > just the highest or lowest level?
> > 
> > if so, please add a brief explanation to the patch description  
> 
> Yes, that seemed like the simplest thing to do.
> 
> Will add a explanation.
> 
> Or should I add a flag and only increment the top level?

that's up to you, see what makes more sense

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

[Janosch Frank]

On 5/9/23 17:14, Claudio Imbrenda wrote:
> On Tue, 09 May 2023 16:54:21 +0200
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
>> Quoting Janosch Frank (2023-05-09 13:59:46)
>> [...]
>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>> index 3c3fe45085ec..7f70e3bbb44c 100644
>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>> @@ -777,6 +777,11 @@ struct kvm_vm_stat {
>>>>        u64 inject_service_signal;
>>>>        u64 inject_virtio;
>>>>        u64 aen_forward;
>>>> +     u64 gmap_shadow_acquire;
>>>> +     u64 gmap_shadow_r2;
>>>> +     u64 gmap_shadow_r3;
>>>> +     u64 gmap_shadow_segment;
>>>> +     u64 gmap_shadow_page;
>>>
>>> This needs to be gmap_shadow_pgt and then we need a separate shadow page
>>> counter that's beeing incremented in kvm_s390_shadow_fault().
>>>
>>>
>>> I'm wondering if we should name them after the entries to reduce
>>> confusion especially when we get huge pages in the future.
>>>
>>> gmap_shadow_acquire
>>> gmap_shadow_r1_te (ptr to r2 table)
>>> gmap_shadow_r2_te (ptr to r3 table)
>>> gmap_shadow_r3_te (ptr to segment table)
>>> gmap_shadow_sg_te (ptr to page table)
>>> gmap_shadow_pg_te (single page table entry)
> 
> but then why not calling them gmap_shadow_{pte,pmd,pud,p4d,pgd} ?
> 

Because I'll need to look up the order of the names after the pmd :)
The gmap mostly works with s390 names.

I'm not totally opposed to that but I also don't see a clear benefit.