[PATCH V2 1/2] asm-offsets.c: adding #define to break circular dependency

[PATCH V2 1/2] asm-offsets.c: adding #define to break circular dependency

[Jim Quinlan]

irqflags.h depends on asm-offsets.h, but asm-offsets.h depends
on irqflags.h when generating asm-offsets.h.  Adding a definition
to the top of asm-offsets.c allows us to break this circle.  There
is a similar define in bounds.c

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 arch/mips/kernel/asm-offsets.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/mips/kernel/asm-offsets.c b/arch/mips/kernel/asm-offsets.c
index 6b30fb2..035f167 100644
--- a/arch/mips/kernel/asm-offsets.c
+++ b/arch/mips/kernel/asm-offsets.c
@@ -8,6 +8,7 @@
  * Kevin Kissell, kevink@mips.com and Carsten Langgaard, carstenl@mips.com
  * Copyright (C) 2000 MIPS Technologies, Inc.
  */
+#define __GENERATING_OFFSETS_S
 #include <linux/compat.h>
 #include <linux/types.h>
 #include <linux/sched.h>
-- 
1.7.6

[PATCH V2 2/2] MIPS: irqflags.h: make funcs preempt-safe for non-mipsr2

[Jim Quinlan]

For non MIPSr2 processors, such as the BMIPS 5000, calls to
arch_local_irq_disable() and others may be preempted, and in doing
so a stale value may be restored to c0_status.  This fix disables
preemption for such processors prior to the call and enables it
after the call.

This bug was observed in a BMIPS 5000, occuring once every few hours
in a continuous reboot test.  It was traced to the write_lock_irq()
function which was being invoked in release_task() in exit.c.
By placing a number of "nops" inbetween the mfc0/mtc0 pair in
arch_local_irq_disable(), which is called by write_lock_irq(), we
were able to greatly increase the occurance of this bug.  Similarly,
the application of this commit silenced the bug.

It is better to use the preemption functions declared in <linux/preempt.h>
rather than defining new ones as is done in this commit.  However,
including that file from irqflags effected many compiler errors.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 arch/mips/include/asm/irqflags.h |   81 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/arch/mips/include/asm/irqflags.h b/arch/mips/include/asm/irqflags.h
index 309cbcd..d6e71ed 100644
--- a/arch/mips/include/asm/irqflags.h
+++ b/arch/mips/include/asm/irqflags.h
@@ -16,6 +16,71 @@
 #include <linux/compiler.h>
 #include <asm/hazards.h>
 
+#if defined(__GENERATING_BOUNDS_H) || defined(__GENERATING_OFFSETS_S)
+#define __TI_PRE_COUNT (-1)
+#else
+#include <asm/asm-offsets.h>
+#define __TI_PRE_COUNT TI_PRE_COUNT
+#endif
+
+
+/*
+ * Non-mipsr2 processors executing functions such as arch_local_irq_disable()
+ * are not preempt-safe: if preemption occurs between the mfc0 and the mtc0,
+ * a stale status value may be stored.  To prevent this, we define
+ * here arch_local_preempt_disable() and arch_local_preempt_enable(), which
+ * are called before the mfc0 and after the mtc0, respectively.  A better
+ * solution would "#include <linux/preempt.h> and use its declared routines,
+ * but that is not viable due to numerous compile errors.
+ *
+ * MipsR2 processors with atomic interrupt enable/disable instructions
+ * (ei/di) do not have this issue.
+ */
+__asm__(
+	"	.macro	arch_local_preempt_disable ti_pre_count		\n"
+	"	.set	push						\n"
+	"	.set	noat						\n"
+	"	lw	$1, \\ti_pre_count($28)				\n"
+	"	addi	$1, $1, 1					\n"
+	"	sw	$1, \\ti_pre_count($28)				\n"
+	"	.set	pop						\n"
+	"	.endm");
+static inline void arch_local_preempt_disable(void)
+{
+#if defined(CONFIG_PREEMPT) && !defined(CONFIG_CPU_MIPSR2)
+	__asm__ __volatile__(
+		"arch_local_preempt_disable\t%0"
+		: /* no outputs */
+		: "n" (__TI_PRE_COUNT)
+		: "memory");
+	barrier();
+#endif
+}
+
+
+__asm__(
+	"	.macro	arch_local_preempt_enable ti_pre_count		\n"
+	"	.set	push						\n"
+	"	.set	noat						\n"
+	"	lw	$1, \\ti_pre_count($28)				\n"
+	"	addi	$1, $1, -1					\n"
+	"	sw	$1, \\ti_pre_count($28)				\n"
+	"	.set	pop						\n"
+	"	.endm");
+
+static inline void arch_local_preempt_enable(void)
+{
+#if defined(CONFIG_PREEMPT) && !defined(CONFIG_CPU_MIPSR2)
+	__asm__ __volatile__(
+		"arch_local_preempt_enable\t%0"
+		: /* no outputs */
+		: "n" (__TI_PRE_COUNT)
+		: "memory");
+	barrier();
+#endif
+}
+
+
 __asm__(
 	"	.macro	arch_local_irq_enable				\n"
 	"	.set	push						\n"
@@ -99,11 +164,15 @@ __asm__(
 
 static inline void arch_local_irq_disable(void)
 {
+	arch_local_preempt_disable();
+
 	__asm__ __volatile__(
 		"arch_local_irq_disable"
 		: /* no outputs */
 		: /* no inputs */
 		: "memory");
+
+	arch_local_preempt_enable();
 }
 
 __asm__(
@@ -153,10 +222,15 @@ __asm__(
 static inline unsigned long arch_local_irq_save(void)
 {
 	unsigned long flags;
+
+	arch_local_preempt_disable();
+
 	asm volatile("arch_local_irq_save\t%0"
 		     : "=r" (flags)
 		     : /* no inputs */
 		     : "memory");
+
+	arch_local_preempt_enable();
 	return flags;
 }
 
@@ -214,23 +288,30 @@ static inline void arch_local_irq_restore(unsigned long flags)
 	if (unlikely(!(flags & 0x0400)))
 		smtc_ipi_replay();
 #endif
+	arch_local_preempt_disable();
 
 	__asm__ __volatile__(
 		"arch_local_irq_restore\t%0"
 		: "=r" (__tmp1)
 		: "0" (flags)
 		: "memory");
+
+	arch_local_preempt_enable();
 }
 
 static inline void __arch_local_irq_restore(unsigned long flags)
 {
 	unsigned long __tmp1;
 
+	arch_local_preempt_disable();
+
 	__asm__ __volatile__(
 		"arch_local_irq_restore\t%0"
 		: "=r" (__tmp1)
 		: "0" (flags)
 		: "memory");
+
+	arch_local_preempt_enable();
 }
 
 static inline int arch_irqs_disabled_flags(unsigned long flags)
-- 
1.7.6

Re: [PATCH V2 1/2] asm-offsets.c: adding #define to break circular dependency

[David Daney]

On 08/29/2012 03:34 PM, Jim Quinlan wrote:
> irqflags.h depends on asm-offsets.h, but asm-offsets.h depends
> on irqflags.h when generating asm-offsets.h.

What is there in irqflags.h that is required by asm-offsets.c?

Why can't the include tangle be undone so that that part can be factored 
out to a separate file?



> Adding a definition
> to the top of asm-offsets.c allows us to break this circle.  There
> is a similar define in bounds.c
>
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>   arch/mips/kernel/asm-offsets.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/kernel/asm-offsets.c b/arch/mips/kernel/asm-offsets.c
> index 6b30fb2..035f167 100644
> --- a/arch/mips/kernel/asm-offsets.c
> +++ b/arch/mips/kernel/asm-offsets.c
> @@ -8,6 +8,7 @@
>    * Kevin Kissell, kevink@mips.com and Carsten Langgaard, carstenl@mips.com
>    * Copyright (C) 2000 MIPS Technologies, Inc.
>    */
> +#define __GENERATING_OFFSETS_S
>   #include <linux/compat.h>
>   #include <linux/types.h>
>   #include <linux/sched.h>
>

Re: [PATCH V2 1/2] asm-offsets.c: adding #define to break circular dependency

[Jim Quinlan]

I'm not sure the tangle is so easily undone.  The first dependency I see is

asm-offsets.c
asm/processors.h
linux/cpumask.h
linux/kernel.h
linux/bitops.h
asm/bitops.h
linux/irqflags.h
asm/irqflags.h

When compared to other architectures, the MIPS asm/bitops.h seems to
include more files at the top, including linux/irqflags.h.
Any suggestions?

On Wed, Aug 29, 2012 at 7:01 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> On 08/29/2012 03:34 PM, Jim Quinlan wrote:
>>
>> irqflags.h depends on asm-offsets.h, but asm-offsets.h depends
>> on irqflags.h when generating asm-offsets.h.
>
>
> What is there in irqflags.h that is required by asm-offsets.c?
>
> Why can't the include tangle be undone so that that part can be factored out
> to a separate file?
>
>
>
>
>> Adding a definition
>> to the top of asm-offsets.c allows us to break this circle.  There
>> is a similar define in bounds.c
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> ---
>>   arch/mips/kernel/asm-offsets.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/mips/kernel/asm-offsets.c
>> b/arch/mips/kernel/asm-offsets.c
>> index 6b30fb2..035f167 100644
>> --- a/arch/mips/kernel/asm-offsets.c
>> +++ b/arch/mips/kernel/asm-offsets.c
>> @@ -8,6 +8,7 @@
>>    * Kevin Kissell, kevink@mips.com and Carsten Langgaard,
>> carstenl@mips.com
>>    * Copyright (C) 2000 MIPS Technologies, Inc.
>>    */
>> +#define __GENERATING_OFFSETS_S
>>   #include <linux/compat.h>
>>   #include <linux/types.h>
>>   #include <linux/sched.h>
>>
>

Re: [PATCH V2 1/2] asm-offsets.c: adding #define to break circular dependency

[Ralf Baechle]

On Thu, Aug 30, 2012 at 10:06:30AM -0400, Jim Quinlan wrote:

> I'm not sure the tangle is so easily undone.  The first dependency I see is
> 
> asm-offsets.c
> asm/processors.h
> linux/cpumask.h
> linux/kernel.h
> linux/bitops.h
> asm/bitops.h
> linux/irqflags.h
> asm/irqflags.h
> 
> When compared to other architectures, the MIPS asm/bitops.h seems to
> include more files at the top, including linux/irqflags.h.
> Any suggestions?

This is because MIPS bitops for some ancient processors which don't have
atomic operations and Cavium cnMIPS cores where disabling interrupts is
faster than the atomic operations are implemented by disabling interrupts.

This makes these atomic operations relativly bloated in terms of code size
generated and may it'd be a good idea to outline the bits.  With a bit
of luck we even get better cache locality - and fewer header file
inclusions.

  Ralf

Re: [PATCH V2 2/2] MIPS: irqflags.h: make funcs preempt-safe for non-mipsr2

[David Daney]

On 08/29/2012 03:34 PM, Jim Quinlan wrote:
> For non MIPSr2 processors, such as the BMIPS 5000, calls to
> arch_local_irq_disable() and others may be preempted, and in doing
> so a stale value may be restored to c0_status.  This fix disables
> preemption for such processors prior to the call and enables it
> after the call.
>
> This bug was observed in a BMIPS 5000, occuring once every few hours
> in a continuous reboot test.  It was traced to the write_lock_irq()
> function which was being invoked in release_task() in exit.c.
> By placing a number of "nops" inbetween the mfc0/mtc0 pair in
> arch_local_irq_disable(), which is called by write_lock_irq(), we
> were able to greatly increase the occurance of this bug.  Similarly,
> the application of this commit silenced the bug.
>
> It is better to use the preemption functions declared in <linux/preempt.h>
> rather than defining new ones as is done in this commit.  However,
> including that file from irqflags effected many compiler errors.
>
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>   arch/mips/include/asm/irqflags.h |   81 ++++++++++++++++++++++++++++++++++++++
>   1 files changed, 81 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/include/asm/irqflags.h b/arch/mips/include/asm/irqflags.h
> index 309cbcd..d6e71ed 100644
> --- a/arch/mips/include/asm/irqflags.h
> +++ b/arch/mips/include/asm/irqflags.h
> @@ -16,6 +16,71 @@
>   #include <linux/compiler.h>
>   #include <asm/hazards.h>
>
> +#if defined(__GENERATING_BOUNDS_H) || defined(__GENERATING_OFFSETS_S)
> +#define __TI_PRE_COUNT (-1)
> +#else
> +#include <asm/asm-offsets.h>
> +#define __TI_PRE_COUNT TI_PRE_COUNT
> +#endif
> +
> +
> +/*
> + * Non-mipsr2 processors executing functions such as arch_local_irq_disable()
> + * are not preempt-safe: if preemption occurs between the mfc0 and the mtc0,
> + * a stale status value may be stored.  To prevent this, we define
> + * here arch_local_preempt_disable() and arch_local_preempt_enable(), which
> + * are called before the mfc0 and after the mtc0, respectively.  A better
> + * solution would "#include <linux/preempt.h> and use its declared routines,
> + * but that is not viable due to numerous compile errors.
> + *

I'm with Ralf's idea from the other branch of the thread.  Put all this 
non-mipsr2 stuff out of line (perhaps creating lib/mips-atomic.c).


> + * MipsR2 processors with atomic interrupt enable/disable instructions
> + * (ei/di) do not have this issue.
> + */

For mipsr2, we leave them alone so they can be inlined.

This way you shouldn't need the ugly #include hackery.

David Daney

Re: [PATCH V2 1/2] asm-offsets.c: adding #define to break circular dependency

[Jim Quinlan]

Are you saying you want me to outline most of the functions in
asm/bitops.h , or do you want me somehow outline just the "else"
clause that invokes raw_local_irq_{save_restore}?

Jim

On Thu, Aug 30, 2012 at 10:15 AM, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Thu, Aug 30, 2012 at 10:06:30AM -0400, Jim Quinlan wrote:
>
>> I'm not sure the tangle is so easily undone.  The first dependency I see is
>>
>> asm-offsets.c
>> asm/processors.h
>> linux/cpumask.h
>> linux/kernel.h
>> linux/bitops.h
>> asm/bitops.h
>> linux/irqflags.h
>> asm/irqflags.h
>>
>> When compared to other architectures, the MIPS asm/bitops.h seems to
>> include more files at the top, including linux/irqflags.h.
>> Any suggestions?
>
> This is because MIPS bitops for some ancient processors which don't have
> atomic operations and Cavium cnMIPS cores where disabling interrupts is
> faster than the atomic operations are implemented by disabling interrupts.
>
> This makes these atomic operations relativly bloated in terms of code size
> generated and may it'd be a good idea to outline the bits.  With a bit
> of luck we even get better cache locality - and fewer header file
> inclusions.
>
>   Ralf