From: Christopher M. Riedl <hidden> Date: 2019-08-02 04:21:16
Fixes an oops when calling the shared-processor spinlock implementation
from a non-SP LPAR. Also take this opportunity to refactor
SHARED_PROCESSOR a bit.
Reference: https://github.com/linuxppc/issues/issues/229
Changes since v1:
- Improve comment wording to make it clear why the BOOK3S #ifdef is
required in is_shared_processor() in spinlock.h
- Replace empty #define of splpar_*_yield() with actual functions with
empty bodies.
Christopher M. Riedl (3):
powerpc/spinlocks: Refactor SHARED_PROCESSOR
powerpc/spinlocks: Rename SPLPAR-only spinlocks
powerpc/spinlocks: Fix oops in shared-processor spinlocks
arch/powerpc/include/asm/spinlock.h | 62 +++++++++++++++++++++--------
arch/powerpc/lib/locks.c | 6 +--
2 files changed, 48 insertions(+), 20 deletions(-)
--
2.22.0
From: Christopher M. Riedl <hidden> Date: 2019-08-02 04:22:53
The __rw_yield and __spin_yield locks only pertain to SPLPAR mode.
Rename them to make this relationship obvious.
Signed-off-by: Christopher M. Riedl <redacted>
Reviewed-by: Andrew Donnellan <redacted>
---
arch/powerpc/include/asm/spinlock.h | 6 ++++--
arch/powerpc/lib/locks.c | 6 +++---
2 files changed, 7 insertions(+), 5 deletions(-)
@@ -101,8 +101,10 @@ 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 */-externvoid__spin_yield(arch_spinlock_t*lock);-externvoid__rw_yield(arch_rwlock_t*lock);+voidsplpar_spin_yield(arch_spinlock_t*lock);+voidsplpar_rw_yield(arch_rwlock_t*lock);+#define __spin_yield(x) splpar_spin_yield(x)+#define __rw_yield(x) splpar_rw_yield(x)#else /* SPLPAR */#define __spin_yield(x) barrier()#define __rw_yield(x) barrier()
From: Christopher M. Riedl <hidden> Date: 2019-08-02 04:24:24
Determining if a processor is in shared processor mode is not a constant
so don't hide it behind a #define.
Signed-off-by: Christopher M. Riedl <redacted>
Reviewed-by: Andrew Donnellan <redacted>
---
arch/powerpc/include/asm/spinlock.h | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
@@ -101,15 +101,27 @@ 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 (lppaca_shared_proc(local_paca->lppaca_ptr))externvoid__spin_yield(arch_spinlock_t*lock);externvoid__rw_yield(arch_rwlock_t*lock);#else /* SPLPAR */#define __spin_yield(x) barrier()#define __rw_yield(x) barrier()-#define SHARED_PROCESSOR 0#endif+staticinlineboolis_shared_processor(void)+{+/*+*LPPACAisonlyavailableonBOOK3SsoguardanythingLPPACArelatedto+*allowotherplatforms(whichincludethiscommonheader)tocompile.+*/+#ifdef CONFIG_PPC_BOOK3S+return(IS_ENABLED(CONFIG_PPC_SPLPAR)&&+lppaca_shared_proc(local_paca->lppaca_ptr));+#else+returnfalse;+#endif+}+staticinlinevoidarch_spin_lock(arch_spinlock_t*lock){while(1){
@@ -103,11 +103,9 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)/* We only yield to the hypervisor if we are in shared processor mode */voidsplpar_spin_yield(arch_spinlock_t*lock);voidsplpar_rw_yield(arch_rwlock_t*lock);-#define __spin_yield(x) splpar_spin_yield(x)-#define __rw_yield(x) splpar_rw_yield(x)#else /* SPLPAR */-#define __spin_yield(x) barrier()-#define __rw_yield(x) barrier()+staticinlinevoidsplpar_spin_yield(arch_spinlock_t*lock){};+staticinlinevoidsplpar_rw_yield(arch_rwlock_t*lock){};#endifstaticinlineboolis_shared_processor(void)
static inline void arch_spin_lock(arch_spinlock_t *lock)
{
while (1) {
@@ -132,7 +146,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) do { HMT_low(); if (is_shared_processor())- __spin_yield(lock);+ spin_yield(lock);
This leaves us with a double test of is_shared_processor() doesn't it?
Yep, and that's no good. Hmm, executing the barrier() in the non-shared-processor
case probably hurts performance here?
It's only a "compiler barrier", so it shouldn't generate any code.
But it does have the effect of telling the compiler it can't optimise
across that barrier, which can be important.
In those spin loops all we're doing is checking lock->slock which is
already marked volatile in the definition of arch_spinlock_t, so the
extra barrier shouldn't really make any difference.
But still the current code doesn't have a barrier() there, so we should
make sure we don't introduce one as part of this refactor.
So I think you just want to change the call to spin_yield() above to
splpar_spin_yield(), which avoids the double check, and also avoids the
barrier() in the SPLPAR=n case.
And then arch_spin_relax() calls spin_yield() etc.
cheers
From: Christopher M Riedl <hidden> Date: 2019-08-06 12:34:06
On August 6, 2019 at 7:14 AM Michael Ellerman [off-list ref] wrote:
Christopher M Riedl [off-list ref] writes:
quoted
quoted
On August 2, 2019 at 6:38 AM Michael Ellerman [off-list ref] wrote:
"Christopher M. Riedl" [off-list ref] writes:
This leaves us with a double test of is_shared_processor() doesn't it?
Yep, and that's no good. Hmm, executing the barrier() in the non-shared-processor
case probably hurts performance here?
It's only a "compiler barrier", so it shouldn't generate any code.
But it does have the effect of telling the compiler it can't optimise
across that barrier, which can be important.
In those spin loops all we're doing is checking lock->slock which is
already marked volatile in the definition of arch_spinlock_t, so the
extra barrier shouldn't really make any difference.
But still the current code doesn't have a barrier() there, so we should
make sure we don't introduce one as part of this refactor.
Thank you for taking the time to explain this. I have some more reading to
do about compiler-barriers it seems :)
So I think you just want to change the call to spin_yield() above to
splpar_spin_yield(), which avoids the double check, and also avoids the
barrier() in the SPLPAR=n case.
And then arch_spin_relax() calls spin_yield() etc.
I submitted a v3 before your reply with this change already - figured this
is the best way to avoid the double check and maintain legacy behavior.
On Tue, Aug 06, 2019 at 10:14:27PM +1000, Michael Ellerman wrote:
Christopher M Riedl [off-list ref] writes:
quoted
Yep, and that's no good. Hmm, executing the barrier() in the non-shared-processor
case probably hurts performance here?
It's only a "compiler barrier", so it shouldn't generate any code.
But it does have the effect of telling the compiler it can't optimise
across that barrier, which can be important.
This is
#define barrier() __asm__ __volatile__("": : :"memory")
It doesn't tell the compiler "not to optimise" across the barrier. It
tells the compiler that all memory accesses before the barrier should
stay before it, and all accesses after the barrier should stay after it,
because it says the "barrier" can access and/or change any memory.
This does not tell the hardware not to move those accesses around. It
also doesn't say anything about things that are not in memory. Not
everything you think is in memory, is. What is and isn't in memory can
change during compilation.
[ This message brought to you by the "Stamp Out Optimisation Barrier"
campaign. ]
Segher