| Message ID | 20240504122841.1177683-20-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | powerpc improvements | expand |
On 04/05/2024 14.28, Nicholas Piggin wrote: > The test harness uses spinlocks if they are implemented with larx/stcx. > it can prevent some test scenarios such as testing migration of a > reservation. I'm having a hard time to understand that patch description. Maybe you could rephrase it / elaborate what's the exact problem here? Thanks, Thomas > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > lib/powerpc/asm/smp.h | 1 + > lib/powerpc/smp.c | 5 +++++ > lib/powerpc/spinlock.c | 29 +++++++++++++++++++++++++++++ > lib/ppc64/asm/spinlock.h | 7 ++++++- > powerpc/Makefile.common | 1 + > 5 files changed, 42 insertions(+), 1 deletion(-) > create mode 100644 lib/powerpc/spinlock.c > > diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h > index c45988bfa..66188b9dd 100644 > --- a/lib/powerpc/asm/smp.h > +++ b/lib/powerpc/asm/smp.h > @@ -15,6 +15,7 @@ struct cpu { > > extern int nr_cpus_present; > extern int nr_cpus_online; > +extern bool multithreaded; > extern struct cpu cpus[]; > > register struct cpu *__current_cpu asm("r13"); > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c > index 27b169841..73c0ef214 100644 > --- a/lib/powerpc/smp.c > +++ b/lib/powerpc/smp.c > @@ -276,6 +276,8 @@ static void start_each_secondary(int fdtnode, u64 regval __unused, void *info) > start_core(fdtnode, datap->entry); > } > > +bool multithreaded = false; > + > /* > * Start all stopped cpus on the guest at entry with register 3 set to r3 > * We expect that we come in with only one thread currently started > @@ -290,6 +292,7 @@ bool start_all_cpus(secondary_entry_fn entry) > > assert(nr_cpus_online == 1); > assert(nr_started == 1); > + multithreaded = true; > ret = dt_for_each_cpu_node(start_each_secondary, &data); > assert(ret == 0); > assert(nr_started == nr_cpus_present); > @@ -361,10 +364,12 @@ static void wait_each_secondary(int fdtnode, u64 regval __unused, void *info) > > void stop_all_cpus(void) > { > + assert(multithreaded); > while (nr_cpus_online > 1) > cpu_relax(); > > dt_for_each_cpu_node(wait_each_secondary, NULL); > mb(); > nr_started = 1; > + multithreaded = false; > } > diff --git a/lib/powerpc/spinlock.c b/lib/powerpc/spinlock.c > new file mode 100644 > index 000000000..623a1f2c1 > --- /dev/null > +++ b/lib/powerpc/spinlock.c > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: LGPL-2.0 */ > +#include <asm/spinlock.h> > +#include <asm/smp.h> > + > +/* > + * Skip the atomic when single-threaded, which helps avoid larx/stcx. in > + * the harness when testing tricky larx/stcx. sequences (e.g., migration > + * vs reservation). > + */ > +void spin_lock(struct spinlock *lock) > +{ > + if (!multithreaded) { > + assert(lock->v == 0); > + lock->v = 1; > + } else { > + while (__sync_lock_test_and_set(&lock->v, 1)) > + ; > + } > +} > + > +void spin_unlock(struct spinlock *lock) > +{ > + assert(lock->v == 1); > + if (!multithreaded) { > + lock->v = 0; > + } else { > + __sync_lock_release(&lock->v); > + } > +} > diff --git a/lib/ppc64/asm/spinlock.h b/lib/ppc64/asm/spinlock.h > index f59eed191..b952386da 100644 > --- a/lib/ppc64/asm/spinlock.h > +++ b/lib/ppc64/asm/spinlock.h > @@ -1,6 +1,11 @@ > #ifndef _ASMPPC64_SPINLOCK_H_ > #define _ASMPPC64_SPINLOCK_H_ > > -#include <asm-generic/spinlock.h> > +struct spinlock { > + unsigned int v; > +}; > + > +void spin_lock(struct spinlock *lock); > +void spin_unlock(struct spinlock *lock); > > #endif /* _ASMPPC64_SPINLOCK_H_ */ > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common > index b98f71c2f..1ee9c25d6 100644 > --- a/powerpc/Makefile.common > +++ b/powerpc/Makefile.common > @@ -49,6 +49,7 @@ cflatobjs += lib/powerpc/rtas.o > cflatobjs += lib/powerpc/processor.o > cflatobjs += lib/powerpc/handlers.o > cflatobjs += lib/powerpc/smp.o > +cflatobjs += lib/powerpc/spinlock.o > > OBJDIRS += lib/powerpc >
On Tue Jun 4, 2024 at 3:27 PM AEST, Thomas Huth wrote: > On 04/05/2024 14.28, Nicholas Piggin wrote: > > The test harness uses spinlocks if they are implemented with larx/stcx. > > it can prevent some test scenarios such as testing migration of a > > reservation. > > I'm having a hard time to understand that patch description. Maybe you could > rephrase it / elaborate what's the exact problem here? Yeah that's wrong, "harness uses spinlocks *which are* implemented with larx/stcx." The problem IIRC was only testing migration of reservations, so I should be explicit about that in the changelog. You could leave this out for now. Thanks, Nick
diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h index c45988bfa..66188b9dd 100644 --- a/lib/powerpc/asm/smp.h +++ b/lib/powerpc/asm/smp.h @@ -15,6 +15,7 @@ struct cpu { extern int nr_cpus_present; extern int nr_cpus_online; +extern bool multithreaded; extern struct cpu cpus[]; register struct cpu *__current_cpu asm("r13"); diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c index 27b169841..73c0ef214 100644 --- a/lib/powerpc/smp.c +++ b/lib/powerpc/smp.c @@ -276,6 +276,8 @@ static void start_each_secondary(int fdtnode, u64 regval __unused, void *info) start_core(fdtnode, datap->entry); } +bool multithreaded = false; + /* * Start all stopped cpus on the guest at entry with register 3 set to r3 * We expect that we come in with only one thread currently started @@ -290,6 +292,7 @@ bool start_all_cpus(secondary_entry_fn entry) assert(nr_cpus_online == 1); assert(nr_started == 1); + multithreaded = true; ret = dt_for_each_cpu_node(start_each_secondary, &data); assert(ret == 0); assert(nr_started == nr_cpus_present); @@ -361,10 +364,12 @@ static void wait_each_secondary(int fdtnode, u64 regval __unused, void *info) void stop_all_cpus(void) { + assert(multithreaded); while (nr_cpus_online > 1) cpu_relax(); dt_for_each_cpu_node(wait_each_secondary, NULL); mb(); nr_started = 1; + multithreaded = false; } diff --git a/lib/powerpc/spinlock.c b/lib/powerpc/spinlock.c new file mode 100644 index 000000000..623a1f2c1 --- /dev/null +++ b/lib/powerpc/spinlock.c @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: LGPL-2.0 */ +#include <asm/spinlock.h> +#include <asm/smp.h> + +/* + * Skip the atomic when single-threaded, which helps avoid larx/stcx. in + * the harness when testing tricky larx/stcx. sequences (e.g., migration + * vs reservation). + */ +void spin_lock(struct spinlock *lock) +{ + if (!multithreaded) { + assert(lock->v == 0); + lock->v = 1; + } else { + while (__sync_lock_test_and_set(&lock->v, 1)) + ; + } +} + +void spin_unlock(struct spinlock *lock) +{ + assert(lock->v == 1); + if (!multithreaded) { + lock->v = 0; + } else { + __sync_lock_release(&lock->v); + } +} diff --git a/lib/ppc64/asm/spinlock.h b/lib/ppc64/asm/spinlock.h index f59eed191..b952386da 100644 --- a/lib/ppc64/asm/spinlock.h +++ b/lib/ppc64/asm/spinlock.h @@ -1,6 +1,11 @@ #ifndef _ASMPPC64_SPINLOCK_H_ #define _ASMPPC64_SPINLOCK_H_ -#include <asm-generic/spinlock.h> +struct spinlock { + unsigned int v; +}; + +void spin_lock(struct spinlock *lock); +void spin_unlock(struct spinlock *lock); #endif /* _ASMPPC64_SPINLOCK_H_ */ diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common index b98f71c2f..1ee9c25d6 100644 --- a/powerpc/Makefile.common +++ b/powerpc/Makefile.common @@ -49,6 +49,7 @@ cflatobjs += lib/powerpc/rtas.o cflatobjs += lib/powerpc/processor.o cflatobjs += lib/powerpc/handlers.o cflatobjs += lib/powerpc/smp.o +cflatobjs += lib/powerpc/spinlock.o OBJDIRS += lib/powerpc
The test harness uses spinlocks if they are implemented with larx/stcx. it can prevent some test scenarios such as testing migration of a reservation. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- lib/powerpc/asm/smp.h | 1 + lib/powerpc/smp.c | 5 +++++ lib/powerpc/spinlock.c | 29 +++++++++++++++++++++++++++++ lib/ppc64/asm/spinlock.h | 7 ++++++- powerpc/Makefile.common | 1 + 5 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 lib/powerpc/spinlock.c