imx6q restart is broken

Commit Message

Shawn Guo Aug. 9, 2012, 12:01 p.m. UTC

On Thu, Aug 09, 2012 at 10:20:21AM +0100, Russell King - ARM Linux wrote:
> I suspect having this dmb inside cpu_relax() is flooding the
> interconnects with traffic, which then prevents other CPUs getting
> a look-in (maybe there's no fairness when it comes to dmb's.
> 
> If I'm right, you'll find is that even converting this to the ARMv7
> DMB instruction won't fix the problem.  It does, however, point
> towards a more serious problem - it means that any tight loop using
> dmb is detremental.  I have heard some people mention that even on
> various ARM SMP platforms, they have see quite an amount of
> interaction between the individual CPU cores, and I'm beginning
> to wonder whether this is why.
> 
> I think a useful test would be to only execute the DMB maybe once
> in 50 or 100 loops - the DMB is there to work around a different
> problem with the temporal locality of stores on the local CPU.  So,
> the only requirement is that we issue a DMB at some point while
> spinning waiting for another CPU to respond to our previous writes.

You got it.

The following change fixed the problem for me.  But I think you only
meant it for testing.  So how should we actually fix the problem?

Regards,
Shawn

--8<---

linux-arm-kernel mailing list
[e-mail]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Shawn Guo Aug. 9, 2012, 12:22 p.m. UTC | #1

On Thu, Aug 09, 2012 at 08:01:47PM +0800, Shawn Guo wrote:
> diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
> index 99afa74..1282b61 100644
> --- a/arch/arm/include/asm/processor.h
> +++ b/arch/arm/include/asm/processor.h
> @@ -80,11 +80,18 @@ extern void release_thread(struct task_struct *);
>  unsigned long get_wchan(struct task_struct *p);
> 
>  #if __LINUX_ARM_ARCH__ == 6 || defined(CONFIG_ARM_ERRATA_754327)
> -#define cpu_relax()                    smp_mb()
> +#define __cpu_relax()                  smp_mb()
>  #else
> -#define cpu_relax()                    barrier()
> +#define __cpu_relax()                  barrier()
>  #endif
> 
> +#define cpu_relax()                                                    \
> +({                                                                     \
> +       static int i;                                                   \
> +       if (i++ % 100)                                                  \

This should be 

	if (i++ % 100 == 0)

But either way it fixes the problem.

> +               __cpu_relax();                                          \
> +})
> +
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Russell King - ARM Linux Aug. 9, 2012, 1:57 p.m. UTC | #2

On Thu, Aug 09, 2012 at 08:22:22PM +0800, Shawn Guo wrote:
> On Thu, Aug 09, 2012 at 08:01:47PM +0800, Shawn Guo wrote:
> > diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
> > index 99afa74..1282b61 100644
> > --- a/arch/arm/include/asm/processor.h
> > +++ b/arch/arm/include/asm/processor.h
> > @@ -80,11 +80,18 @@ extern void release_thread(struct task_struct *);
> >  unsigned long get_wchan(struct task_struct *p);
> > 
> >  #if __LINUX_ARM_ARCH__ == 6 || defined(CONFIG_ARM_ERRATA_754327)
> > -#define cpu_relax()                    smp_mb()
> > +#define __cpu_relax()                  smp_mb()
> >  #else
> > -#define cpu_relax()                    barrier()
> > +#define __cpu_relax()                  barrier()
> >  #endif
> > 
> > +#define cpu_relax()                                                    \
> > +({                                                                     \
> > +       static int i;                                                   \
> > +       if (i++ % 100)                                                  \
> 
> This should be 
> 
> 	if (i++ % 100 == 0)
> 
> But either way it fixes the problem.

Right, so it does sound like issuing dmb instructions in a tight loop
can bring a system to its knees.

While we can address it in the above way in the kernel, it points to
a much bigger problem - userspace can issue dmb instructions itself,
which means any user program can effectively take a system down.  Can
you confirm this by building and running the following program:

int main()
{
	while (1)
		asm("dmb");
}

Then try running some other programs.  If I'm correct, this should
result in the system becoming rather unresponsive.

Thanks.

Russell King - ARM Linux Aug. 9, 2012, 2:01 p.m. UTC | #3

On Thu, Aug 09, 2012 at 02:57:55PM +0100, Russell King - ARM Linux wrote:
> Then try running some other programs.  If I'm correct, this should
> result in the system becoming rather unresponsive.

I should better describe what I mean by "unresponsive" - it won't lock
the system up (because the dmb loop will be interrupted from time to
time) but it should make it rather sluggish to deal with page faults on
other CPUs.

Shawn Guo Aug. 9, 2012, 2:24 p.m. UTC | #4

On Thu, Aug 09, 2012 at 03:01:37PM +0100, Russell King - ARM Linux wrote:
> On Thu, Aug 09, 2012 at 02:57:55PM +0100, Russell King - ARM Linux wrote:
> > Then try running some other programs.  If I'm correct, this should
> > result in the system becoming rather unresponsive.
> 
> I should better describe what I mean by "unresponsive" - it won't lock
> the system up (because the dmb loop will be interrupted from time to
> time) but it should make it rather sluggish to deal with page faults on
> other CPUs.

You are right.  I'm seeing exactly the "unresponsive" you describe here
after launching that little program.

Patch

diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 99afa74..1282b61 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -80,11 +80,18 @@  extern void release_thread(struct task_struct *);
 unsigned long get_wchan(struct task_struct *p);

 #if __LINUX_ARM_ARCH__ == 6 || defined(CONFIG_ARM_ERRATA_754327)
-#define cpu_relax()                    smp_mb()
+#define __cpu_relax()                  smp_mb()
 #else
-#define cpu_relax()                    barrier()
+#define __cpu_relax()                  barrier()
 #endif

+#define cpu_relax()                                                    \
+({                                                                     \
+       static int i;                                                   \
+       if (i++ % 100)                                                  \
+               __cpu_relax();                                          \
+})
+


_______________________________________________