Skip to contentSkip to conversation

ARM TLB flush fix: don't forget to re-enable preemption

PATCH In review Kevin Hilman 4 participants 11 messages May 22, 2007 – May 24, 2007

lore
Share & tools
lore

Kevin Hilman

author
#0 lore raw

PATCH 1/1 ARM TLB flush fix: don't forget to re-enable preemption

Add a preempt_enable() to flush_tlb_kernel_page() since -rt4 patch adds a preempt_disable but no preempt_enable().

Signed-off-by Kevin Hilman <[email protected]>
---
 include/asm-arm/tlbflush.h |    1 +
 1 file changed, 1 insertion(+)
include/asm-arm/tlbflush.h +1 -0
Index: linux-2.6.21/include/asm-arm/tlbflush.h
===================================================================
@@ -378,6 +378,7 @@ static inline void local_flush_tlb_kerne
 		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (kaddr) : "cc");
 	if (tlb_flag(TLB_V6_I_PAGE))
 		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (kaddr) : "cc");
+	preempt_enable();
 
 	if (tlb_flag(TLB_V6_I_FULL | TLB_V6_D_FULL |
 		     TLB_V6_I_PAGE | TLB_V6_D_PAGE |

--

#1 lore raw
Quoted 15 lines · Kevin Hilman

Add a preempt_enable() to flush_tlb_kernel_page() since -rt4 patch adds a preempt_disable but no preempt_enable().

Signed-off-by Kevin Hilman <[email protected]>
---
 include/asm-arm/tlbflush.h |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.21/include/asm-arm/tlbflush.h
===================================================================

include/asm-arm/tlbflush.h static inline void local_flush_tlb_kerne · line 380

Quoted 3 lines
include/asm-arm/tlbflush.h +0 -0
@@ -378,6 +378,7 @@ static inline void local_flush_tlb_kerne
 		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (kaddr) : "cc");
 	if (tlb_flag(TLB_V6_I_PAGE))
 		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (kaddr) : "cc");

Aren't these mcr operations atomic?

Daniel

author
#2 lore raw
Quoted 11 lines · Daniel Walker
Quoted 10 lines · Kevin Hilman

Add a preempt_enable() to flush_tlb_kernel_page() since -rt4 patch adds a preempt_disable but no preempt_enable().

Signed-off-by Kevin Hilman <[email protected]>
---
 include/asm-arm/tlbflush.h |    1 +
 1 file changed, 1 insertion(+)
Quoted 11 lines

include/asm-arm/tlbflush.h static inline void local_flush_tlb_kerne · line 380

Quoted 8 lines

Index: linux-2.6.21/include/asm-arm/tlbflush.h
===================================================================

include/asm-arm/tlbflush.h +0 -0
@@ -378,6 +378,7 @@ static inline void local_flush_tlb_kerne
 		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (kaddr) : "cc");
 	if (tlb_flag(TLB_V6_I_PAGE))
 		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (kaddr) : "cc");

Aren't these mcr operations atomic?

Individually, yes. But the point of the preempt_disable/enable is to make the whole sequence atomic.

Kevin

#3 lore raw
Quoted 3 lines · Kevin Hilman

Individually, yes. But the point of the preempt_disable/enable is to make the whole sequence atomic.

I was under the impression that only one of those mcr lines is called per board type, the rest just compile away?

Daniel

#4 lore raw
In reply to Kevin Hilman
Quoted 24 lines · Kevin Hilman
Quoted 11 lines · Daniel Walker
Quoted 10 lines · Kevin Hilman

Add a preempt_enable() to flush_tlb_kernel_page() since -rt4 patch adds a preempt_disable but no preempt_enable().

Signed-off-by Kevin Hilman <[email protected]>
---
 include/asm-arm/tlbflush.h |    1 +
 1 file changed, 1 insertion(+)
Quoted 11 lines

include/asm-arm/tlbflush.h static inline void local_flush_tlb_kerne · line 380

Quoted 8 lines

Index: linux-2.6.21/include/asm-arm/tlbflush.h
===================================================================

include/asm-arm/tlbflush.h +0 -0
@@ -378,6 +378,7 @@ static inline void local_flush_tlb_kerne
 		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (kaddr) : "cc");
 	if (tlb_flag(TLB_V6_I_PAGE))
 		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (kaddr) : "cc");

Aren't these mcr operations atomic?

Quoted 2 lines

Individually, yes. But the point of the preempt_disable/enable is to make the whole sequence atomic.

In which case shouldn't it be at the end of the function so it includes the write buffer handling as well?

