Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1314194 > unrolled thread

RCU lockup? (was: Re: [PATCH v2 tip/core/rcu 10/14] rcu: Don't redundantly disable irqs in rcu_irq_{enter,exit}())

Back to article view | Back to linux.kernel

Contents

  RCU lockup? (was: Re: [PATCH v2 tip/core/rcu 10/14] rcu: Don't  redundantly disable irqs in rcu_irq_{enter,exit}()) Geert Uytterhoeven <geert@linux-m68k.org> - 2016-01-21 14:30 +0100
    Re: RCU lockup? (was: Re: [PATCH v2 tip/core/rcu 10/14] rcu: Don't  redundantly disable irqs in rcu_irq_{enter,exit}()) "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-21 17:20 +0100
      Re: RCU lockup? (was: Re: [PATCH v2 tip/core/rcu 10/14] rcu: Don't  redundantly disable irqs in rcu_irq_{enter,exit}()) Geert Uytterhoeven <geert@linux-m68k.org> - 2016-01-22 10:00 +0100
        Re: RCU lockup? (was: Re: [PATCH v2 tip/core/rcu 10/14] rcu: Don't  redundantly disable irqs in rcu_irq_{enter,exit}()) "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-22 21:50 +0100
          Re: RCU lockup? (was: Re: [PATCH v2 tip/core/rcu 10/14] rcu: Don't  redundantly disable irqs in rcu_irq_{enter,exit}()) Geert Uytterhoeven <geert@linux-m68k.org> - 2016-01-23 10:50 +0100
            Re: RCU lockup? (was: Re: [PATCH v2 tip/core/rcu 10/14] rcu: Don't  redundantly disable irqs in rcu_irq_{enter,exit}()) "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-24 23:40 +0100

#1314194 — RCU lockup? (was: Re: [PATCH v2 tip/core/rcu 10/14] rcu: Don't redundantly disable irqs in rcu_irq_{enter,exit}())

Hi Paul,

On Thu, Dec 10, 2015 at 12:10 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> This commit replaces a local_irq_save()/local_irq_restore() pair with
> a lockdep assertion that interrupts are already disabled.  This should
> remove the corresponding overhead from the interrupt entry/exit fastpaths.
>
> This change was inspired by the fact that Iftekhar Ahmed's mutation
> testing showed that removing rcu_irq_enter()'s call to local_ird_restore()
> had no effect, which might indicate that interrupts were always enabled
> anyway.
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  include/linux/rcupdate.h   |  4 ++--
>  include/linux/rcutiny.h    |  8 ++++++++
>  include/linux/rcutree.h    |  2 ++
>  include/linux/tracepoint.h |  4 ++--
>  kernel/rcu/tree.c          | 32 ++++++++++++++++++++++++++------
>  5 files changed, 40 insertions(+), 10 deletions(-)

This commit (7c9906ca5e582a773fff696975e312cef58a7386) is triggering lock ups
during boot on r8a7791/koelsch (dual Cortex A15). Probably this commit does not
contain the real bug, but a symptom.

Unfortunately I cannot reproduce it with CONFIG_PROVE_RCU=y.

I started seeing the issue when disabling an innocent option in
shmobile_defconfig. I tracked it down to the removal of an unused C function,
containing hardware support for another system. Replacing the C function by
a dummy function with the right number of "asm("nop")"s (depending on kernel
version and/or kernel config, sigh) made the issue go away.
Adding or removing nops makes the issue reappear, and has some impact on
how early the issue happens (sometimes as late as early userspace).
Adding a multiple of 16 nops has no impact.
So it looks like something that should be cacheline-aligned isn't...

CONFIG_TREE_RCU=y

Do you have a suggestion?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [next] | [standalone]

#1314324

On Thu, Jan 21, 2016 at 02:22:56PM +0100, Geert Uytterhoeven wrote:
> Hi Paul,
> 
> On Thu, Dec 10, 2015 at 12:10 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > This commit replaces a local_irq_save()/local_irq_restore() pair with
> > a lockdep assertion that interrupts are already disabled.  This should
> > remove the corresponding overhead from the interrupt entry/exit fastpaths.
> >
> > This change was inspired by the fact that Iftekhar Ahmed's mutation
> > testing showed that removing rcu_irq_enter()'s call to local_ird_restore()
> > had no effect, which might indicate that interrupts were always enabled
> > anyway.
> >
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  include/linux/rcupdate.h   |  4 ++--
> >  include/linux/rcutiny.h    |  8 ++++++++
> >  include/linux/rcutree.h    |  2 ++
> >  include/linux/tracepoint.h |  4 ++--
> >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++++++++------
> >  5 files changed, 40 insertions(+), 10 deletions(-)
> 
> This commit (7c9906ca5e582a773fff696975e312cef58a7386) is triggering lock ups
> during boot on r8a7791/koelsch (dual Cortex A15). Probably this commit does not
> contain the real bug, but a symptom.

On the off-chance that it is related, here is Ding Tianhong's patch
that addressed some lockups:

http://www.eenyhelp.com/patch-rfc-locking-mutexes-dont-spin-owner-when-wait-list-not-null-help-215929641.html

Does that help in your case?

> Unfortunately I cannot reproduce it with CONFIG_PROVE_RCU=y.
> 
> I started seeing the issue when disabling an innocent option in
> shmobile_defconfig. I tracked it down to the removal of an unused C function,
> containing hardware support for another system. Replacing the C function by
> a dummy function with the right number of "asm("nop")"s (depending on kernel
> version and/or kernel config, sigh) made the issue go away.
> Adding or removing nops makes the issue reappear, and has some impact on
> how early the issue happens (sometimes as late as early userspace).
> Adding a multiple of 16 nops has no impact.
> So it looks like something that should be cacheline-aligned isn't...

The other possibility is that it is timing related.  Either way, fun
to find...

> CONFIG_TREE_RCU=y
> 
> Do you have a suggestion?

Only trying Ding's patch...

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1314835

Hi Paul,

On Thu, Jan 21, 2016 at 5:06 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Jan 21, 2016 at 02:22:56PM +0100, Geert Uytterhoeven wrote:
>> On Thu, Dec 10, 2015 at 12:10 AM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > This commit replaces a local_irq_save()/local_irq_restore() pair with
>> > a lockdep assertion that interrupts are already disabled.  This should
>> > remove the corresponding overhead from the interrupt entry/exit fastpaths.
>> >
>> > This change was inspired by the fact that Iftekhar Ahmed's mutation
>> > testing showed that removing rcu_irq_enter()'s call to local_ird_restore()
>> > had no effect, which might indicate that interrupts were always enabled
>> > anyway.
>> >
>> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> > ---
>> >  include/linux/rcupdate.h   |  4 ++--
>> >  include/linux/rcutiny.h    |  8 ++++++++
>> >  include/linux/rcutree.h    |  2 ++
>> >  include/linux/tracepoint.h |  4 ++--
>> >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++++++++------
>> >  5 files changed, 40 insertions(+), 10 deletions(-)
>>
>> This commit (7c9906ca5e582a773fff696975e312cef58a7386) is triggering lock ups
>> during boot on r8a7791/koelsch (dual Cortex A15). Probably this commit does not
>> contain the real bug, but a symptom.
>
> On the off-chance that it is related, here is Ding Tianhong's patch
> that addressed some lockups:
>
> http://www.eenyhelp.com/patch-rfc-locking-mutexes-dont-spin-owner-when-wait-list-not-null-help-215929641.html
>
> Does that help in your case?

Unfortunately not.

>> Unfortunately I cannot reproduce it with CONFIG_PROVE_RCU=y.
>>
>> I started seeing the issue when disabling an innocent option in
>> shmobile_defconfig. I tracked it down to the removal of an unused C function,
>> containing hardware support for another system. Replacing the C function by
>> a dummy function with the right number of "asm("nop")"s (depending on kernel
>> version and/or kernel config, sigh) made the issue go away.
>> Adding or removing nops makes the issue reappear, and has some impact on
>> how early the issue happens (sometimes as late as early userspace).
>> Adding a multiple of 16 nops has no impact.
>> So it looks like something that should be cacheline-aligned isn't...
>
> The other possibility is that it is timing related.  Either way, fun
> to find...
>
>> CONFIG_TREE_RCU=y
>>
>> Do you have a suggestion?
>
> Only trying Ding's patch...

Thanks for the pointer anyway!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1315268

On Fri, Jan 22, 2016 at 09:55:44AM +0100, Geert Uytterhoeven wrote:
> Hi Paul,
> 
> On Thu, Jan 21, 2016 at 5:06 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Thu, Jan 21, 2016 at 02:22:56PM +0100, Geert Uytterhoeven wrote:
> >> On Thu, Dec 10, 2015 at 12:10 AM, Paul E. McKenney
> >> <paulmck@linux.vnet.ibm.com> wrote:
> >> > This commit replaces a local_irq_save()/local_irq_restore() pair with
> >> > a lockdep assertion that interrupts are already disabled.  This should
> >> > remove the corresponding overhead from the interrupt entry/exit fastpaths.
> >> >
> >> > This change was inspired by the fact that Iftekhar Ahmed's mutation
> >> > testing showed that removing rcu_irq_enter()'s call to local_ird_restore()
> >> > had no effect, which might indicate that interrupts were always enabled
> >> > anyway.
> >> >
> >> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >> > ---
> >> >  include/linux/rcupdate.h   |  4 ++--
> >> >  include/linux/rcutiny.h    |  8 ++++++++
> >> >  include/linux/rcutree.h    |  2 ++
> >> >  include/linux/tracepoint.h |  4 ++--
> >> >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++++++++------
> >> >  5 files changed, 40 insertions(+), 10 deletions(-)
> >>
> >> This commit (7c9906ca5e582a773fff696975e312cef58a7386) is triggering lock ups
> >> during boot on r8a7791/koelsch (dual Cortex A15). Probably this commit does not
> >> contain the real bug, but a symptom.
> >
> > On the off-chance that it is related, here is Ding Tianhong's patch
> > that addressed some lockups:
> >
> > http://www.eenyhelp.com/patch-rfc-locking-mutexes-dont-spin-owner-when-wait-list-not-null-help-215929641.html
> >
> > Does that help in your case?
> 
> Unfortunately not.

We could revert the RCU patch without any real problems -- it is after
all just an optimization.

Hmmm...  One issue that we have seen before is that the irq-disabled
indication is a software flag that is not always in sync with
hardware conditions.  Might it be that we are hitting a situation where
irqs_disabled() is giving the wrong answer, thus suppressing the lockdep
warning?

								Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1315563

Hi Paul,

On Fri, Jan 22, 2016 at 9:44 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Fri, Jan 22, 2016 at 09:55:44AM +0100, Geert Uytterhoeven wrote:
>> On Thu, Jan 21, 2016 at 5:06 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Thu, Jan 21, 2016 at 02:22:56PM +0100, Geert Uytterhoeven wrote:
>> >> On Thu, Dec 10, 2015 at 12:10 AM, Paul E. McKenney
>> >> <paulmck@linux.vnet.ibm.com> wrote:
>> >> > This commit replaces a local_irq_save()/local_irq_restore() pair with
>> >> > a lockdep assertion that interrupts are already disabled.  This should
>> >> > remove the corresponding overhead from the interrupt entry/exit fastpaths.
>> >> >
>> >> > This change was inspired by the fact that Iftekhar Ahmed's mutation
>> >> > testing showed that removing rcu_irq_enter()'s call to local_ird_restore()
>> >> > had no effect, which might indicate that interrupts were always enabled
>> >> > anyway.
>> >> >
>> >> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> >> > ---
>> >> >  include/linux/rcupdate.h   |  4 ++--
>> >> >  include/linux/rcutiny.h    |  8 ++++++++
>> >> >  include/linux/rcutree.h    |  2 ++
>> >> >  include/linux/tracepoint.h |  4 ++--
>> >> >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++++++++------
>> >> >  5 files changed, 40 insertions(+), 10 deletions(-)
>> >>
>> >> This commit (7c9906ca5e582a773fff696975e312cef58a7386) is triggering lock ups
>> >> during boot on r8a7791/koelsch (dual Cortex A15). Probably this commit does not
>> >> contain the real bug, but a symptom.
>> >
>> > On the off-chance that it is related, here is Ding Tianhong's patch
>> > that addressed some lockups:
>> >
>> > http://www.eenyhelp.com/patch-rfc-locking-mutexes-dont-spin-owner-when-wait-list-not-null-help-215929641.html
>> >
>> > Does that help in your case?
>>
>> Unfortunately not.
>
> We could revert the RCU patch without any real problems -- it is after
> all just an optimization.

I replaced the calls to rcu_irq_{enter,exit}() in irq_{enter,exit}() by their
_irqson counterparts, which should be equivalent to the old code, but the issue
persisted. Strange...

Does it matter that arm has
#define __ARCH_IRQ_EXIT_IRQS_DISABLED   1
?

I tried JTAG, but enabling JTAG on r8a7791/koelsch requires changing a switch
on the board, which also disables the second CPU core, and thus makes the issue
disappear...

> Hmmm...  One issue that we have seen before is that the irq-disabled
> indication is a software flag that is not always in sync with
> hardware conditions.  Might it be that we are hitting a situation where
> irqs_disabled() is giving the wrong answer, thus suppressing the lockdep
> warning?

Possible. I tried adding 'if(!irqs_disabled) printk("something")' just before
the RCU_LOCKDEP_WARN(), but it never triggered. Worse, the issue went away by
doing that :-(

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1316105

On Sat, Jan 23, 2016 at 10:43:19AM +0100, Geert Uytterhoeven wrote:
> Hi Paul,
> 
> On Fri, Jan 22, 2016 at 9:44 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Fri, Jan 22, 2016 at 09:55:44AM +0100, Geert Uytterhoeven wrote:
> >> On Thu, Jan 21, 2016 at 5:06 PM, Paul E. McKenney
> >> <paulmck@linux.vnet.ibm.com> wrote:
> >> > On Thu, Jan 21, 2016 at 02:22:56PM +0100, Geert Uytterhoeven wrote:
> >> >> On Thu, Dec 10, 2015 at 12:10 AM, Paul E. McKenney
> >> >> <paulmck@linux.vnet.ibm.com> wrote:
> >> >> > This commit replaces a local_irq_save()/local_irq_restore() pair with
> >> >> > a lockdep assertion that interrupts are already disabled.  This should
> >> >> > remove the corresponding overhead from the interrupt entry/exit fastpaths.
> >> >> >
> >> >> > This change was inspired by the fact that Iftekhar Ahmed's mutation
> >> >> > testing showed that removing rcu_irq_enter()'s call to local_ird_restore()
> >> >> > had no effect, which might indicate that interrupts were always enabled
> >> >> > anyway.
> >> >> >
> >> >> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >> >> > ---
> >> >> >  include/linux/rcupdate.h   |  4 ++--
> >> >> >  include/linux/rcutiny.h    |  8 ++++++++
> >> >> >  include/linux/rcutree.h    |  2 ++
> >> >> >  include/linux/tracepoint.h |  4 ++--
> >> >> >  kernel/rcu/tree.c          | 32 ++++++++++++++++++++++++++------
> >> >> >  5 files changed, 40 insertions(+), 10 deletions(-)
> >> >>
> >> >> This commit (7c9906ca5e582a773fff696975e312cef58a7386) is triggering lock ups
> >> >> during boot on r8a7791/koelsch (dual Cortex A15). Probably this commit does not
> >> >> contain the real bug, but a symptom.
> >> >
> >> > On the off-chance that it is related, here is Ding Tianhong's patch
> >> > that addressed some lockups:
> >> >
> >> > http://www.eenyhelp.com/patch-rfc-locking-mutexes-dont-spin-owner-when-wait-list-not-null-help-215929641.html
> >> >
> >> > Does that help in your case?
> >>
> >> Unfortunately not.
> >
> > We could revert the RCU patch without any real problems -- it is after
> > all just an optimization.
> 
> I replaced the calls to rcu_irq_{enter,exit}() in irq_{enter,exit}() by their
> _irqson counterparts, which should be equivalent to the old code, but the issue
> persisted. Strange...

Indeed...

> Does it matter that arm has
> #define __ARCH_IRQ_EXIT_IRQS_DISABLED   1
> ?

No idea.  I added Arnd and Olof on CC in case they can tell us more.

> I tried JTAG, but enabling JTAG on r8a7791/koelsch requires changing a switch
> on the board, which also disables the second CPU core, and thus makes the issue
> disappear...

:-(

> > Hmmm...  One issue that we have seen before is that the irq-disabled
> > indication is a software flag that is not always in sync with
> > hardware conditions.  Might it be that we are hitting a situation where
> > irqs_disabled() is giving the wrong answer, thus suppressing the lockdep
> > warning?
> 
> Possible. I tried adding 'if(!irqs_disabled) printk("something")' just before
> the RCU_LOCKDEP_WARN(), but it never triggered. Worse, the issue went away by
> doing that :-(

That would be "if (!irqs_disabled())..." with the "()", correct?

But if you had lockdep enabled, and if lockdep didn't complain, I would
not expect the "if" to complain either.  The fact that the problem was
suppressed by the extra check is a bit annoying, I will grant you that!

							Thanx, Paul

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web