Avi,
I modify it according your comments. The only thing I want to keep is 
the module param ple_gap/window.  Although they are not per-guest, they 
can be used to find the right value, and disable PLE for debug purpose.

Thanks,


Avi Kivity wrote:
> On 09/28/2009 11:33 AM, 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.
>>     
>
>
> __read_mostly doesn't just mean it's read mostly.  It also means it's 
> read often.  Otherwise it's just wasting space in hot cachelines.
>
>   
>>> 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.
>>     
>
> No, global parameters don't work for tuning workloads and guests since 
> they cannot be modified on a per-guest basis.  enable_ept is only useful 
> for debugging and testing.
>
>   
>>>> +    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.
>>     
>
> Right.  I thought of the software spinlock detector, but that's another 
> problem.
>
> I think you can drop the sleep_time parameter, it can be part of the 
> function.  Also kvm_vcpu_sleep() is confusing, we also sleep on halt.  
> Please call it kvm_vcpu_on_spin() or something (since that's what the 
> guest is doing).
>
>