[PATCH -rt] revert bh_lru_lock() to preempt_disable()

[PATCH -rt] revert bh_lru_lock() to preempt_disable()

[Daniel Walker]

	The bh_lru_lock() was set to disable interrupt to protect
from IPI's, which are only on SMP . So I don't think it's needed
in UP PREEMPT_RT configs. 

Signed-Off-By: Daniel Walker <dwalker@mvista.com>

Index: linux-2.6.16/fs/buffer.c
===================================================================
--- linux-2.6.16.orig/fs/buffer.c
+++ linux-2.6.16/fs/buffer.c
@@ -1326,7 +1326,7 @@ struct bh_lru {
 
 static DEFINE_PER_CPU(struct bh_lru, bh_lrus) = {{ NULL }};
 
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
+#if defined(CONFIG_SMP) 
 #define bh_lru_lock()	local_irq_disable()
 #define bh_lru_unlock()	local_irq_enable()
 #else

Re: [PATCH -rt] revert bh_lru_lock() to preempt_disable()

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> 	The bh_lru_lock() was set to disable interrupt to protect from 
> IPI's, which are only on SMP . So I don't think it's needed in UP 
> PREEMPT_RT configs.

i agree that this is a problem, but the fix is incorrect. What would be 
the right approach is to convert the PER_CPU bh_lrus to PER_CPU_LOCKED, 
and to use the appropriate primitives to use them. That automatically 
makes this code rt-safe. (it isnt right now)

	Ingo

Re: [PATCH -rt] revert bh_lru_lock() to preempt_disable()

[Daniel Walker]

On Wed, 2006-05-03 at 22:47 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > 	The bh_lru_lock() was set to disable interrupt to protect from 
> > IPI's, which are only on SMP . So I don't think it's needed in UP 
> > PREEMPT_RT configs.
> 
> i agree that this is a problem, but the fix is incorrect. What would be 
> the right approach is to convert the PER_CPU bh_lrus to PER_CPU_LOCKED, 
> and to use the appropriate primitives to use them. That automatically 
> makes this code rt-safe. (it isnt right now)


Hmm, in UP it should be safe to access per cpu data under either a
preempt_disable or local_irq_disable . I'm not sure how RT changes
that .. Is there some other part of the code that isn't rt-safe, which
I've overlooked ?

Daniel

Re: [PATCH -rt] revert bh_lru_lock() to preempt_disable()

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> > i agree that this is a problem, but the fix is incorrect. What would be 
> > the right approach is to convert the PER_CPU bh_lrus to PER_CPU_LOCKED, 
> > and to use the appropriate primitives to use them. That automatically 
> > makes this code rt-safe. (it isnt right now)
> 
> Hmm, in UP it should be safe to access per cpu data under either a 
> preempt_disable or local_irq_disable . I'm not sure how RT changes 
> that .. Is there some other part of the code that isn't rt-safe, which 
> I've overlooked ?

hm, you are right - that code can be in a preempt-off section. I've 
applied your patch.

	Ingo

Re: [PATCH -rt] revert bh_lru_lock() to preempt_disable()

[Daniel Walker]

On Wed, 2006-05-03 at 23:01 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > > i agree that this is a problem, but the fix is incorrect. What would be 
> > > the right approach is to convert the PER_CPU bh_lrus to PER_CPU_LOCKED, 
> > > and to use the appropriate primitives to use them. That automatically 
> > > makes this code rt-safe. (it isnt right now)
> > 
> > Hmm, in UP it should be safe to access per cpu data under either a 
> > preempt_disable or local_irq_disable . I'm not sure how RT changes 
> > that .. Is there some other part of the code that isn't rt-safe, which 
> > I've overlooked ?
> 
> hm, you are right - that code can be in a preempt-off section. I've 
> applied your patch.

We could switch it to locked types if it's a latency concern..

Daniel