Re: [PATCH] KVM: avoid taking ioapic mutex for non-ioapic EOIs

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Dec 29, 2009 at 12:35:17PM +0200, Avi Kivity wrote:
> On 12/28/2009 11:30 PM, Marcelo Tosatti wrote:
>> On Mon, Dec 28, 2009 at 10:47:20PM +0200, Avi Kivity wrote:
>>    
>>> On 12/28/2009 10:37 PM, Marcelo Tosatti wrote:
>>>      
>>>> On Mon, Dec 28, 2009 at 02:08:30PM +0200, Avi Kivity wrote:
>>>>
>>>>        
>>>>> When the guest acknowledges an interrupt, it sends an EOI message to the local
>>>>> apic, which broadcasts it to the ioapic.  To handle the EOI, we need to take
>>>>> the ioapic mutex.
>>>>>
>>>>> On large guests, this causes a lot of contention on this mutex.  Since large
>>>>> guests usually don't route interrupts via the ioapic (they use msi instead),
>>>>> this is completely unnecessary.
>>>>>
>>>>> Avoid taking the mutex by introducing a handled_vectors bitmap.  Before taking
>>>>> the mutex, check if the ioapic was actually responsible for the acked vector.
>>>>> If not, we can return early.
>>>>>
>>>>>          
>>>> Can't you skip IOAPIC EOI for edge triggered interrupts (in the LAPIC
>>>> code), instead?
>>>>
>>>>        
>>> That's a lot cleaner, yes.  Indeed there's the TMR which holds this
>>> info.  Gleb suggested doing this in the local apic but we didn't think
>>> of using the TMR.
>>>      
>> Problem with storing in the LAPIC is you have to migrate the bitmap
>> along (otherwise can't know if EOI is from MSI or IOAPIC). But it sounds
>> much simpler.
>>    
>
> If we move the vectors_handled bitmap to the local apic, I don't see how  
> it simplified things.

Its vcpu-local.

>>> There's a small race there - the TMR is set after the IRR, so the
>>> interrupt can be injected and acked before the TMR is updated, but that
>>> can be fixed by switching the order.
>>>      
>> Makes sense.
>>    
>
> Btw, that race is already exposed to the guest, if it cares to read TMR.  
> I'll send a patch.
>
>>    
>>> But what about kvm_notify_acked_irq() in __kvm_ioapic_update_eoi()?
>>>      
>> Oops.
>>
>> The worrying thing about the handled_vectors bitmap in the IOAPIC is
>> that the update is not atomic wrt to lapic EOI handler.
>>
>> Unless its certain that races there are the guests problem, which should
>> have proper locking to never allow things like
>>
>> kvm_set_ioapic vec
>>                          update handled bitmap, vec not IOAPIC
>>                          handled anymore
>> ack lapic irq vec
>>
>> to happen.
>>
>> (with bitmap in LAPIC you avoid those things).
>>
>>    
>
> It seems real hardware will have the same issue (also look at comments  
> regarding irq migration in arch/x86/kernel/io_apic.c).  So I think a  
> guest is required to ack before migrating an irq.

Fair. Applied, thanks.