KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/3] KVM: X86: Some tracepoint enhancements
@ 2019-08-15 10:34 Peter Xu
  2019-08-15 10:34 ` [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit Peter Xu
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Peter Xu @ 2019-08-15 10:34 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan, peterx

v2:
- fix commit messages, change format of ple window tracepoint [Sean]
- rebase [Wanpeng]

Each small patch explains itself.  I noticed them when I'm tracing
some IRQ paths and I found them helpful at least to me.

Please have a look.  Thanks,

Peter Xu (3):
  KVM: X86: Trace vcpu_id for vmexit
  KVM: X86: Remove tailing newline for tracepoints
  KVM: X86: Tune PLE Window tracepoint

 arch/x86/kvm/svm.c     | 16 ++++++++--------
 arch/x86/kvm/trace.h   | 30 ++++++++++++------------------
 arch/x86/kvm/vmx/vmx.c | 14 ++++++++------
 arch/x86/kvm/x86.c     |  2 +-
 4 files changed, 29 insertions(+), 33 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit
  2019-08-15 10:34 [PATCH v2 0/3] KVM: X86: Some tracepoint enhancements Peter Xu
@ 2019-08-15 10:34 ` Peter Xu
  2019-09-04 17:26   ` Sean Christopherson
  2019-08-15 10:34 ` [PATCH v2 2/3] KVM: X86: Remove tailing newline for tracepoints Peter Xu
  2019-08-15 10:34 ` [PATCH v2 3/3] KVM: X86: Tune PLE Window tracepoint Peter Xu
  2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2019-08-15 10:34 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan, peterx

Tracing the ID helps to pair vmenters and vmexits for guests with
multiple vCPUs.

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/trace.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index b5c831e79094..c682f3f7f998 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -232,17 +232,20 @@ TRACE_EVENT(kvm_exit,
 		__field(	u32,	        isa             )
 		__field(	u64,	        info1           )
 		__field(	u64,	        info2           )
+		__field(	int,	        vcpu_id         )
 	),
 
 	TP_fast_assign(
 		__entry->exit_reason	= exit_reason;
 		__entry->guest_rip	= kvm_rip_read(vcpu);
 		__entry->isa            = isa;
+		__entry->vcpu_id        = vcpu->vcpu_id;
 		kvm_x86_ops->get_exit_info(vcpu, &__entry->info1,
 					   &__entry->info2);
 	),
 
-	TP_printk("reason %s rip 0x%lx info %llx %llx",
+	TP_printk("vcpu %d reason %s rip 0x%lx info %llx %llx",
+		  __entry->vcpu_id,
 		 (__entry->isa == KVM_ISA_VMX) ?
 		 __print_symbolic(__entry->exit_reason, VMX_EXIT_REASONS) :
 		 __print_symbolic(__entry->exit_reason, SVM_EXIT_REASONS),
-- 
2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] KVM: X86: Remove tailing newline for tracepoints
  2019-08-15 10:34 [PATCH v2 0/3] KVM: X86: Some tracepoint enhancements Peter Xu
  2019-08-15 10:34 ` [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit Peter Xu
@ 2019-08-15 10:34 ` Peter Xu
  2019-09-04 17:27   ` Sean Christopherson
  2019-08-15 10:34 ` [PATCH v2 3/3] KVM: X86: Tune PLE Window tracepoint Peter Xu
  2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2019-08-15 10:34 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan, peterx

It's done by TP_printk() already.

Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/trace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index c682f3f7f998..76a39bc25b95 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -1323,7 +1323,7 @@ TRACE_EVENT(kvm_avic_incomplete_ipi,
 		__entry->index = index;
 	),
 
-	TP_printk("vcpu=%u, icrh:icrl=%#010x:%08x, id=%u, index=%u\n",
+	TP_printk("vcpu=%u, icrh:icrl=%#010x:%08x, id=%u, index=%u",
 		  __entry->vcpu, __entry->icrh, __entry->icrl,
 		  __entry->id, __entry->index)
 );
@@ -1348,7 +1348,7 @@ TRACE_EVENT(kvm_avic_unaccelerated_access,
 		__entry->vec = vec;
 	),
 
-	TP_printk("vcpu=%u, offset=%#x(%s), %s, %s, vec=%#x\n",
+	TP_printk("vcpu=%u, offset=%#x(%s), %s, %s, vec=%#x",
 		  __entry->vcpu,
 		  __entry->offset,
 		  __print_symbolic(__entry->offset, kvm_trace_symbol_apic),
-- 
2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] KVM: X86: Tune PLE Window tracepoint
  2019-08-15 10:34 [PATCH v2 0/3] KVM: X86: Some tracepoint enhancements Peter Xu
  2019-08-15 10:34 ` [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit Peter Xu
  2019-08-15 10:34 ` [PATCH v2 2/3] KVM: X86: Remove tailing newline for tracepoints Peter Xu
@ 2019-08-15 10:34 ` Peter Xu
  2019-09-04 17:32   ` Sean Christopherson
  2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2019-08-15 10:34 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan, peterx

The PLE window tracepoint triggers even if the window is not changed,
and the wording can be a bit confusing too.  One example line:

  kvm_ple_window: vcpu 0: ple_window 4096 (shrink 4096)

It easily let people think of "the window now is 4096 which is
shrinked", but the truth is the value actually didn't change (4096).

Let's only dump this message if the value really changed, and we make
the message even simpler like:

  kvm_ple_window: vcpu 4 old 4096 new 8192 (growed)

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/x86/kvm/svm.c     | 16 ++++++++--------
 arch/x86/kvm/trace.h   | 21 ++++++---------------
 arch/x86/kvm/vmx/vmx.c | 14 ++++++++------
 arch/x86/kvm/x86.c     |  2 +-
 4 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index d685491fce4d..d5cb6b5a9254 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1269,11 +1269,11 @@ static void grow_ple_window(struct kvm_vcpu *vcpu)
 							pause_filter_count_grow,
 							pause_filter_count_max);
 
-	if (control->pause_filter_count != old)
+	if (control->pause_filter_count != old) {
 		mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
-
-	trace_kvm_ple_window_grow(vcpu->vcpu_id,
-				  control->pause_filter_count, old);
+		trace_kvm_ple_window_update(vcpu->vcpu_id,
+					    control->pause_filter_count, old);
+	}
 }
 
 static void shrink_ple_window(struct kvm_vcpu *vcpu)
@@ -1287,11 +1287,11 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
 						    pause_filter_count,
 						    pause_filter_count_shrink,
 						    pause_filter_count);
-	if (control->pause_filter_count != old)
+	if (control->pause_filter_count != old) {
 		mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
-
-	trace_kvm_ple_window_shrink(vcpu->vcpu_id,
-				    control->pause_filter_count, old);
+		trace_kvm_ple_window_update(vcpu->vcpu_id,
+					    control->pause_filter_count, old);
+	}
 }
 
 static __init int svm_hardware_setup(void)
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 76a39bc25b95..97df9d7cae71 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -890,36 +890,27 @@ TRACE_EVENT(kvm_pml_full,
 	TP_printk("vcpu %d: PML full", __entry->vcpu_id)
 );
 
-TRACE_EVENT(kvm_ple_window,
-	TP_PROTO(bool grow, unsigned int vcpu_id, int new, int old),
-	TP_ARGS(grow, vcpu_id, new, old),
+TRACE_EVENT(kvm_ple_window_update,
+	TP_PROTO(unsigned int vcpu_id, int new, int old),
+	TP_ARGS(vcpu_id, new, old),
 
 	TP_STRUCT__entry(
-		__field(                bool,      grow         )
 		__field(        unsigned int,   vcpu_id         )
 		__field(                 int,       new         )
 		__field(                 int,       old         )
 	),
 
 	TP_fast_assign(
-		__entry->grow           = grow;
 		__entry->vcpu_id        = vcpu_id;
 		__entry->new            = new;
 		__entry->old            = old;
 	),
 
-	TP_printk("vcpu %u: ple_window %d (%s %d)",
-	          __entry->vcpu_id,
-	          __entry->new,
-	          __entry->grow ? "grow" : "shrink",
-	          __entry->old)
+	TP_printk("vcpu %u old %d new %d (%s)",
+	          __entry->vcpu_id, __entry->old, __entry->new,
+		  __entry->old < __entry->new ? "growed" : "shrinked")
 );
 
-#define trace_kvm_ple_window_grow(vcpu_id, new, old) \
-	trace_kvm_ple_window(true, vcpu_id, new, old)
-#define trace_kvm_ple_window_shrink(vcpu_id, new, old) \
-	trace_kvm_ple_window(false, vcpu_id, new, old)
-
 TRACE_EVENT(kvm_pvclock_update,
 	TP_PROTO(unsigned int vcpu_id, struct pvclock_vcpu_time_info *pvclock),
 	TP_ARGS(vcpu_id, pvclock),
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 42ed3faa6af8..469c4134a4a7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5233,10 +5233,11 @@ static void grow_ple_window(struct kvm_vcpu *vcpu)
 					    ple_window_grow,
 					    ple_window_max);
 
-	if (vmx->ple_window != old)
+	if (vmx->ple_window != old) {
 		vmx->ple_window_dirty = true;
-
-	trace_kvm_ple_window_grow(vcpu->vcpu_id, vmx->ple_window, old);
+		trace_kvm_ple_window_update(vcpu->vcpu_id,
+					    vmx->ple_window, old);
+	}
 }
 
 static void shrink_ple_window(struct kvm_vcpu *vcpu)
@@ -5248,10 +5249,11 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
 					      ple_window_shrink,
 					      ple_window);
 
-	if (vmx->ple_window != old)
+	if (vmx->ple_window != old) {
 		vmx->ple_window_dirty = true;
-
-	trace_kvm_ple_window_shrink(vcpu->vcpu_id, vmx->ple_window, old);
+		trace_kvm_ple_window_update(vcpu->vcpu_id,
+					    vmx->ple_window, old);
+	}
 }
 
 /*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 93b0bd45ac73..69ad184edc90 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10082,7 +10082,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_invlpga);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_skinit);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_intercepts);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_write_tsc_offset);
-EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_ple_window);
+EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_ple_window_update);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_pml_full);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_pi_irte_update);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_avic_unaccelerated_access);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit
  2019-08-15 10:34 ` [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit Peter Xu
@ 2019-09-04 17:26   ` Sean Christopherson
  2019-09-05  2:15     ` Peter Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2019-09-04 17:26 UTC (permalink / raw)
  To: Peter Xu; +Cc: kvm, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan

On Thu, Aug 15, 2019 at 06:34:56PM +0800, Peter Xu wrote:
> Tracing the ID helps to pair vmenters and vmexits for guests with
> multiple vCPUs.
> 
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/kvm/trace.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index b5c831e79094..c682f3f7f998 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -232,17 +232,20 @@ TRACE_EVENT(kvm_exit,
>  		__field(	u32,	        isa             )
>  		__field(	u64,	        info1           )
>  		__field(	u64,	        info2           )
> +		__field(	int,	        vcpu_id         )

It doesn't actually affect anything, but vcpu_id is stored and printed as
an 'unsigned int' everywhere else in the trace code.  Stylistically I like
that approach even though struct kvm_vcpu holds it as a signed int.

>  	),
>  
>  	TP_fast_assign(
>  		__entry->exit_reason	= exit_reason;
>  		__entry->guest_rip	= kvm_rip_read(vcpu);
>  		__entry->isa            = isa;
> +		__entry->vcpu_id        = vcpu->vcpu_id;
>  		kvm_x86_ops->get_exit_info(vcpu, &__entry->info1,
>  					   &__entry->info2);
>  	),
>  
> -	TP_printk("reason %s rip 0x%lx info %llx %llx",
> +	TP_printk("vcpu %d reason %s rip 0x%lx info %llx %llx",
> +		  __entry->vcpu_id,
>  		 (__entry->isa == KVM_ISA_VMX) ?
>  		 __print_symbolic(__entry->exit_reason, VMX_EXIT_REASONS) :
>  		 __print_symbolic(__entry->exit_reason, SVM_EXIT_REASONS),
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] KVM: X86: Remove tailing newline for tracepoints
  2019-08-15 10:34 ` [PATCH v2 2/3] KVM: X86: Remove tailing newline for tracepoints Peter Xu
@ 2019-09-04 17:27   ` Sean Christopherson
  0 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2019-09-04 17:27 UTC (permalink / raw)
  To: Peter Xu; +Cc: kvm, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan

On Thu, Aug 15, 2019 at 06:34:57PM +0800, Peter Xu wrote:
> It's done by TP_printk() already.
> 
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] KVM: X86: Tune PLE Window tracepoint
  2019-08-15 10:34 ` [PATCH v2 3/3] KVM: X86: Tune PLE Window tracepoint Peter Xu
@ 2019-09-04 17:32   ` Sean Christopherson
  2019-09-05  2:25     ` Peter Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2019-09-04 17:32 UTC (permalink / raw)
  To: Peter Xu; +Cc: kvm, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan

On Thu, Aug 15, 2019 at 06:34:58PM +0800, Peter Xu wrote:
> The PLE window tracepoint triggers even if the window is not changed,
> and the wording can be a bit confusing too.  One example line:
> 
>   kvm_ple_window: vcpu 0: ple_window 4096 (shrink 4096)
> 
> It easily let people think of "the window now is 4096 which is
> shrinked", but the truth is the value actually didn't change (4096).
> 
> Let's only dump this message if the value really changed, and we make
> the message even simpler like:
> 
>   kvm_ple_window: vcpu 4 old 4096 new 8192 (growed)
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/kvm/svm.c     | 16 ++++++++--------
>  arch/x86/kvm/trace.h   | 21 ++++++---------------
>  arch/x86/kvm/vmx/vmx.c | 14 ++++++++------
>  arch/x86/kvm/x86.c     |  2 +-
>  4 files changed, 23 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index d685491fce4d..d5cb6b5a9254 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1269,11 +1269,11 @@ static void grow_ple_window(struct kvm_vcpu *vcpu)
>  							pause_filter_count_grow,
>  							pause_filter_count_max);
>  
> -	if (control->pause_filter_count != old)
> +	if (control->pause_filter_count != old) {
>  		mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> -
> -	trace_kvm_ple_window_grow(vcpu->vcpu_id,
> -				  control->pause_filter_count, old);
> +		trace_kvm_ple_window_update(vcpu->vcpu_id,
> +					    control->pause_filter_count, old);
> +	}
>  }
>  
>  static void shrink_ple_window(struct kvm_vcpu *vcpu)
> @@ -1287,11 +1287,11 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
>  						    pause_filter_count,
>  						    pause_filter_count_shrink,
>  						    pause_filter_count);
> -	if (control->pause_filter_count != old)
> +	if (control->pause_filter_count != old) {
>  		mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> -
> -	trace_kvm_ple_window_shrink(vcpu->vcpu_id,
> -				    control->pause_filter_count, old);
> +		trace_kvm_ple_window_update(vcpu->vcpu_id,
> +					    control->pause_filter_count, old);
> +	}
>  }
>  
>  static __init int svm_hardware_setup(void)
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 76a39bc25b95..97df9d7cae71 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -890,36 +890,27 @@ TRACE_EVENT(kvm_pml_full,
>  	TP_printk("vcpu %d: PML full", __entry->vcpu_id)
>  );
>  
> -TRACE_EVENT(kvm_ple_window,
> -	TP_PROTO(bool grow, unsigned int vcpu_id, int new, int old),
> -	TP_ARGS(grow, vcpu_id, new, old),
> +TRACE_EVENT(kvm_ple_window_update,
> +	TP_PROTO(unsigned int vcpu_id, int new, int old),
> +	TP_ARGS(vcpu_id, new, old),
>  
>  	TP_STRUCT__entry(
> -		__field(                bool,      grow         )
>  		__field(        unsigned int,   vcpu_id         )
>  		__field(                 int,       new         )
>  		__field(                 int,       old         )

Not your code, but these should really be 'unsigned int', especially now
that they are directly compared when printing "growed" versus "shrinked".
For SVM it doesn't matter since the underlying hardware fields are only
16 bits, but on VMX they're 32 bits, e.g. theoretically userspace could
set ple_window and ple_window_max to a negative value.

The ple_window variable in struct vcpu_vmx and local snapshots of the
field should also be updated, but that can be done separately.

>  	),
>  
>  	TP_fast_assign(
> -		__entry->grow           = grow;
>  		__entry->vcpu_id        = vcpu_id;
>  		__entry->new            = new;
>  		__entry->old            = old;
>  	),
>  
> -	TP_printk("vcpu %u: ple_window %d (%s %d)",
> -	          __entry->vcpu_id,
> -	          __entry->new,
> -	          __entry->grow ? "grow" : "shrink",
> -	          __entry->old)
> +	TP_printk("vcpu %u old %d new %d (%s)",
> +	          __entry->vcpu_id, __entry->old, __entry->new,
> +		  __entry->old < __entry->new ? "growed" : "shrinked")
>  );
>  
> -#define trace_kvm_ple_window_grow(vcpu_id, new, old) \
> -	trace_kvm_ple_window(true, vcpu_id, new, old)
> -#define trace_kvm_ple_window_shrink(vcpu_id, new, old) \
> -	trace_kvm_ple_window(false, vcpu_id, new, old)
> -
>  TRACE_EVENT(kvm_pvclock_update,
>  	TP_PROTO(unsigned int vcpu_id, struct pvclock_vcpu_time_info *pvclock),
>  	TP_ARGS(vcpu_id, pvclock),
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 42ed3faa6af8..469c4134a4a7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5233,10 +5233,11 @@ static void grow_ple_window(struct kvm_vcpu *vcpu)
>  					    ple_window_grow,
>  					    ple_window_max);
>  
> -	if (vmx->ple_window != old)
> +	if (vmx->ple_window != old) {
>  		vmx->ple_window_dirty = true;
> -
> -	trace_kvm_ple_window_grow(vcpu->vcpu_id, vmx->ple_window, old);
> +		trace_kvm_ple_window_update(vcpu->vcpu_id,
> +					    vmx->ple_window, old);
> +	}
>  }
>  
>  static void shrink_ple_window(struct kvm_vcpu *vcpu)
> @@ -5248,10 +5249,11 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
>  					      ple_window_shrink,
>  					      ple_window);
>  
> -	if (vmx->ple_window != old)
> +	if (vmx->ple_window != old) {
>  		vmx->ple_window_dirty = true;
> -
> -	trace_kvm_ple_window_shrink(vcpu->vcpu_id, vmx->ple_window, old);
> +		trace_kvm_ple_window_update(vcpu->vcpu_id,
> +					    vmx->ple_window, old);
> +	}
>  }
>  
>  /*
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 93b0bd45ac73..69ad184edc90 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10082,7 +10082,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_invlpga);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_skinit);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_intercepts);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_write_tsc_offset);
> -EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_ple_window);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_ple_window_update);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_pml_full);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_pi_irte_update);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_avic_unaccelerated_access);
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit
  2019-09-04 17:26   ` Sean Christopherson
@ 2019-09-05  2:15     ` Peter Xu
  2019-09-05 15:56       ` Sean Christopherson
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2019-09-05  2:15 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan

On Wed, Sep 04, 2019 at 10:26:58AM -0700, Sean Christopherson wrote:
> On Thu, Aug 15, 2019 at 06:34:56PM +0800, Peter Xu wrote:
> > Tracing the ID helps to pair vmenters and vmexits for guests with
> > multiple vCPUs.
> > 
> > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  arch/x86/kvm/trace.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > index b5c831e79094..c682f3f7f998 100644
> > --- a/arch/x86/kvm/trace.h
> > +++ b/arch/x86/kvm/trace.h
> > @@ -232,17 +232,20 @@ TRACE_EVENT(kvm_exit,
> >  		__field(	u32,	        isa             )
> >  		__field(	u64,	        info1           )
> >  		__field(	u64,	        info2           )
> > +		__field(	int,	        vcpu_id         )
> 
> It doesn't actually affect anything, but vcpu_id is stored and printed as
> an 'unsigned int' everywhere else in the trace code.  Stylistically I like
> that approach even though struct kvm_vcpu holds it as a signed int.

True.  I can switch to unsigned int to get aligned with the rest if
there's other comment to address.  Though from codebase-wise I would
even prefer signed because it gives us a chance to set an invalid vcpu
id (-1) where necessary, or notice something severly wrong when <-1.
After all it should far cover our usage (IIUC max vcpu id should be
512 cross archs).

Thanks,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] KVM: X86: Tune PLE Window tracepoint
  2019-09-04 17:32   ` Sean Christopherson
@ 2019-09-05  2:25     ` Peter Xu
  0 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2019-09-05  2:25 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan

On Wed, Sep 04, 2019 at 10:32:54AM -0700, Sean Christopherson wrote:
> On Thu, Aug 15, 2019 at 06:34:58PM +0800, Peter Xu wrote:
> > The PLE window tracepoint triggers even if the window is not changed,
> > and the wording can be a bit confusing too.  One example line:
> > 
> >   kvm_ple_window: vcpu 0: ple_window 4096 (shrink 4096)
> > 
> > It easily let people think of "the window now is 4096 which is
> > shrinked", but the truth is the value actually didn't change (4096).
> > 
> > Let's only dump this message if the value really changed, and we make
> > the message even simpler like:
> > 
> >   kvm_ple_window: vcpu 4 old 4096 new 8192 (growed)
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  arch/x86/kvm/svm.c     | 16 ++++++++--------
> >  arch/x86/kvm/trace.h   | 21 ++++++---------------
> >  arch/x86/kvm/vmx/vmx.c | 14 ++++++++------
> >  arch/x86/kvm/x86.c     |  2 +-
> >  4 files changed, 23 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index d685491fce4d..d5cb6b5a9254 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -1269,11 +1269,11 @@ static void grow_ple_window(struct kvm_vcpu *vcpu)
> >  							pause_filter_count_grow,
> >  							pause_filter_count_max);
> >  
> > -	if (control->pause_filter_count != old)
> > +	if (control->pause_filter_count != old) {
> >  		mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> > -
> > -	trace_kvm_ple_window_grow(vcpu->vcpu_id,
> > -				  control->pause_filter_count, old);
> > +		trace_kvm_ple_window_update(vcpu->vcpu_id,
> > +					    control->pause_filter_count, old);
> > +	}
> >  }
> >  
> >  static void shrink_ple_window(struct kvm_vcpu *vcpu)
> > @@ -1287,11 +1287,11 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
> >  						    pause_filter_count,
> >  						    pause_filter_count_shrink,
> >  						    pause_filter_count);
> > -	if (control->pause_filter_count != old)
> > +	if (control->pause_filter_count != old) {
> >  		mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> > -
> > -	trace_kvm_ple_window_shrink(vcpu->vcpu_id,
> > -				    control->pause_filter_count, old);
> > +		trace_kvm_ple_window_update(vcpu->vcpu_id,
> > +					    control->pause_filter_count, old);
> > +	}
> >  }
> >  
> >  static __init int svm_hardware_setup(void)
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > index 76a39bc25b95..97df9d7cae71 100644
> > --- a/arch/x86/kvm/trace.h
> > +++ b/arch/x86/kvm/trace.h
> > @@ -890,36 +890,27 @@ TRACE_EVENT(kvm_pml_full,
> >  	TP_printk("vcpu %d: PML full", __entry->vcpu_id)
> >  );
> >  
> > -TRACE_EVENT(kvm_ple_window,
> > -	TP_PROTO(bool grow, unsigned int vcpu_id, int new, int old),
> > -	TP_ARGS(grow, vcpu_id, new, old),
> > +TRACE_EVENT(kvm_ple_window_update,
> > +	TP_PROTO(unsigned int vcpu_id, int new, int old),
> > +	TP_ARGS(vcpu_id, new, old),
> >  
> >  	TP_STRUCT__entry(
> > -		__field(                bool,      grow         )
> >  		__field(        unsigned int,   vcpu_id         )
> >  		__field(                 int,       new         )
> >  		__field(                 int,       old         )
> 
> Not your code, but these should really be 'unsigned int', especially now
> that they are directly compared when printing "growed" versus "shrinked".
> For SVM it doesn't matter since the underlying hardware fields are only
> 16 bits, but on VMX they're 32 bits, e.g. theoretically userspace could
> set ple_window and ple_window_max to a negative value.
> 
> The ple_window variable in struct vcpu_vmx and local snapshots of the
> field should also be updated, but that can be done separately.

Indeed.  Let me add a separated patch.  Thanks,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit
  2019-09-05  2:15     ` Peter Xu
@ 2019-09-05 15:56       ` Sean Christopherson
  0 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2019-09-05 15:56 UTC (permalink / raw)
  To: Peter Xu; +Cc: kvm, Paolo Bonzini, Wanpeng Li, Krish Sadhukhan

On Thu, Sep 05, 2019 at 10:15:15AM +0800, Peter Xu wrote:
> On Wed, Sep 04, 2019 at 10:26:58AM -0700, Sean Christopherson wrote:
> > On Thu, Aug 15, 2019 at 06:34:56PM +0800, Peter Xu wrote:
> > > Tracing the ID helps to pair vmenters and vmexits for guests with
> > > multiple vCPUs.
> > > 
> > > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  arch/x86/kvm/trace.h | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > > index b5c831e79094..c682f3f7f998 100644
> > > --- a/arch/x86/kvm/trace.h
> > > +++ b/arch/x86/kvm/trace.h
> > > @@ -232,17 +232,20 @@ TRACE_EVENT(kvm_exit,
> > >  		__field(	u32,	        isa             )
> > >  		__field(	u64,	        info1           )
> > >  		__field(	u64,	        info2           )
> > > +		__field(	int,	        vcpu_id         )
> > 
> > It doesn't actually affect anything, but vcpu_id is stored and printed as
> > an 'unsigned int' everywhere else in the trace code.  Stylistically I like
> > that approach even though struct kvm_vcpu holds it as a signed int.
> 
> True.  I can switch to unsigned int to get aligned with the rest if
> there's other comment to address.  Though from codebase-wise I would
> even prefer signed because it gives us a chance to set an invalid vcpu
> id (-1) where necessary, or notice something severly wrong when <-1.

I agree that a signed int makes sense for flows like kvm_get_vcpu_by_id(),
where the incoming id isn't sanitized.  But for struct kvm_vcpu, a negative 
vcpu_id is simply impossible, e.g. vcpu_id is set to a postive (and capped)
value as part of the core vcpu initialization and is never changed.  I
prefer storing an unsigned val in the tracing as it communicates that
vcpu_id is always valid.  I suspect struct kvm_vcpu uses a signed int
purely to avoid having to constantly cast it to an int.

> After all it should far cover our usage (IIUC max vcpu id should be
> 512 cross archs).

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-15 10:34 [PATCH v2 0/3] KVM: X86: Some tracepoint enhancements Peter Xu
2019-08-15 10:34 ` [PATCH v2 1/3] KVM: X86: Trace vcpu_id for vmexit Peter Xu
2019-09-04 17:26   ` Sean Christopherson
2019-09-05  2:15     ` Peter Xu
2019-09-05 15:56       ` Sean Christopherson
2019-08-15 10:34 ` [PATCH v2 2/3] KVM: X86: Remove tailing newline for tracepoints Peter Xu
2019-09-04 17:27   ` Sean Christopherson
2019-08-15 10:34 ` [PATCH v2 3/3] KVM: X86: Tune PLE Window tracepoint Peter Xu
2019-09-04 17:32   ` Sean Christopherson
2019-09-05  2:25     ` Peter Xu

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org kvm@archiver.kernel.org
	public-inbox-index kvm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox