lkml.org 
[lkml]   [2012]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.


\
 
 \ /
  Last update: 2012-03-16 00:03    [W:0.084 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site