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

[Alexander Gordeev]

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/