Re: [PATCH] [RESEND] KVM:VMX: Add support for Pause-Loop Exiting

["Zhai, Edwin" <edwin.zhai@intel.com>]

Avi,
Any comments for this new patch?
Thanks,


Zhai, Edwin wrote:
> Avi Kivity wrote:
>   
>> +#define KVM_VMX_DEFAULT_PLE_GAP    41
>> +#define KVM_VMX_DEFAULT_PLE_WINDOW 4096
>> +static int __read_mostly ple_gap = KVM_VMX_DEFAULT_PLE_GAP;
>> +module_param(ple_gap, int, S_IRUGO);
>> +
>> +static int __read_mostly ple_window = KVM_VMX_DEFAULT_PLE_WINDOW;
>> +module_param(ple_window, int, S_IRUGO);
>>    
>>   
>>
>> Shouldn't be __read_mostly since they're read very rarely (__read_mostly 
>> should be for variables that are very often read, and rarely written).
>>   
>>     
>
> In general, they are read only except that experienced user may try 
> different parameter for perf tuning.
>
>   
>> I'm not even sure they should be parameters.
>>   
>>     
>
> For different spinlock in different OS, and for different workloads, we 
> need different parameter for tuning. It's similar as the enable_ept.
>
>   
>>   
>>     
>>>   /*
>>> + * Indicate a busy-waiting vcpu in spinlock. We do not enable the PAUSE
>>> + * exiting, so only get here on cpu with PAUSE-Loop-Exiting.
>>> + */
>>> +static int handle_pause(struct kvm_vcpu *vcpu,
>>> +				struct kvm_run *kvm_run)
>>> +{
>>> +	ktime_t expires;
>>> +	skip_emulated_instruction(vcpu);
>>> +
>>> +	/* Sleep for 1 msec, and hope lock-holder got scheduled */
>>> +	expires = ktime_add_ns(ktime_get(), 1000000UL);
>>>    
>>>     
>>>       
>> I think this should be much lower, 50-100us.  Maybe this should be a 
>> parameter.  With 1ms we losing significant cpu time if the congestion 
>> clears.
>>   
>>     
>
> I have made it a parameter with default value of 100 us.
>
>   
>>   
>>     
>>> +	set_current_state(TASK_INTERRUPTIBLE);
>>> +	schedule_hrtimeout(&expires, HRTIMER_MODE_ABS);
>>> +
>>>    
>>>     
>>>       
>> Please add a tracepoint for this (since it can cause significant change 
>> in behaviour), 
>>     
>
> Isn't trace_kvm_exit(exit_reason, ...) enough? We can tell the PLE 
> vmexit from other vmexits.
>
>   
>> and move the logic to kvm_main.c.  It will be reused by 
>> the AMD implementation, possibly my software spinlock detector, 
>> paravirtualized spinlocks, and hopefully other architectures.
>>   
>>     
>
> Done.
>   
>>   
>>     
>>> +	return 1;
>>> +}
>>> +
>>> +/*
>>>    
>>>     
>>>       
>>   
>>