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

[Avi Kivity <avi@redhat.com>]

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

-- 
error compiling committee.c: too many arguments to function