* [RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait() @ 2012-11-19 6:16 Li Zhong 2013-01-10 6:02 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 8+ messages in thread From: Li Zhong @ 2012-11-19 6:16 UTC (permalink / raw) To: PowerPC email list; +Cc: Paul E. McKenney, Paul Mackerras 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait() 2012-11-19 6:16 [RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait() Li Zhong @ 2013-01-10 6:02 ` Benjamin Herrenschmidt 2013-01-10 7:50 ` Li Zhong 2013-01-10 9:00 ` [PATCH powerpc ] Avoid debug_smp_processor_id() check " Li Zhong 0 siblings, 2 replies; 8+ messages in thread From: Benjamin Herrenschmidt @ 2013-01-10 6:02 UTC (permalink / raw) To: Li Zhong; +Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait() 2013-01-10 6:02 ` Benjamin Herrenschmidt @ 2013-01-10 7:50 ` Li Zhong 2013-01-10 9:00 ` [PATCH powerpc ] Avoid debug_smp_processor_id() check " Li Zhong 1 sibling, 0 replies; 8+ messages in thread From: Li Zhong @ 2013-01-10 7:50 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list 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. > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH powerpc ] Avoid debug_smp_processor_id() check in arch_spin_unlock_wait() 2013-01-10 6:02 ` Benjamin Herrenschmidt 2013-01-10 7:50 ` Li Zhong @ 2013-01-10 9:00 ` Li Zhong 2013-01-24 3:47 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 8+ messages in thread From: Li Zhong @ 2013-01-10 9:00 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH powerpc ] Avoid debug_smp_processor_id() check in arch_spin_unlock_wait() 2013-01-10 9:00 ` [PATCH powerpc ] Avoid debug_smp_processor_id() check " Li Zhong @ 2013-01-24 3:47 ` Benjamin Herrenschmidt 2013-01-24 10:13 ` Li Zhong 0 siblings, 1 reply; 8+ messages in thread From: Benjamin Herrenschmidt @ 2013-01-24 3:47 UTC (permalink / raw) To: Li Zhong; +Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list 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(); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH powerpc ] Avoid debug_smp_processor_id() check in arch_spin_unlock_wait() 2013-01-24 3:47 ` Benjamin Herrenschmidt @ 2013-01-24 10:13 ` Li Zhong 2013-01-24 21:44 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 8+ messages in thread From: Li Zhong @ 2013-01-24 10:13 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list 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(); > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH powerpc ] Avoid debug_smp_processor_id() check in arch_spin_unlock_wait() 2013-01-24 10:13 ` Li Zhong @ 2013-01-24 21:44 ` Benjamin Herrenschmidt 2013-01-25 7:51 ` [PATCH v2 powerpc ] Avoid debug_smp_processor_id() check in SHARED_PROCESSOR Li Zhong 0 siblings, 1 reply; 8+ messages in thread From: Benjamin Herrenschmidt @ 2013-01-24 21:44 UTC (permalink / raw) To: Li Zhong; +Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 powerpc ] Avoid debug_smp_processor_id() check in SHARED_PROCESSOR 2013-01-24 21:44 ` Benjamin Herrenschmidt @ 2013-01-25 7:51 ` Li Zhong 0 siblings, 0 replies; 8+ messages in thread From: Li Zhong @ 2013-01-25 7:51 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-01-25 7:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-19 6:16 [RFC PATCH powerpc ] Protect smp_processor_id() in arch_spin_unlock_wait() Li Zhong 2013-01-10 6:02 ` Benjamin Herrenschmidt 2013-01-10 7:50 ` Li Zhong 2013-01-10 9:00 ` [PATCH powerpc ] Avoid debug_smp_processor_id() check " Li Zhong 2013-01-24 3:47 ` Benjamin Herrenschmidt 2013-01-24 10:13 ` Li Zhong 2013-01-24 21:44 ` Benjamin Herrenschmidt 2013-01-25 7:51 ` [PATCH v2 powerpc ] Avoid debug_smp_processor_id() check in SHARED_PROCESSOR Li Zhong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).