Davide Libenzi wrote: > On Fri, 19 Jun 2009, Davide Libenzi wrote: > > >> On Fri, 19 Jun 2009, Gregory Haskins wrote: >> >> >>> I am fairly confident it is not that simple after having thought about >>> this issue over the last few days. But I've been wrong in the past. >>> Propose a patch and I will review it for races/correctness, if you >>> like. Perhaps a combination of that plus your asymmetrical locking >>> scheme would work. One of the challenges you will hit is avoiding ABBA >>> between your "get" lock and the wqh, but good luck! >>> >> A patch for what? The eventfd patch is a one-liner. >> Yes, understood. What I was trying to gently say is that the one-liner proposal alone is still broken afaict. However, if there is another solution that works that you like better than 133-liner I posted, I am more than willing to help analyze it. In the end, I only care that this is fixed. >> It seems hard to believe that the thing cannot be handled on your side. >> Once the wake_up_locked() is turned into a wake_up(), what other races are >> there? >> > > AFAICS, the IRQfd code simply registers the callback to ->poll() and waits > for two events. > In the POLLIN event, you schedule_work(&irqfd->inject) and there are no > races there AFAICS (you basically do not care of anything eventfd memory > related at all). > For POLLHUP, you do: > > spin_lock(irqfd->slock); > if (irqfd->wqh) > schedule_work(&irqfd->inject); > irqfd->wqh = NULL; > spin_unlock(irqfd->slock); > > In your work function you notice the POLLHUP condition and take proper > action (dunno what it is in your case). > In your kvm_irqfd_release() function: > > spin_lock(irqfd->slock); > if (irqfd->wqh) > remove_wait_queue(irqfd->wqh, &irqfd->wait); > irqfd->wqh = NULL; > spin_unlock(irqfd->slock); > > Any races in there? > Yes, for one you have an ABBA deadlock on the irqfd->slock and wqh->lock (recall wqh has to be locked to fix that other race I mentioned). (As a hint, I think I fixed 4-5 races with these patches, so there are a few others still lurking as well) -Greg
Attachment:
signature.asc
Description: OpenPGP digital signature
