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

[Peter Xu <zhexu@redhat.com>]

On Mon, Jul 29, 2019 at 09:23:38AM -0700, Sean Christopherson wrote:
> On Mon, Jul 29, 2019 at 01:32:43PM +0800, Peter Xu wrote:
> > The PLE window tracepoint triggers easily and it 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 (4096 -> 8192)
> 
> This seems a bit too terse, e.g. requires a decent amount of effort to
> do relatively simple things like show only cases where the windows was
> shrunk, or grew/shrunk by a large amount.  In this case, more is likely
> better, e.g.: 
> 
>   kvm_ple_window_changed: vcpu 4 ple_window 8192 old 4096 grow 4096
> 
> and
> 
>   kvm_ple_window_changed: vcpu 4 ple_window 4096 old 8192 shrink 4096

How about:

   kvm_ple_window: vcpu 4 (4096 -> 8192, growed)

Or:

   kvm_ple_window: vcpu 4 old 4096 new 8192 growed

I would prefer the arrow which is very clear to me to show a value
change, but I'd be fine to see what's your final preference or any
further reviewers.  Anyway I think any of them is clearer than the
original version...

> 
> 
> Tangentially related, it'd be nice to settle on a standard format for
> printing field+val.  Right now there are four different styles, e.g.
> "field=val", "field = val", "field: val" and "field val".

Right, I ses "field val" is used most frequently.  But I didn't touch
those up because they haven't yet caused any confusion to me.

[...]

> >  	TP_STRUCT__entry(
> > -		__field(                bool,      grow         )
> 
> Side note, if the tracepoint is invoked only on changes the "grow" field
> can be removed even if the tracepoint prints grow vs. shrink, i.e. there's
> no ambiguity since new==old will never happen.

But I do see it happen...  Please see below.

> 
> >  		__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 (%d -> %d)",
> > +	          __entry->vcpu_id, __entry->old, __entry->new)
> >  );
> >  
> > -#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)
> > +#define trace_kvm_ple_window_changed(vcpu, new, old)		\
> > +	do {							\
> > +		if (old != new)					\
> > +			trace_kvm_ple_window(vcpu, new, old);	\
> > +	} while (0)
> >  
> >  TRACE_EVENT(kvm_pvclock_update,
> >  	TP_PROTO(unsigned int vcpu_id, struct pvclock_vcpu_time_info *pvclock),
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index d98eac371c0a..cc1f98130e6a 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -5214,7 +5214,7 @@ static void grow_ple_window(struct kvm_vcpu *vcpu)
> >  	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_changed(vcpu->vcpu_id, vmx->ple_window, old);
> 
> No need for the macro, the snippet right about already checks 'new != old'.
> Though I do like the rename, i.e. rename the trace function to
> trace_kvm_ple_window_changed().

Do you mean this one?

	if (vmx->ple_window != old)
		vmx->ple_window_dirty = true;

It didn't return, did it? :)

Thanks,

-- 
Peter Xu