Re: [PATCH 4/4] genirq: Get rid of unnecessary IRQTF_DIED flag

From: Alexander Gordeev
Date: Mon Mar 12 2012 - 07:39:20 EST


On Fri, Mar 09, 2012 at 05:17:16PM +0100, Thomas Gleixner wrote:
> On Fri, 9 Mar 2012, Oleg Nesterov wrote:
>
> > Of course I can't ack this, but afaics the whole series looks fine.
> >
> > Only one minor nit,
> >
> > On 03/09, Alexander Gordeev wrote:
> > >
> > > --- a/kernel/irq/handle.c
> > > +++ b/kernel/irq/handle.c
> > > @@ -60,8 +60,8 @@ static void irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
> > > * device interrupt, so no irq storm is lurking. If the
> > > * RUNTHREAD bit is already set, nothing to do.
> > > */
> > > - if (test_bit(IRQTF_DIED, &action->thread_flags) ||
> > > - test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
> > > + if (test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags) ||
> > > + (action->thread->flags & PF_EXITING))
> > > return;
> >
> > perhaps it makes sense to check PF_EXITING first, we do not want
> > to set IRQTF_RUNTHREAD in this case. I think this doesn't really
> > matter (and the check is obviously racy anyway), just looks a bit
> > confusing.
>
> It does not matter, the thread cleans it up in the exit path. So it's
> mostly cosmetic, but you are right, it reads better :)

Oleg, Thomas,

I swapped the checks because I wanted to avoid this scenario:


CPU1 CPU2

do_exit()
exit_signals(tsk); /* sets PF_EXITING */
smp_mb(); <--------------------------------+
exit_irq_thread(); |
irq_finalize_oneshot(); |
desc->threads_oneshot &= ~action->thread_mask;
unmask_irq(desc); |
|
/* Once the irq is unmasked new interrupt can come... */ |
|
irq_wake_thread() |
|
/* To notice PF_EXITING has changed a call to smp_rmb() should be here |
* to pair with smp_mb() in do_exit() ---------------------------------+
* But it is not, and the condition might not fulfill.
* Hence the thread might not return, although it should.
*/
if ((action->thread->flags & PF_EXITING) ||
test_and_set_bit(IRQTF_RUNTHREAD, &action->thread_flags))
return;
...

/* And would be stalled bit will be mistakenly set */
desc->threads_oneshot |= action->thread_mask;


Given that PF_EXITING check almost never fulfills but always gets executed and
test_and_set_bit() is a smp general barrier I thought swapping of PF_EXITING vs
IRQTF_RUNTHREAD checks is better than putting explicit smp_rmb().

If my considerations are flawed I will repost the patch with the original
order as it indeed reads better :)

--
Regards,
Alexander Gordeev
agordeev@xxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/