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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 12, 2009 at 12:57:07PM +0300, Avi Kivity wrote:
> 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.
>
So change it to mutex then?

>>>>> 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?
>
Interrupt wasn't processed by a guest. Why should it be accounted as
such?

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

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux