Skip to contentSkip to conversation

Fix a longstanding ARM RT bug

PATCH In review Carsten Emde 2 participants 4 messages Jun 4, 2013 – Jun 5, 2013 Arm (32-bit)

lore
Share & tools
lore

Carsten Emde

author
#0 lore raw

cover Fix a longstanding ARM RT bug

The reward of nearly six months of debugging: At least one of the ARM test systems no longer crashes.

-Carsten.

author
#1 lore raw

PATCH 1/1 ARM mm: Fix RT life lock on ASID rollover

The original mechanism to synchronize all online CPUs after ASID reallocation used an IPI mechanism with IRQs enabled. This is a valid mechanism in mainline. An RT kernel, however, may hang forever due to a life lock between sending the IPI and waiting for the ASID lock to be freed. Such hangers were observed and analyzed using JTAG hardware debugging on an OMAP4430 board. Mean uptime was about two days with a maximum of seven days observed once.

In 2012, Will Deacon provided a new ASID rollover synchronization mechanism without IPI broadcasting. This *improved* a suboptimal implementation in mainline - but it *fixed* a disastrous bug in RT kernels that was extremely hard to decode.

After applying this backport of Will's original patch that appeared in 3.8, the test system no longer crashes; current uptime is 17 days.

Note that this patch applies to the most recent 3.0 RT kernel 3.0.80-rt108. The RT kernels 3.2, 3.4 and 3.6 need to be patched as well in a similar way. From 3.8 onwards, RT kernels are ok.

Original commits:

37f47e3d62533c931b04cb409f2eb299e6342331
ARM: 7658/1: mm: fix race updating mm->context.id on ASID rollover

bf51bb82ccd9a74e9702d06107b23e54b27a5707
ARM: mm: use bitmap operations when allocating new ASIDs

4b883160835faf38c9356f0885cf491a1e661e88
ARM: mm: avoid taking ASID spinlock on fastpath

b5466f8728527a05a493cc4abe9e6f034a1bbaab
ARM: mm: remove IPI broadcasting on ASID rollover

ASIDs are allocated to MMU contexts based on a rolling counter. This means that after 255 allocations we must invalidate all existing ASIDs via an expensive IPI mechanism to synchronise all of the online CPUs and ensure that all tasks execute with an ASID from the new generation.

This patch changes the rollover behaviour so that we rely instead on the hardware broadcasting of the TLB invalidation to avoid the IPI calls. This works by keeping track of the active ASID on each core, which is then reserved in the case of a rollover so that currently scheduled tasks can continue to run. For cores without hardware TLB broadcasting, we keep track of pending flushes in a cpumask, so cores can flush their local TLB before scheduling a new mm.

Signed-off-by Will Deacon <[email protected]>
Signed-off-by Carsten Emde <[email protected]>
---
 arch/arm/include/asm/mmu.h         |   13 -
 arch/arm/include/asm/mmu_context.h |   83 ++++------
 arch/arm/include/asm/tlbflush.h    |  263 ++++++++++++++--------------------
 arch/arm/mm/alignment.c            |    1 
 arch/arm/mm/context.c              |  284 +++++++++++++++++++++++--------------
 5 files changed, 324 insertions(+), 320 deletions(-)
arch/arm/include/asm/mmu.h +5 -8
Index: linux-3.0.80-rt108/arch/arm/include/asm/mmu.h
===================================================================
@@ -5,18 +5,15 @@
 
 typedef struct {
 #ifdef CONFIG_CPU_HAS_ASID
-	unsigned int id;
-	raw_spinlock_t id_lock;
+	atomic64_t	id;
 #endif
-	unsigned int kvm_seq;
+	unsigned int	kvm_seq;
 } mm_context_t;
 
 #ifdef CONFIG_CPU_HAS_ASID
-#define ASID(mm)	((mm)->context.id & 255)
-
-/* init_mm.context.id_lock should be initialized. */
-#define INIT_MM_CONTEXT(name)                                                 \
-	.context.id_lock    = __RAW_SPIN_LOCK_UNLOCKED(name.context.id_lock),
+#define ASID_BITS	8
+#define ASID_MASK	((~0ULL) << ASID_BITS)
+#define ASID(mm)	((mm)->context.id.counter & ~ASID_MASK)
 #else
 #define ASID(mm)	(0)
 #endif
arch/arm/include/asm/mmu_context.h +32 -51

110 lines not on this page · raw message

arch/arm/include/asm/tlbflush.h +109 -154

419 lines not on this page · raw message

arch/arm/mm/alignment.c +1 -0
Index: linux-3.0.80-rt108/arch/arm/mm/alignment.c
===================================================================
@@ -819,6 +819,7 @@ do_alignment(unsigned long addr, unsigne
 		break;
 
 	case 0x08000000:	/* ldm or stm, or thumb-2 32bit instruction */
+		offset.un = 0;
 		if (thumb2_32b)
 			handler = do_alignment_t32_to_handler(&instr, regs, &offset);
 		else
arch/arm/mm/context.c +177 -107

327 lines not on this page · raw message

#2 lore raw

Re: [PATCH 1/1] ARM mm: Fix RT life lock on ASID rollover

Hi Carsten,

Quoted 8 lines · Carsten Emde

The original mechanism to synchronize all online CPUs after ASID reallocation used an IPI mechanism with IRQs enabled. This is a valid mechanism in mainline. An RT kernel, however, may hang forever due to a life lock between sending the IPI and waiting for the ASID lock to be freed. Such hangers were observed and analyzed using JTAG hardware debugging on an OMAP4430 board. Mean uptime was about two days with a maximum of seven days observed once.

Quoted 4 lines

In 2012, Will Deacon provided a new ASID rollover synchronization mechanism without IPI broadcasting. This *improved* a suboptimal implementation in mainline - but it *fixed* a disastrous bug in RT kernels that was extremely hard to decode.

Ha, that's a nice and unanticipated side-effect :)

Quoted 5 lines

Original commits:

37f47e3d62533c931b04cb409f2eb299e6342331
ARM: 7658/1: mm: fix race updating mm->context.id on ASID rollover

Quoted 8 lines

bf51bb82ccd9a74e9702d06107b23e54b27a5707
ARM: mm: use bitmap operations when allocating new ASIDs

4b883160835faf38c9356f0885cf491a1e661e88
ARM: mm: avoid taking ASID spinlock on fastpath

b5466f8728527a05a493cc4abe9e6f034a1bbaab
ARM: mm: remove IPI broadcasting on ASID rollover

You seem to have a few extra bits and pieces in here, which you might not care about:

Quoted 21 lines

Index: linux-3.0.80-rt108/arch/arm/include/asm/tlbflush.h
===================================================================

arch/arm/include/asm/tlbflush.h +0 -2
@@ -14,7 +14,6 @@
 
 #include <asm/glue.h>
 
-#define TLB_V3_PAGE    (1 << 0)
 #define TLB_V4_U_PAGE  (1 << 1)
 #define TLB_V4_D_PAGE  (1 << 2)
 #define TLB_V4_I_PAGE  (1 << 3)

@@ -22,7 +21,6 @@
 #define TLB_V6_D_PAGE  (1 << 5)
 #define TLB_V6_I_PAGE  (1 << 6)
 
-#define TLB_V3_FULL    (1 << 8)
 #define TLB_V4_U_FULL  (1 << 9)
 #define TLB_V4_D_FULL  (1 << 10)
 #define TLB_V4_I_FULL  (1 << 11)

arch/arm/include/asm/tlbflush.h line 35

Quoted 5 lines
arch/arm/include/asm/tlbflush.h +1 -1
@@ -34,16 +32,15 @@
 #define TLB_V6_D_ASID  (1 << 17)
 #define TLB_V6_I_ASID  (1 << 18)
 
-#define TLB_BTB                (1 << 28)
+#define TLB_V6_BP      (1 << 19)

This hunk (and related ones) are from a patch adding branch predictor maintenance that I also wrote. It's harmless, but you likely don't need it.

Quoted 4 lines
arch/arm/include/asm/tlbflush.h +4 -0
+#ifdef CONFIG_ARM_ERRATA_798181
+static inline void dummy_flush_tlb_a15_erratum(void)
+{
+       /*

arch/arm/include/asm/tlbflush.h static inline void local_flush_tlb_kerne · line 459

Quoted 10 lines
arch/arm/include/asm/tlbflush.h +10 -0
+        * Dummy TLBIMVAIS. Using the unmapped address 0 and ASID 0.
+        */
+       asm("mcr p15, 0, %0, c8, c3, 1" : : "r" (0));
+       dsb();
+}
+#else
+static inline void dummy_flush_tlb_a15_erratum(void)
+{
+}
+#endif

And this is an A15 erratum workaround from Catalin. Actually, the original version of that workaround didn't interact nicely with PREEMPT kernels, so you should double-check what you've got (it was fixed recently in mainline).

Furthermore, the workaround requires IPIs on TLB invalidation, so you might have your livelock problem again...

arch/arm/mm/alignment.c do_alignment(unsigned long addr, unsigne · line 825

Quoted 13 lines

/*
Index: linux-3.0.80-rt108/arch/arm/mm/alignment.c
===================================================================

arch/arm/mm/alignment.c +1 -0
@@ -819,6 +819,7 @@ do_alignment(unsigned long addr, unsigne
                break;
 
        case 0x08000000:        /* ldm or stm, or thumb-2 32bit instruction */
+               offset.un = 0;
                if (thumb2_32b)
                        handler = do_alignment_t32_to_handler(&instr, regs, &offset);
                else

Unrelated?

Will

author
#3 lore raw

Re: [PATCH 1/1] ARM mm: Fix RT life lock on ASID rollover

Hi Will,

Quoted 8 lines
Quoted 8 lines

The original mechanism to synchronize all online CPUs after ASID reallocation used an IPI mechanism with IRQs enabled. This is a valid mechanism in mainline. An RT kernel, however, may hang forever due to a life lock between sending the IPI and waiting for the ASID lock to be freed. Such hangers were observed and analyzed using JTAG hardware debugging on an OMAP4430 board. Mean uptime was about two days with a maximum of seven days observed once.

Quoted 6 lines
Quoted 4 lines

In 2012, Will Deacon provided a new ASID rollover synchronization mechanism without IPI broadcasting. This *improved* a suboptimal implementation in mainline - but it *fixed* a disastrous bug in RT kernels that was extremely hard to decode.

Ha, that's a nice and unanticipated side-effect :)

Yeah, thanks a lot for fixing it.

Quoted 3 lines

[..]
You seem to have a few extra bits and pieces in here, which you might not care about:

Was a long way from current mainline back to a 3.0 vendor kernel mess - so I used a shortcut that obviously picked up some more code than needed.

Quoted 24 lines
Quoted 24 lines

Index: linux-3.0.80-rt108/arch/arm/include/asm/tlbflush.h
===================================================================

arch/arm/include/asm/tlbflush.h +0 -2
@@ -14,7 +14,6 @@
 
  #include <asm/glue.h>
 
-#define TLB_V3_PAGE    (1 << 0)
  #define TLB_V4_U_PAGE  (1 << 1)
  #define TLB_V4_D_PAGE  (1 << 2)
  #define TLB_V4_I_PAGE  (1 << 3)

@@ -22,7 +21,6 @@
  #define TLB_V6_D_PAGE  (1 << 5)
  #define TLB_V6_I_PAGE  (1 << 6)
 
-#define TLB_V3_FULL    (1 << 8)
  #define TLB_V4_U_FULL  (1 << 9)
  #define TLB_V4_D_FULL  (1 << 10)
  #define TLB_V4_I_FULL  (1 << 11)

@@ -34,16 +32,15 @@
  #define TLB_V6_D_ASID  (1 << 17)
  #define TLB_V6_I_ASID  (1 << 18)
Quoted 6 lines

arch/arm/include/asm/tlbflush.h line 35

Quoted 2 lines
arch/arm/include/asm/tlbflush.h +1 -1
-#define TLB_BTB                (1 << 28)
+#define TLB_V6_BP      (1 << 19)

This hunk (and related ones) are from a patch adding branch predictor maintenance that I also wrote. It's harmless, but you likely don't need it.

OK, will check.

Quoted 19 lines
Quoted 4 lines
arch/arm/include/asm/tlbflush.h +4 -0
+#ifdef CONFIG_ARM_ERRATA_798181
+static inline void dummy_flush_tlb_a15_erratum(void)
+{
+       /*

arch/arm/include/asm/tlbflush.h static inline void local_flush_tlb_kerne · line 459

Quoted 10 lines
arch/arm/include/asm/tlbflush.h +10 -0
+        * Dummy TLBIMVAIS. Using the unmapped address 0 and ASID 0.
+        */
+       asm("mcr p15, 0, %0, c8, c3, 1" : : "r" (0));
+       dsb();
+}
+#else
+static inline void dummy_flush_tlb_a15_erratum(void)
+{
+}
+#endif

And this is an A15 erratum workaround from Catalin. Actually, the original version of that workaround didn't interact nicely with PREEMPT kernels, so you should double-check what you've got (it was fixed recently in mainline).

Quoted 2 lines

Furthermore, the workaround requires IPIs on TLB invalidation, so you might have your livelock problem again...

Hmm, will check as well.

Quoted 8 lines
Quoted 8 lines

/*
Index: linux-3.0.80-rt108/arch/arm/mm/alignment.c
===================================================================

arch/arm/mm/alignment.c +0 -0
@@ -819,6 +819,7 @@ do_alignment(unsigned long addr, unsigne
                 break;
Quoted 6 lines

arch/arm/mm/alignment.c do_alignment(unsigned long addr, unsigne · line 825

Quoted 5 lines
arch/arm/mm/alignment.c +1 -0
         case 0x08000000:        /* ldm or stm, or thumb-2 32bit instruction */
+               offset.un = 0;
                 if (thumb2_32b)
                         handler = do_alignment_t32_to_handler(&instr, regs, &offset);
                 else

Unrelated?

This fixes a compiler warning:
CC arch/arm/mm/alignment.o
arch/arm/mm/alignment.c: In function ‘do_alignment’:
arch/arm/mm/alignment.c:327:15: warning: ‘offset.un’ may be used uninitialized in this function [-Wuninitialized] arch/arm/mm/alignment.c:749:21: note: ‘offset.un’ was declared here

Should have gone into a separate patch.

Thanks,
-Carsten.
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html