On Wed, 2012-08-22 at 11:25 +0300, Michael S. Tsirkin wrote: > On Tue, Aug 21, 2012 at 07:28:15PM -0600, Alex Williamson wrote: > > On Wed, 2012-08-22 at 03:31 +0300, Michael S. Tsirkin wrote: > > > On Tue, Aug 21, 2012 at 01:28:57PM -0600, Alex Williamson wrote: > > > > Here's the much anticipated re-write of support for level irqfds. As > > > > Michael suggested, I've rolled the eoi/ack notification fd into > > > > KVM_IRQFD as a new mode. For lack of a better name, as there seems to > > > > be objections to associating this specifically with an EOI or an ACK, > > > > I've name this OADN or "On Ack, De-assert & Notify". > > > > > > > > Patch 1of2 switches current KVM_IRQFDs to use their own IRQ source ID > > > > since we're potentially stepping on KVM_USERSPACE_IRQ_SOURCE_ID. > > > > Unfurtunately I was not able to make 2of2 use a single IRQ source ID, > > > > the reason is it's racy. Objects to track OADNs are made dynamically, > > > > we look through existing ones for a match under spinlock and setup a > > > > new one if there's no match. On teardown, we can remove the OADN from > > > > the list under lock, but that same lock prevents us from de-assigning > > > > the IRQ ACK notifier or waiting for an RCU grace period. We must make > > > > sure that any unused GSI is de-asserted, but the above means it's > > > > possible that another OADN has been created for this source ID/GSI > > > > and de-asserting the GSI could lead to breakage. > > > > > > I do not see it. What breakage? Could you give an example please? > > > > > > > > > I think what you are saying is last deassign must clear > > > since otherwise we never will clear. > > > I agree it is either that or delay deassign until ack. > > > > > > Can it be as simple as this (after all rcu etc dances)? > > > lock irqfds > > > if no oadns > > > set level to 0 > > > unlock irqfds > > > ? > > > > lock irqfds > > remove irqfd from oadn list > > if no oadns > > remove oadn > > set gsi 0 > > unlock > > lock irqfds > > new oadn > > unlock irqfds > > > > >> EOI > > ack notify new oadn > > de-assert gsi > > notify new oadn > > >> re-assert irqfd > > ack notify old oadn > > de-assert gsi > > notify old oadn > > > > synchronize_rcu > > > > kvm_unregister_irq_ack_notifier > > > > So, because the unregister is removed from the final clear and because > > we share an IRQ source ID there's a window where we can have two oadns > > registered for the same GSI. The new one will de-assert and notify > > while the old one has an empty list to notify, but still de-asserts. We > > can therefore de-assert w/o notify. > > > > By using a new source ID, we separate the two so users of the new oadn > > can't race the old and we can cleanly free the old source ID, > > de-asserting it. > > Need to think about it some more but is the problem two > ack notifiers for the same gsi? yes > In that case, how about we add __kvm_unregister_irq_ack_notifier > with no locking, and do most of the above under > kvm->irq_lock? Converting locks makes me nervous, but I'll give it a shot. I don't know how easy/possible it is though. I know in previous iterations I tried to make something similar to irqfd use a mutex and couldn't, but I don't remember the details. > With one change: it is better not to call synchronize_rcu > under irq lock, I think we can safely move it to after > __kvm_unregister_irq_ack_notifier. Yep, that makes the interface pretty ugly though as we then have two separate, but dependent steps. Thanks, Alex -- 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
