Re: Debugging Thinkpad T430s occasional suspend failure.

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, 18 Feb 2013, Frederic Weisbecker wrote:
> On Sun, Feb 17, 2013 at 10:02:11PM +0100, Frederic Weisbecker wrote:
> > 2013/2/17 Frederic Weisbecker <fweisbec@gmail.com>:
> > > 2013/2/17 Linus Torvalds <torvalds@linux-foundation.org>:
> > >> On Sun, Feb 17, 2013 at 7:11 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > >>>
> > >>> preempt_value_in_interrupt() looks buggy in your patch: it makes
> > >>> invoke_softirq() returning if (val & HARDIRQ_MASK). But that's always
> > >>> true since you have moved further the sub_preempt_count(IRQ_EXIT)
> > >>> further.
> > >>
> > >> No, that's not it. The value hasn't been written back yet, but it already did:
> > >>
> > >> +       int offset = IRQ_EXIT_OFFSET;
> > >> +       int count = preempt_count() - offset;
> > >>
> > >> so the 'count' has the IRQ_EXIT_OFFSET already subtracted. So no,
> > >> HARDIRQ_MASK is *not* always set.
> > >
> > > Another thing. Perhaps we can push the idea of your patch a little
> > > further by re-entering HARDIRQ_OFFSET at the end of the softirq
> > > processing and do the final sub_preempt_count(HARDIRQ_OFFSET) at the
> > > very end of irq_exit().
> > >
> > > This way irq_exit() looks a bit more simple to me: everything there
> > > becomes considered as in hardirq: (ie: rcu_irq_exit() and
> > > tick_nohz_irq_exit() won't appear anymore as weird special cases) and
> > > we get rid of that IRQ_EXIT_OFFSET hack that fixes the CONFIG_PREEMPT
> > > case.
> > >
> > > I'm attaching an untested patch that modify yours. It's probably
> > > missing some corner cases that rely on in_interrupt() value in
> > > rcu_irq_exit() and tick_nohz_irq_exit() and may be other things.
> > 
> > I messed up preempt_offset_in_interrupt() with in_atomic() code
> > instead of in_interrupt(). Anyway the patch is untested and is more
> > there to get your opinion for now. I'll put some more care on it if
> > people like it.
> 
> Here is an updated version. It cleans up things a bit and boots fine with my usual
> config. There might be still some small details to work on but here is the big picture.
> 
> What do you think?
> 
> ---
> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index 624ef3f..d1ab5b4 100644
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -87,6 +87,7 @@
>  #define in_irq()		(hardirq_count())
>  #define in_softirq()		(softirq_count())
>  #define in_interrupt()		(irq_count())
> +#define in_nesting_interrupt()  (irq_count() - HARDIRQ_OFFSET)
>  #define in_serving_softirq()	(softirq_count() & SOFTIRQ_OFFSET)
>  
>  /*
> @@ -118,10 +119,8 @@
>  
>  #ifdef CONFIG_PREEMPT_COUNT
>  # define preemptible()	(preempt_count() == 0 && !irqs_disabled())
> -# define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
>  #else
>  # define preemptible()	0
> -# define IRQ_EXIT_OFFSET HARDIRQ_OFFSET
>  #endif
>  
>  #if defined(CONFIG_SMP) || defined(CONFIG_GENERIC_HARDIRQS)
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index ed567ba..69fbefd 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -277,6 +277,17 @@ restart:
>  	tsk_restore_flags(current, old_flags, PF_MEMALLOC);
>  }
>  
> +#ifndef __ARCH_IRQ_EXIT_IRQS_DISABLED

We really should get rid of that and make irqs disabled on irq exit
mandatory.

> +static void __do_softirq_irq_unsafe(void)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	__do_softirq();
> +	local_irq_restore(flags);
> +}
> +#endif
> +
>  #ifndef __ARCH_HAS_DO_SOFTIRQ
>  
>  asmlinkage void do_softirq(void)
> @@ -320,20 +331,45 @@ void irq_enter(void)
>  	__irq_enter();
>  }
>  
> +/*
> + * Invoce softirq's from irq_exit().
> + *
> + * Return the preempt offset: either IRQ_EXIT_OFFSET (if we
> + * did nothing to the preemption count) or SOFTIRQ_OFFSET (in
> + * case we did softirq processing and changed the preemption
> + * count to account for that).
> + */
>  static inline void invoke_softirq(void)
>  {
> -	if (!force_irqthreads) {
> +	/* Can we run softirq's at all? We migth be nesting interrupts */
> +	if (in_nesting_interrupt())
> +		return;

We might also be in a softirq disabled region where it makes no sense
to continue. So this should be

   in_nesting_interrupt() || in_softirq()

> +	/* Are there any pending? */
> +	if (!local_softirq_pending())
> +		return;
> +
> +	/* Do we force irq threads? If so, just wake up the daemon */
> +	if (force_irqthreads) {
> +		wakeup_softirqd();
> +		return;
> +	}
> +
> +	/*
> +	 * Ok, do actual softirq handling!
> +	 *
> +	 * This downgrades us from hardirq context to softirq context.
> +	 */
> +	preempt_count() += SOFTIRQ_OFFSET - HARDIRQ_OFFSET;
> +
> +	trace_softirqs_off(__builtin_return_address(0));
>  #ifdef __ARCH_IRQ_EXIT_IRQS_DISABLED
> -		__do_softirq();
> +	__do_softirq();
>  #else
> -		do_softirq();
> +	__do_softirq_irq_unsafe();
>  #endif
> -	} else {
> -		__local_bh_disable((unsigned long)__builtin_return_address(0),
> -				SOFTIRQ_OFFSET);
> -		wakeup_softirqd();
> -		__local_bh_enable(SOFTIRQ_OFFSET);
> -	}
> +	preempt_count() += HARDIRQ_OFFSET - SOFTIRQ_OFFSET;
> +	trace_softirqs_on((unsigned long)__builtin_return_address(0));
>  }
>  
>  /*
> @@ -343,17 +379,17 @@ void irq_exit(void)
>  {
>  	vtime_account_irq_exit(current);
>  	trace_hardirq_exit();
> -	sub_preempt_count(IRQ_EXIT_OFFSET);
> -	if (!in_interrupt() && local_softirq_pending())
> -		invoke_softirq();
> +	invoke_softirq();
>  
>  #ifdef CONFIG_NO_HZ
>  	/* Make sure that timer wheel updates are propagated */
> -	if (idle_cpu(smp_processor_id()) && !in_interrupt() && !need_resched())
> -		tick_nohz_irq_exit();
> +	if (idle_cpu(smp_processor_id())) {
> +		if (!in_nesting_interrupt() && !need_resched())
> +			tick_nohz_irq_exit();
> +	}
>  #endif
>  	rcu_irq_exit();
> -	sched_preempt_enable_no_resched();
> +	sub_preempt_count(HARDIRQ_OFFSET);
>  }

Otherwise this is a nice improvement!

Thanks,

	tglx