Re: KVM: test for LAPIC timer migration request after disabling IRQ's

[Avi Kivity <avi@qumranet.com>]

Marcelo Tosatti wrote:
> On Sun, May 18, 2008 at 09:35:14AM +0300, Avi Kivity wrote:
>   
>> Marcelo Tosatti wrote:
>>     
>>> (resending to kvm@vger.kernel.org)
>>>
>>> A guest vcpu instance can be scheduled to a different physical CPU
>>> between the test for KVM_REQ_MIGRATE_TIMER and local_irq_disable().
>>>
>>> If that happens, the timer will only be migrated to the current pCPU on
>>> the next exit, meaning that guest LAPIC timer event can be delayed until
>>> a host timer interrupt is triggered.
>>>
>>> Fix that by testing the migration request after local_irq_disable(),
>>> similarly to what is done for KVM_REQ_MMU_RELOAD.
>>>
>>>  
>>>       
>> An alternative fix is to have __apic_timer_fn() call 
>> smp_call_function_single() to the current vcpu->cpu with a dummy 
>> function (not saying it's better).
>>     
>
> smp_call_function_single() can't function from IRQ context (there is a
> global lock protecting internal data and waiting for a remote CPU to
> finish a function from IRQ context does not sound good). Remember the
> vcpu_kick() in the pit timer handler? :)
>
>   

Right you are.

>> How about
>>
>>
>>    if (vcpu->requests) {
>>          local_irq_enable();
>>          preempt_enable();
>>          r = 1;
>>          goto out;
>>    }
>>
>> ?
>>
>> It ensures that we never enter guest mode with _any_ pending request, so 
>> long as request bits are set on vcpu->cpu or are followed by an IPI.  I 
>> think it fixes a similar race with KVM_REQ_TLB_FLUSH.
>>     
>
> Yes doing it for any pending requests sounds much better.
>
> As for KVM_REQ_TLB_FLUSH, the check happens after local_irq_disable()
> already. Which is sort of problematic in that requests arriving after
> entering the IRQ-disabled section but before the test_and_clear_bit()
> will run ->tlb_flush but won't clear the pending IPI, so there's an exit
> for nothing. Probably uncommon though.
>
>   

True.  The main advantage of my suggestion is that control flow is
simpler, not any performance or correctness benefit.


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