RE: [RFC 3/3] arch/x86/kvm: SVM: Introduce pause loop exit logic in SVM

[Moger, Babu]

> -----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.