Re: [tip:irq/core] genirq: Flush the irq thread on synchronization

[Ido Yariv <ido@wizery.com>]

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.