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;
>> +}
>> +
>> +/*
>>    
>>     
>
>