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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Sep 30, 2009 at 09:01:51AM +0800, Zhai, Edwin wrote:
> 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).

kvm_vcpu_on_spin() should add the vcpu to vcpu->wq (so a new pending
interrupt wakes it up immediately).

Do you (and/or Mark) have any numbers for non-vcpu overcommited guests?