However, I think I agree with Daniel on this one. I don't see the point of the preempt_disable() here.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
author
#5 lore raw
Quoted 25 lines · Russell King
Quoted 24 lines · Kevin Hilman
Quoted 22 lines · Daniel Walker
Quoted 15 lines · Kevin Hilman

Add a preempt_enable() to flush_tlb_kernel_page() since -rt4 patch adds a preempt_disable but no preempt_enable().

Signed-off-by Kevin Hilman <[email protected]>
---
 include/asm-arm/tlbflush.h |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.21/include/asm-arm/tlbflush.h
===================================================================

include/asm-arm/tlbflush.h static inline void local_flush_tlb_kerne · line 380

Quoted 3 lines
include/asm-arm/tlbflush.h +0 -0
@@ -378,6 +378,7 @@ static inline void local_flush_tlb_kerne
 		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (kaddr) : "cc");
 	if (tlb_flag(TLB_V6_I_PAGE))
 		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (kaddr) : "cc");

Aren't these mcr operations atomic?

Quoted 8 lines
Quoted 2 lines

Individually, yes. But the point of the preempt_disable/enable is to make the whole sequence atomic.

In which case shouldn't it be at the end of the function so it includes the write buffer handling as well?

However, I think I agree with Daniel on this one. I don't see the point of the preempt_disable() here.

Note that my patch simply adds an enable to match the disable added by the -rt patch. I'm not sure where the disable originally came from, but there are disable/enable pairs scattered throughout tlbflush.h in the -rt patch.

If this one isn't necessary, then the others probably are not either. In most cases there are 2 mcr instructions inside the critical section. One for the dsb() and the other for the actual function.

Russell, is there a reason any of these sections should be atomic?

Kevin

#6 lore raw
Quoted 12 lines · Kevin Hilman
Quoted 5 lines · Russell King

In which case shouldn't it be at the end of the function so it includes the write buffer handling as well?

However, I think I agree with Daniel on this one. I don't see the point of the preempt_disable() here.

Note that my patch simply adds an enable to match the disable added by the -rt patch. I'm not sure where the disable originally came from, but there are disable/enable pairs scattered throughout tlbflush.h in the -rt patch.

Quoted 5 lines

If this one isn't necessary, then the others probably are not either. In most cases there are 2 mcr instructions inside the critical section. One for the dsb() and the other for the actual function.

Russell, is there a reason any of these sections should be atomic?

I don't see any reason for them to be - when switching to another process we'll generally do a full TLB flush anyway, so what's the point in making these flushes atomic?

Consider:

flush_tlb_page()
first mcr - invalidates tlb single entry
--- context switch, invalidates entire tlb, inc dsb ---
something else runs
--- context switch, invalidates entire tlb, inc dsb, again ---
dsb

That context switch is harmless - we end up with the entire TLB being invalidated and a DSB following. Now consider:

flush_tlb_page()
--- context switch, invalidates entire tlb, inc dsb ---
something else runs
--- context switch, invalidates entire tlb, inc dsb, again ---
preempt_disable()
first mcr - invalidates tlb single entry
dsb
preempt_enable()

Any difference? No. Without the preempt disable/enable fiddling? No.

flush_tlb_page()
preempt_disable()
first mcr - invalidates tlb single entry
dsb
preempt_enable()
--- context switch, invalidates entire tlb, inc dsb ---
something else runs
--- context switch, invalidates entire tlb, inc dsb, again ---

Any difference? No. Without the preempt disable/enable fiddling? No.

In every case of a preemption occuring in the middle of a tlb operation, the ultimate result is identical irrespective of preempt control sprinkling.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
author
#7 lore raw
Quoted 16 lines · Russell King
Quoted 15 lines · Kevin Hilman
Quoted 5 lines · Russell King

In which case shouldn't it be at the end of the function so it includes the write buffer handling as well?

However, I think I agree with Daniel on this one. I don't see the point of the preempt_disable() here.

Note that my patch simply adds an enable to match the disable added by the -rt patch. I'm not sure where the disable originally came from, but there are disable/enable pairs scattered throughout tlbflush.h in the -rt patch.

If this one isn't necessary, then the others probably are not either. In most cases there are 2 mcr instructions inside the critical section. One for the dsb() and the other for the actual function.

Quoted 5 lines
Quoted 1 line

Russell, is there a reason any of these sections should be atomic?

I don't see any reason for them to be - when switching to another process we'll generally do a full TLB flush anyway, so what's the point in making these flushes atomic?

OK, I've removed the locally and will be doing some testing on OMAP2 (ARMv6.) I'll submit a patch to Ingo if things look good.

In the meantime, my previous fix is still necessary for -rt to even work on ARM.

Kevin

