[RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait()

[RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait()

[Li Zhong]

This patch tries to disable preemption for using smp_processor_id() in arch_spin_unlock_wait(), 
to avoid following report:

[   17.279377] BUG: using smp_processor_id() in preemptible [00000000] code: autorun/1024
[   17.279407] caller is .arch_spin_unlock_wait+0x34/0x80
[   17.279415] Call Trace:
[   17.279423] [c00000000911fb30] [c000000000015230] .show_stack+0x70/0x1c0 (unreliable)
[   17.279441] [c00000000911fbe0] [c0000000003e13b0] .debug_smp_processor_id+0x110/0x130
[   17.279455] [c00000000911fc80] [c00000000004c2b4] .arch_spin_unlock_wait+0x34/0x80
[   17.279470] [c00000000911fd00] [c0000000000b0e50] .task_work_run+0x80/0x160
[   17.279482] [c00000000911fdb0] [c000000000018744] .do_notify_resume+0x94/0xa0
[   17.279495] [c00000000911fe30] [c000000000009d1c] .ret_from_except_lite+0x48/0x4c

Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 arch/powerpc/lib/locks.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/lib/locks.c b/arch/powerpc/lib/locks.c
index bb7cfec..7a7c31b 100644
--- a/arch/powerpc/lib/locks.c
+++ b/arch/powerpc/lib/locks.c
@@ -72,8 +72,10 @@ void arch_spin_unlock_wait(arch_spinlock_t *lock)
 {
 	while (lock->slock) {
 		HMT_low();
+		preempt_disable();
 		if (SHARED_PROCESSOR)
 			__spin_yield(lock);
+		preempt_enable();
 	}
 	HMT_medium();
 }
-- 
1.7.9.5

Re: [RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait()

[Benjamin Herrenschmidt]

On Mon, 2012-11-19 at 14:16 +0800, Li Zhong wrote:
> This patch tries to disable preemption for using smp_processor_id() in arch_spin_unlock_wait(), 
> to avoid following report:

 .../...

> diff --git a/arch/powerpc/lib/locks.c b/arch/powerpc/lib/locks.c
> index bb7cfec..7a7c31b 100644
> --- a/arch/powerpc/lib/locks.c
> +++ b/arch/powerpc/lib/locks.c
> @@ -72,8 +72,10 @@ void arch_spin_unlock_wait(arch_spinlock_t *lock)
>  {
>  	while (lock->slock) {
>  		HMT_low();
> +		preempt_disable();
>  		if (SHARED_PROCESSOR)
>  			__spin_yield(lock);
> +		preempt_enable();
>  	}

I assume what you are protecting is the PACA access in SHARED_PROCESSOR
or is there more ?

In that case I'd say just make it use local_paca-> directly or something
like that. It doesn't matter if the access is racy, all processors will
have the same value for that field as far as I can tell.

Cheers,
Ben.

Re: [RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait()

[Li Zhong]

On Thu, 2013-01-10 at 17:02 +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2012-11-19 at 14:16 +0800, Li Zhong wrote:
> > This patch tries to disable preemption for using smp_processor_id() in arch_spin_unlock_wait(), 
> > to avoid following report:
> 
>  .../...
> 
> > diff --git a/arch/powerpc/lib/locks.c b/arch/powerpc/lib/locks.c
> > index bb7cfec..7a7c31b 100644
> > --- a/arch/powerpc/lib/locks.c
> > +++ b/arch/powerpc/lib/locks.c
> > @@ -72,8 +72,10 @@ void arch_spin_unlock_wait(arch_spinlock_t *lock)
> >  {
> >  	while (lock->slock) {
> >  		HMT_low();
> > +		preempt_disable();
> >  		if (SHARED_PROCESSOR)
> >  			__spin_yield(lock);
> > +		preempt_enable();
> >  	}
> 
> I assume what you are protecting is the PACA access in SHARED_PROCESSOR
> or is there more ?

Yes, only the one in SHARED_PROCESSOR.

> 
> In that case I'd say just make it use local_paca-> directly or something
> like that. It doesn't matter if the access is racy, all processors will
> have the same value for that field as far as I can tell.

It also seemed to me that all processors have the same value :). I'll
send an updated version based on your suggestion soon.

Thanks, Zhong

> 
> Cheers,
> Ben.
> 
> 

[PATCH powerpc ] Avoid debug_smp_processor_id() check in arch_spin_unlock_wait()

[Li Zhong]

Use local_paca directly in arch_spin_unlock_wait(), as all processors have the
same value for the field shared_proc, so we don't need care racy here.

Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 arch/powerpc/lib/locks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/lib/locks.c b/arch/powerpc/lib/locks.c
index bb7cfec..850bea6 100644
--- a/arch/powerpc/lib/locks.c
+++ b/arch/powerpc/lib/locks.c
@@ -72,7 +72,7 @@ void arch_spin_unlock_wait(arch_spinlock_t *lock)
 {
 	while (lock->slock) {
 		HMT_low();
-		if (SHARED_PROCESSOR)
+		if (local_paca->lppaca_ptr->shared_proc)
 			__spin_yield(lock);
 	}
 	HMT_medium();
-- 
1.7.9.5

Re: [PATCH powerpc ] Avoid debug_smp_processor_id() check in arch_spin_unlock_wait()

[Benjamin Herrenschmidt]

On Thu, 2013-01-10 at 17:00 +0800, Li Zhong wrote:
> Use local_paca directly in arch_spin_unlock_wait(), as all processors have the
> same value for the field shared_proc, so we don't need care racy here.

Of course that won't build if CONFIG_PPC_SPLPAR isn't defined...

Maybe you could change the definition of the SHARED_PROCESSOR
macro itself. The only possible "risk" would be a stale lppaca
if we preempt & hot unplug the CPU at the wrong time (provided
we no longer stop_machine either), I suppose if that's a real
concern we could delay freeing of lppaca's via RCU or such.

Ben.

> Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> ---
>  arch/powerpc/lib/locks.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/lib/locks.c b/arch/powerpc/lib/locks.c
> index bb7cfec..850bea6 100644
> --- a/arch/powerpc/lib/locks.c
> +++ b/arch/powerpc/lib/locks.c
> @@ -72,7 +72,7 @@ void arch_spin_unlock_wait(arch_spinlock_t *lock)
>  {
>  	while (lock->slock) {
>  		HMT_low();
> -		if (SHARED_PROCESSOR)
> +		if (local_paca->lppaca_ptr->shared_proc)
>  			__spin_yield(lock);
>  	}
>  	HMT_medium();

Re: [PATCH powerpc ] Avoid debug_smp_processor_id() check in arch_spin_unlock_wait()

[Li Zhong]

On Thu, 2013-01-24 at 14:47 +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2013-01-10 at 17:00 +0800, Li Zhong wrote:
> > Use local_paca directly in arch_spin_unlock_wait(), as all processors have the
> > same value for the field shared_proc, so we don't need care racy here.
> 
> Of course that won't build if CONFIG_PPC_SPLPAR isn't defined...

...ah, I didn't notice that lppaca_ptr is only defined under
CONFIG_PPC_BOOK3S, and the whole paca is only defined under
CONFIG_PPC64; while the function changed seems could be used by any
configuration. Sorry about the carelessness. 

> 
> Maybe you could change the definition of the SHARED_PROCESSOR
> macro itself. The only possible "risk" would be a stale lppaca
> if we preempt & hot unplug the CPU at the wrong time (provided
> we no longer stop_machine either), I suppose if that's a real
> concern we could delay freeing of lppaca's via RCU or such.

I'm not very clear about the "risk" you mentioned above. It seems to me
that the freeing of lppaca only appeared in free_unused_pacas(), called
by early setup code, at which time hotplug seems impossible. 

I must missed something ...

I'll update the SHARED_PROCESSOR directly, it seems better to me now.
(wonder why I gave it up immediately when it first came into my mind
while I was doing the last update...)

Thanks, Zhong

> Ben.
> 
> > Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/lib/locks.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/lib/locks.c b/arch/powerpc/lib/locks.c
> > index bb7cfec..850bea6 100644
> > --- a/arch/powerpc/lib/locks.c
> > +++ b/arch/powerpc/lib/locks.c
> > @@ -72,7 +72,7 @@ void arch_spin_unlock_wait(arch_spinlock_t *lock)
> >  {
> >  	while (lock->slock) {
> >  		HMT_low();
> > -		if (SHARED_PROCESSOR)
> > +		if (local_paca->lppaca_ptr->shared_proc)
> >  			__spin_yield(lock);
> >  	}
> >  	HMT_medium();
> 
> 

Re: [PATCH powerpc ] Avoid debug_smp_processor_id() check in arch_spin_unlock_wait()

[Benjamin Herrenschmidt]

On Thu, 2013-01-24 at 18:13 +0800, Li Zhong wrote:
> I'm not very clear about the "risk" you mentioned above. It seems to me
> that the freeing of lppaca only appeared in free_unused_pacas(), called
> by early setup code, at which time hotplug seems impossible. 
> 
> I must missed something ...

No you are right, we don't free them at the moment.

> I'll update the SHARED_PROCESSOR directly, it seems better to me now.
> (wonder why I gave it up immediately when it first came into my mind
> while I was doing the last update...)

Cheers,
Ben.

[PATCH v2 powerpc ] Avoid debug_smp_processor_id() check in SHARED_PROCESSOR

[Li Zhong]

Use local_paca directly in macro SHARED_PROCESSOR, as all processors
have the same value for the field shared_proc, so we don't need care
racy here.

Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/spinlock.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
index 7124fc0..5b23f91 100644
--- a/arch/powerpc/include/asm/spinlock.h
+++ b/arch/powerpc/include/asm/spinlock.h
@@ -96,7 +96,7 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
 
 #if defined(CONFIG_PPC_SPLPAR)
 /* We only yield to the hypervisor if we are in shared processor mode */
-#define SHARED_PROCESSOR (get_lppaca()->shared_proc)
+#define SHARED_PROCESSOR (local_paca->lppaca_ptr->shared_proc)
 extern void __spin_yield(arch_spinlock_t *lock);
 extern void __rw_yield(arch_rwlock_t *lock);
 #else /* SPLPAR */
-- 
1.7.1