Re: [PATCH v2 3/3] KVM: X86: Tune PLE Window tracepoint

[Peter Xu <peterx@redhat.com>]

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