> -----Original Message-----
> From: Radim Krčmář <rkrcmar@xxxxxxxxxx>
> Sent: Wednesday, March 14, 2018 8:26 AM
> To: Moger, Babu <Babu.Moger@xxxxxxx>
> Cc: joro@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx;
> hpa@xxxxxxxxx; x86@xxxxxxxxxx; pbonzini@xxxxxxxxxx;
> kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [RFC 3/3] arch/x86/kvm: SVM: Introduce pause loop exit logic in
> SVM
>
> 2018-03-10 05:07+0000, Moger, Babu:
> > Radim,
> > Thanks for the comments. Taken care of most of the comments.
> > I have few questions/comments. Please see inline.
> >
> > > -----Original Message-----
> > > From: Radim Krčmář <rkrcmar@xxxxxxxxxx>
> > > Sent: Friday, March 9, 2018 12:13 PM
> > > To: Moger, Babu <Babu.Moger@xxxxxxx>
> > > Cc: joro@xxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx;
> > > hpa@xxxxxxxxx; x86@xxxxxxxxxx; pbonzini@xxxxxxxxxx;
> > > kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > Subject: Re: [RFC 3/3] arch/x86/kvm: SVM: Introduce pause loop exit
> logic in
> > > SVM
> > >
> > > 2018-03-02 11:17-0500, Babu Moger:
> > > > Bring the PLE(pause loop exit) logic to AMD svm driver.
> > > > We have noticed it help in situations where numerous pauses are
> > > generated
> > > > due to spinlock or other scenarios. Tested it with idle=poll and noticed
> > > > pause interceptions go down considerably.
> > > >
> > > > Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> > > > ---
> > > > @@ -1046,6 +1095,58 @@ static int avic_ga_log_notifier(u32 ga_tag)
> > > > return 0;
> > > > }
> > > >
> > > > +static void grow_ple_window(struct kvm_vcpu *vcpu)
> > > > +{
> > > > + struct vcpu_svm *svm = to_svm(vcpu);
> > > > + struct vmcb_control_area *control = &svm->vmcb->control;
> > > > + int old = control->pause_filter_count;
> > > > +
> > > > + control->pause_filter_count = __grow_ple_window(old,
> > > > + pause_filter_count,
> > > > + ple_window_grow,
> > > > +
> > > ple_window_actual_max);
> > > > +
> > > > + 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);
> > >
> > > Please move the tracing into __shrink_ple_window to share the code.
> > > This probably belongs to patch [2/3].
> >
> > I will have to pass vcpu_id, and have to make few changes to display old
> and new values.
> > I am afraid it might add few more extra instructions.
>
> Right, vcpu_id isn't available in that function.
> Keeping it like this is ok.
>
> > >
> > > > +/*
> > > > + * ple_window_actual_max is computed to be one
> grow_ple_window()
> > > below
> > > > + * ple_window_max. (See __grow_ple_window for the reason.)
> > > > + * This prevents overflows, because ple_window_max is int.
> > > > + * ple_window_max effectively rounded down to a multiple of
> > > ple_window_grow in
> > > > + * this process.
> > > > + * ple_window_max is also prevented from setting control-
> > > >pause_filter_count <
> > > > + * pause_filter_count.
> > > > + */
> > > > +static void update_ple_window_actual_max(void)
> > > > +{
> > > > + ple_window_actual_max =
> > > > + __shrink_ple_window(max(ple_window_max,
> > > pause_filter_count),
> > >
> > > (I have no idea what I was thinking when I wrote that for VMX. :[
> > > I'll write a patch to get rid of ple_window_actual_max, because its
> > > benefits are really minuscule and the logic is complicated.)
> >
> > If you are thinking of just straight forward removal, I can take care of it.
>
> And tweaking the overflow handling to account for that. Go ahead if
> you'd like to.
Ok. Will add new patch to the series to handle this. Thanks.
>
> > >
> > > > + pause_filter_count,
> > > > + ple_window_grow, SHRT_MIN);
> > > > +}
> > > > static __init int svm_hardware_setup(void)
> > > > {
> > > > int cpu;
> > > > @@ -1309,7 +1412,11 @@ static void init_vmcb(struct vcpu_svm *svm)
> > > > svm->vcpu.arch.hflags = 0;
> > > >
> > > > if (boot_cpu_has(X86_FEATURE_PAUSEFILTER)) {
> > > > - control->pause_filter_count = 3000;
> > > > + control->pause_filter_count = pause_filter_count;
> > > > + if (boot_cpu_has(X86_FEATURE_PFTHRESHOLD))
> > > > + control->pause_filter_thresh = pause_filter_thresh;
> > > > + else
> > > > + pause_filter_thresh = 0;
> > >
> > > Please move this to hardware_setup and also clear pause_filter_count if
> >
> > Moving this to hardware_setup will be a problem. We don't have access to
> svm data structure in hardware_setup.
>
> I mean just the pause_filter_thresh = 0 and pause_filter_count = 0 logic
Sure. Will take care.
> based on boot_cpu_has (it's weird if the user-visible parameters are
> corrected after starting a VM); VMCB configuration stays,
>
> thanks.
