Re: Debugging Thinkpad T430s occasional suspend failure.
From: Thomas Gleixner
Date: Mon Feb 18 2013 - 14:58:51 EST
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@xxxxxxxxx>:
> > > 2013/2/17 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>:
> > >> On Sun, Feb 17, 2013 at 7:11 AM, Frederic Weisbecker <fweisbec@xxxxxxxxx> 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
--
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/