Messages in this thread | | | Date | Fri, 16 Mar 2012 00:59:52 +0200 | From | Ido Yariv <> | Subject | Re: [tip:irq/core] genirq: Flush the irq thread on synchronization |
| |
Hi Alexander,
On Thu, Mar 15, 2012 at 08:07:56PM +0100, Alexander Gordeev wrote: > > - /* Prevent a stale desc->threads_oneshot */ > > - irq_finalize_oneshot(desc, action, true); > > + /* > > + * This is the regular exit path. __free_irq() is stopping the > > + * thread via kthread_stop() after calling > > + * synchronize_irq(). So neither IRQTF_RUNTHREAD nor the > > + * oneshot mask bit should be set. > > + * > > + * Verify that this is true. > > + */ > > + if (WARN_ON(test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags))) > > + wake_threads_waitq(desc); > > If we hit this warning we do not know if this IRQTF_RUNTHREAD bit's count in > desc->threads_active was decremented or not. > > Nevertheless, wake_threads_waitq() gets called and desc->threads_active gets > decremented. As result, if desc->threads_active initially was decremented, we > might wrongly wake up the queue while some threaded handler is still running. > > By contrast, if we choose not to wake up here, we might stuck in > synchronize_irq(). Which is probably better than a fooling synchronize_irq().
AFAICT, IRQTF_RUNTHREAD and the desc->threads_active are always modified together: desc->threads_active is incremented if and only if IRQTF_RUNTHREAD is set after being cleared (in irq_wake_thread()). desc->threads_active is decremented in wake_threads_waitq(), which is only called when IRQTF_RUNTHREAD is cleared.
It seems that if we get to this point, either IRQTF_RUNTHREAD is set and desc->threads_active was not decremented, or it is not set.
Do you see any case where the two will be out of sync?
> > + > > + if (WARN_ON(desc->threads_oneshot & action->thread_mask)) > > + irq_finalize_oneshot(desc, action, true); > > This check is called when the action is already removed in __free_irq() and no > desc->lock is held. Hence, a concurrent __setup_irq() could reallocate the very > same bit in the meantime. So neither irq_finalize_oneshot() nor the warning > are legitimate here.
That's interesting. However, it doesn't seem to be a regression that's caused by this patch (the irq_finalize_oneshot() was there before), so it might be a good idea to fix this separately.
Thanks, Ido.
| |