#8 lore raw
In reply to Kevin Hilman
Quoted 10 lines · Kevin Hilman

Note that my patch simply adds an enable to match the disable added by the -rt patch. I'm not sure where the disable originally came from, but there are disable/enable pairs scattered throughout tlbflush.h in the -rt patch.

If this one isn't necessary, then the others probably are not either. In most cases there are 2 mcr instructions inside the critical section. One for the dsb() and the other for the actual function.

Russell, is there a reason any of these sections should be atomic?

Under normal preempt modes those functions would end running with preemption disabled , but with PREEMPT_RT enabled they become preemptive .. I may have been mistaken , but my impression was that one mcr instruction was executed and it was atomic , so there was no need for additional protection there..

If there are two mcr instructions then you could be preempted between the two, which may be unsafe depending on what those instructions are doing ..

Daniel

author
#10 lore raw
Quoted 7 lines · Thomas Gleixner
Quoted 4 lines · Kevin Hilman

Add a preempt_enable() to flush_tlb_kernel_page() since -rt4 patch adds a preempt_disable but no preempt_enable().

Signed-off-by Kevin Hilman <[email protected]>

Good catch. Applied.

Thomas could you apply this instead?

After discussions w/RMK and validation of my own on OMAP2 (ARMv6), it seems that these disable/enable sections aren't necessary.

Signed-off-by Kevin Hilman <[email protected]>

reverted:

include/asm-arm/tlbflush.h +0 -9
@@ -246,7 +246,6 @@
 	const int zero = 0;
 	const unsigned int __tlb_flag = __cpu_tlb_flags;
 
-	preempt_disable();
 	if (tlb_flag(TLB_WB))
 		dsb();
 

@@ -258,7 +257,6 @@
 		asm("mcr p15, 0, %0, c8, c6, 0" : : "r" (zero) : "cc");
 	if (tlb_flag(TLB_V4_I_FULL | TLB_V6_I_FULL))
 		asm("mcr p15, 0, %0, c8, c5, 0" : : "r" (zero) : "cc");
-	preempt_enable();
 
 	if (tlb_flag(TLB_V6_I_FULL | TLB_V6_D_FULL |
 		     TLB_V6_I_PAGE | TLB_V6_D_PAGE |

@@ -276,7 +274,6 @@
 	const int asid = ASID(mm);
 	const unsigned int __tlb_flag = __cpu_tlb_flags;
 
-	preempt_disable();
 	if (tlb_flag(TLB_WB))
 		dsb();
 

@@ -297,7 +294,6 @@
 		asm("mcr p15, 0, %0, c8, c6, 2" : : "r" (asid) : "cc");
 	if (tlb_flag(TLB_V6_I_ASID))
 		asm("mcr p15, 0, %0, c8, c5, 2" : : "r" (asid) : "cc");
-	preempt_enable();
 
 	if (tlb_flag(TLB_V6_I_FULL | TLB_V6_D_FULL |
 		     TLB_V6_I_PAGE | TLB_V6_D_PAGE |

@@ -314,7 +310,6 @@
 	const int zero = 0;
 	const unsigned int __tlb_flag = __cpu_tlb_flags;
 
-	preempt_disable();
 	uaddr = (uaddr & PAGE_MASK) | ASID(vma->vm_mm);
 
 	if (tlb_flag(TLB_WB))

@@ -339,7 +334,6 @@
 		asm("mcr p15, 0, %0, c8, c6, 1" : : "r" (uaddr) : "cc");
 	if (tlb_flag(TLB_V6_I_PAGE))
 		asm("mcr p15, 0, %0, c8, c5, 1" : : "r" (uaddr) : "cc");
-	preempt_enable();
 
 	if (tlb_flag(TLB_V6_I_FULL | TLB_V6_D_FULL |
 		     TLB_V6_I_PAGE | TLB_V6_D_PAGE |

@@ -355,7 +349,6 @@
 	const int zero = 0;
 	const unsigned int __tlb_flag = __cpu_tlb_flags;
 
-	preempt_disable();
 	kaddr &= PAGE_MASK;
 
 	if (tlb_flag(TLB_WB))

@@ -406,13 +399,11 @@
 {
 	const unsigned int __tlb_flag = __cpu_tlb_flags;
 
-	preempt_disable();
 	if (tlb_flag(TLB_DCLEAN))
 		asm("mcr	p15, 0, %0, c7, c10, 1	@ flush_pmd"
 			: : "r" (pmd) : "cc");
 	if (tlb_flag(TLB_WB))
 		dsb();
-	preempt_enable();
 }
 
 static inline void clean_pmd_entry(pmd_t *pmd)