On Wed, Jul 15, 2009 at 11:48:17PM +0300, Gleb Natapov wrote: > > > + spin_unlock(&ioapic->lock); > > > + kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > > > + spin_lock(&ioapic->lock); > > > > While traversing the IOAPIC pins you drop the lock and acquire it again, > > which is error prone: what if the redirection table is changed in > > between, for example? > > > Yes, the ack notifiers is a problematic part. The only thing that > current ack notifiers do is set_irq_level(0) so this is not the problem > in practice. I'll see if I can eliminate this dropping of the lock here. Currently, yes. But take into account that an ack notifier might do other things than set_irq_level(0). Say for example an ack notifier that takes the PIC or IOAPIC lock (for whatever reason). > > Also, irq_lock was held during ack and mask notifier callbacks, so they > > (the callbacks) did not have to worry about concurrency. > > > Is it possible to have more then one ack for the same interrupt line at > the same time? Why not? But maybe this is a somewhat stupid point, because you can require the notifiers to handle that. > > You questioned the purpose of irq_lock earlier, and thats what it is: > > synchronization between pic and ioapic blur at the irq notifiers. > > > Pic has its own locking and it fails miserably in regards ack notifiers. > I bet nobody tried device assignment with pic. It will not work. It fails to take irq_lock on automatic EOI because on guest entry interrupts are disabled (see d5ecfdd25c412df9c0ecf3ab8e066461fd4f69df). > irq_lock is indeed used to synchronization ioapic access, but the way > it is done requires calling kvm_set_irq() with lock held and this adds > unnecessary lock on a msi irq injection path. > > > Using RCU to synchronize ack/mask notifier list walk versus list > > addition is good, but i'm not entirely sure that smaller locks like you > > are proposing is a benefit long term (things become much more tricky). > Without removing irq_lock RCU is useless since the list walking is always > done with irq_lock held. I see smaller locks as simplification BTW. The > thing is tricky now, or so I felt while trying to understand what > irq_lock actually protects. With smaller locks with names that clearly > indicates which data structure it protects I feel the code is much > easy to understand. What is worrying is if you need to take both PIC and IOAPIC locks for some operation (then have to worry about lock ordering etc). > > Maybe it is the only way forward (and so you push this complexity to the > > ack/mask notifiers), but i think it can be avoided until its proven that > > there is contention on irq_lock (which is reduced by faster search). > This is not about contention. This is about existence of the lock in the > first place. With the patch set there is no lock at msi irq injection > path at all and this is better than having lock no matter if the lock is > congested or not. There is still a lock on ioapic path (which MSI does > not use) and removing of this lock should be done only after we see > that it is congested, because removing it involves adding a number of > atomic accesses, so it is not clear win without lock contention. > (removing it will also solve ack notification problem hmmm) -- 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
