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@xxxxxxxxxx>
> > ---
> > 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
