[PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[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 <khilman@mvista.com>


---
 include/asm-arm/tlbflush.h |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.21/include/asm-arm/tlbflush.h
===================================================================
--- linux-2.6.21.orig/include/asm-arm/tlbflush.h
+++ 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 |
--

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Daniel Walker]

On Tue, 2007-05-22 at 16:01 -0700, Kevin Hilman wrote:
> 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 <khilman@mvista.com>
> 
> 
> ---
>  include/asm-arm/tlbflush.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: linux-2.6.21/include/asm-arm/tlbflush.h
> ===================================================================
> --- linux-2.6.21.orig/include/asm-arm/tlbflush.h
> +++ 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");

Aren't these mcr operations atomic?

Daniel

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Kevin Hilman]

On Tue, 2007-05-22 at 16:25 -0700, Daniel Walker wrote:
> On Tue, 2007-05-22 at 16:01 -0700, Kevin Hilman wrote:
> > 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 <khilman@mvista.com>
> > 
> > 
> > ---
> >  include/asm-arm/tlbflush.h |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > Index: linux-2.6.21/include/asm-arm/tlbflush.h
> > ===================================================================
> > --- linux-2.6.21.orig/include/asm-arm/tlbflush.h
> > +++ 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");
> 
> Aren't these mcr operations atomic?
> 

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

Kevin

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Daniel Walker]

On Tue, 2007-05-22 at 16:41 -0700, Kevin Hilman wrote:
 
> 
> 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

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Russell King]

On Tue, May 22, 2007 at 04:41:36PM -0700, Kevin Hilman wrote:
> On Tue, 2007-05-22 at 16:25 -0700, Daniel Walker wrote:
> > On Tue, 2007-05-22 at 16:01 -0700, Kevin Hilman wrote:
> > > 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 <khilman@mvista.com>
> > > 
> > > 
> > > ---
> > >  include/asm-arm/tlbflush.h |    1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > Index: linux-2.6.21/include/asm-arm/tlbflush.h
> > > ===================================================================
> > > --- linux-2.6.21.orig/include/asm-arm/tlbflush.h
> > > +++ 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");
> > 
> > Aren't these mcr operations atomic?
> > 
> 
> 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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Kevin Hilman]

On Wed, 2007-05-23 at 10:22 +0100, Russell King wrote:
> On Tue, May 22, 2007 at 04:41:36PM -0700, Kevin Hilman wrote:
> > On Tue, 2007-05-22 at 16:25 -0700, Daniel Walker wrote:
> > > On Tue, 2007-05-22 at 16:01 -0700, Kevin Hilman wrote:
> > > > 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 <khilman@mvista.com>
> > > > 
> > > > 
> > > > ---
> > > >  include/asm-arm/tlbflush.h |    1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > Index: linux-2.6.21/include/asm-arm/tlbflush.h
> > > > ===================================================================
> > > > --- linux-2.6.21.orig/include/asm-arm/tlbflush.h
> > > > +++ 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");
> > > 
> > > Aren't these mcr operations atomic?
> > > 
> > 
> > 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

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Russell King]

On Wed, May 23, 2007 at 09:13:57AM -0700, Kevin Hilman wrote:
> On Wed, 2007-05-23 at 10:22 +0100, Russell King wrote:
> > 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?

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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Kevin Hilman]

Russell King wrote:
> On Wed, May 23, 2007 at 09:13:57AM -0700, Kevin Hilman wrote:
>> On Wed, 2007-05-23 at 10:22 +0100, Russell King wrote:
>>> 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?
> 
> 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

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Daniel Walker]

On Wed, 2007-05-23 at 09:13 -0700, Kevin Hilman wrote:

> 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

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Thomas Gleixner]

On Tue, 2007-05-22 at 16:01 -0700, Kevin Hilman wrote:
> 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 <khilman@mvista.com>

Good catch. Applied.

Thanks,

	tglx

Re: [PATCH -rt] ARM TLB flush fix: don't forget to re-enable preemption

[Kevin Hilman]

Thomas Gleixner wrote:
> On Tue, 2007-05-22 at 16:01 -0700, Kevin Hilman wrote:
>> 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 <khilman@mvista.com>
> 
> 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 <khilman@mvista.com>

reverted:
--- linux-2.6.21/include/asm-arm/tlbflush.h
+++ linux-2.6.21.orig/include/asm-arm/tlbflush.h
@@ -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)