Re: [PATCH v9 0/2] kvm: level irqfd support

Alex Williamson
2012-09-17T18:08:17Z

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