Re: [PATCH 6/8 v2] Move IO APIC to its own lock.

[Avi Kivity <avi@redhat.com>]

On 08/12/2009 12:47 PM, Gleb Natapov wrote:
>> Ah, the real motivation is msi.  Pushing locks down doesn't help if we
>> keep locking them.  But for msi we avoid the lock entirely.
>>
>>      
> Yes. MSI is one. Multiple IOAPICs may inject interrupt in parallel too
> (if we will choose to implement multiple IOAPICs sometime).
>    

Right.  Given msi, I don't think we'll have multiple ioapics though.

>>>> Why a spinlock and not a mutex?
>>>>
>>>>
>>>>          
>>> Protected sections are small and we do not sleep there.
>>>
>>>        
>> So what?  A mutex is better since it allows preemption (and still has
>> spinlock performance if it isn't preempted).
>>
>>      
> This lock will be taken during irq injection from irqfd, so may be leave
> it as spinlock and take it _irqsave()? Do we want to allow irq injection
> from interrupt context? Otherwise if you say that performance is the
> same I don't care one way or the other.
>    

Let's leave _irqsave() until later since that has other implications.

>>>> Need to explain why this is safe.  I'm not sure it is, because we touch
>>>> state afterwards in pic_intack().  We need to do all vcpu-synchronous
>>>> operations before dropping the lock.
>>>>
>>>>          
>>> Forst pic_intack() calls pic_clear_isr() only in auto eoi mode and this mode
>>> is already broken for assigned devices. Second for level triggered
>>> interrupts pic_intack() does nothing after calling pic_clear_isr() and
>>> third I can move pic_clear_isr() call to the end of pic_intack().
>>>
>>>        
>> I meant, in a comment.
>>      
> I you agree with above I'll add it as a comment.
>    

Sure.

>> It should.  What if there's a reset with an assigned device?  We need to
>> release the device interrupt (after doing FLR?).
>>      
> Doing this will just re-enable host interrupt while irq condition is not
> cleared in the device. The host will hang. So I think we really shouldn't.
>    

Ok.  What about timer acks?




>>> I don't see why we clear remote_irr before dropping the lock. If, while
>>> lock was dropped, interrupt was delivered to this entry it will be
>>> injected when ack notifier returns.
>>>
>>>        
>> But we'll clear remote_irr afterward the redelivery, and we should to
>> that only after the new interrupt is acked.
>>      
> It depend on whether you consider calling ack notifiers a part of
> interrupt acknowledgement process.

I don't really care, but the ack process has to be atomic.  Since we 
need to drop the lock, it means the notifier is not part of the process.

> If you do then remote_irr should not
> be cleared before ack notifiers since ack process is not completed yet.
> With current users functionally it shouldn't matter when we clear
> remote_irr. I prefer doing it like we do it now since this how it was
> before my patches and since code is simpler this way.
>    

No, I think it introduces a race if an interrupt is raised while the ack 
notifier is running.



>> No, I mean keep the for loop in kvm_ioapic_update_eoi().
>>
>>      
> Can't do that. __kvm_ioapic_update_eoi() is called from other place with
> lock held already.
>    

Ok.

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