| Message ID | 20230405141710.3551-4-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | locking: Introduce local{,64}_try_cmpxchg | expand |
On Wed, Apr 05, 2023 at 04:17:08PM +0200, Uros Bizjak wrote: > diff --git a/arch/powerpc/include/asm/local.h b/arch/powerpc/include/asm/local.h > index bc4bd19b7fc2..45492fb5bf22 100644 > --- a/arch/powerpc/include/asm/local.h > +++ b/arch/powerpc/include/asm/local.h > @@ -90,6 +90,17 @@ static __inline__ long local_cmpxchg(local_t *l, long o, long n) > return t; > } > > +static __inline__ bool local_try_cmpxchg(local_t *l, long *po, long n) > +{ > + long o = *po, r; > + > + r = local_cmpxchg(l, o, n); > + if (unlikely(r != o)) > + *po = r; > + > + return likely(r == o); > +} > + Why is the ppc one different from the rest? Why can't it use the try_cmpxchg_local() fallback and needs to have it open-coded?
On Wed, Apr 12, 2023 at 1:33 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Apr 05, 2023 at 04:17:08PM +0200, Uros Bizjak wrote: > > diff --git a/arch/powerpc/include/asm/local.h b/arch/powerpc/include/asm/local.h > > index bc4bd19b7fc2..45492fb5bf22 100644 > > --- a/arch/powerpc/include/asm/local.h > > +++ b/arch/powerpc/include/asm/local.h > > @@ -90,6 +90,17 @@ static __inline__ long local_cmpxchg(local_t *l, long o, long n) > > return t; > > } > > > > +static __inline__ bool local_try_cmpxchg(local_t *l, long *po, long n) > > +{ > > + long o = *po, r; > > + > > + r = local_cmpxchg(l, o, n); > > + if (unlikely(r != o)) > > + *po = r; > > + > > + return likely(r == o); > > +} > > + > > Why is the ppc one different from the rest? Why can't it use the > try_cmpxchg_local() fallback and needs to have it open-coded? Please note that ppc directly defines local_cmpxchg that bypasses cmpxchg_local/arch_cmpxchg_local machinery. The patch takes the same approach for local_try_cmpxchg, because fallbacks are using arch_cmpxchg_local definitions. PPC should be converted to use arch_cmpxchg_local (to also enable instrumentation), but this is not the scope of the proposed patchset. Uros.
On Wed, Apr 12, 2023 at 03:37:50PM +0200, Uros Bizjak wrote: > On Wed, Apr 12, 2023 at 1:33 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Wed, Apr 05, 2023 at 04:17:08PM +0200, Uros Bizjak wrote: > > > diff --git a/arch/powerpc/include/asm/local.h b/arch/powerpc/include/asm/local.h > > > index bc4bd19b7fc2..45492fb5bf22 100644 > > > --- a/arch/powerpc/include/asm/local.h > > > +++ b/arch/powerpc/include/asm/local.h > > > @@ -90,6 +90,17 @@ static __inline__ long local_cmpxchg(local_t *l, long o, long n) > > > return t; > > > } > > > > > > +static __inline__ bool local_try_cmpxchg(local_t *l, long *po, long n) > > > +{ > > > + long o = *po, r; > > > + > > > + r = local_cmpxchg(l, o, n); > > > + if (unlikely(r != o)) > > > + *po = r; > > > + > > > + return likely(r == o); > > > +} > > > + > > > > Why is the ppc one different from the rest? Why can't it use the > > try_cmpxchg_local() fallback and needs to have it open-coded? > > Please note that ppc directly defines local_cmpxchg that bypasses > cmpxchg_local/arch_cmpxchg_local machinery. The patch takes the same > approach for local_try_cmpxchg, because fallbacks are using > arch_cmpxchg_local definitions. > > PPC should be converted to use arch_cmpxchg_local (to also enable > instrumentation), but this is not the scope of the proposed patchset. Ah indeed. Thanks!
> +static __inline__ bool local_try_cmpxchg(local_t *l, long *old, long new) > +{ > + typeof(l->a.counter) *__old = (typeof(l->a.counter) *) old; > + return try_cmpxchg_local(&l->a.counter, __old, new); > +} > + This patch then causes following sparse errors: ./arch/x86/include/asm/local.h:131:16: warning: symbol '__old' shadows an earlier one ./arch/x86/include/asm/local.h:130:30: originally declared here This is then visible in all kinds of builds - which makes it hard to find out actual problems with sparse.
diff --git a/arch/alpha/include/asm/local.h b/arch/alpha/include/asm/local.h index fab26a1c93d5..0fcaad642cc3 100644 --- a/arch/alpha/include/asm/local.h +++ b/arch/alpha/include/asm/local.h @@ -52,8 +52,16 @@ static __inline__ long local_sub_return(long i, local_t * l) return result; } -#define local_cmpxchg(l, o, n) \ - (cmpxchg_local(&((l)->a.counter), (o), (n))) +static __inline__ long local_cmpxchg(local_t *l, long old, long new) +{ + return cmpxchg_local(&l->a.counter, old, new); +} + +static __inline__ bool local_try_cmpxchg(local_t *l, long *old, long new) +{ + return try_cmpxchg_local(&l->a.counter, (s64 *)old, new); +} + #define local_xchg(l, n) (xchg_local(&((l)->a.counter), (n))) /** diff --git a/arch/loongarch/include/asm/local.h b/arch/loongarch/include/asm/local.h index 65fbbae9fc4d..83e995b30e47 100644 --- a/arch/loongarch/include/asm/local.h +++ b/arch/loongarch/include/asm/local.h @@ -56,8 +56,17 @@ static inline long local_sub_return(long i, local_t *l) return result; } -#define local_cmpxchg(l, o, n) \ - ((long)cmpxchg_local(&((l)->a.counter), (o), (n))) +static inline long local_cmpxchg(local_t *l, long old, long new) +{ + return cmpxchg_local(&l->a.counter, old, new); +} + +static inline bool local_try_cmpxchg(local_t *l, long *old, long new) +{ + typeof(l->a.counter) *__old = (typeof(l->a.counter) *) old; + return try_cmpxchg_local(&l->a.counter, __old, new); +} + #define local_xchg(l, n) (atomic_long_xchg((&(l)->a), (n))) /** diff --git a/arch/mips/include/asm/local.h b/arch/mips/include/asm/local.h index 08366b1fd273..5daf6fe8e3e9 100644 --- a/arch/mips/include/asm/local.h +++ b/arch/mips/include/asm/local.h @@ -94,8 +94,17 @@ static __inline__ long local_sub_return(long i, local_t * l) return result; } -#define local_cmpxchg(l, o, n) \ - ((long)cmpxchg_local(&((l)->a.counter), (o), (n))) +static __inline__ long local_cmpxchg(local_t *l, long old, long new) +{ + return cmpxchg_local(&l->a.counter, old, new); +} + +static __inline__ bool local_try_cmpxchg(local_t *l, long *old, long new) +{ + typeof(l->a.counter) *__old = (typeof(l->a.counter) *) old; + return try_cmpxchg_local(&l->a.counter, __old, new); +} + #define local_xchg(l, n) (atomic_long_xchg((&(l)->a), (n))) /** diff --git a/arch/powerpc/include/asm/local.h b/arch/powerpc/include/asm/local.h index bc4bd19b7fc2..45492fb5bf22 100644 --- a/arch/powerpc/include/asm/local.h +++ b/arch/powerpc/include/asm/local.h @@ -90,6 +90,17 @@ static __inline__ long local_cmpxchg(local_t *l, long o, long n) return t; } +static __inline__ bool local_try_cmpxchg(local_t *l, long *po, long n) +{ + long o = *po, r; + + r = local_cmpxchg(l, o, n); + if (unlikely(r != o)) + *po = r; + + return likely(r == o); +} + static __inline__ long local_xchg(local_t *l, long n) { long t; diff --git a/arch/x86/include/asm/local.h b/arch/x86/include/asm/local.h index 349a47acaa4a..56d4ef604b91 100644 --- a/arch/x86/include/asm/local.h +++ b/arch/x86/include/asm/local.h @@ -120,8 +120,17 @@ static inline long local_sub_return(long i, local_t *l) #define local_inc_return(l) (local_add_return(1, l)) #define local_dec_return(l) (local_sub_return(1, l)) -#define local_cmpxchg(l, o, n) \ - (cmpxchg_local(&((l)->a.counter), (o), (n))) +static inline long local_cmpxchg(local_t *l, long old, long new) +{ + return cmpxchg_local(&l->a.counter, old, new); +} + +static inline bool local_try_cmpxchg(local_t *l, long *old, long new) +{ + typeof(l->a.counter) *__old = (typeof(l->a.counter) *) old; + return try_cmpxchg_local(&l->a.counter, __old, new); +} + /* Always has a lock prefix */ #define local_xchg(l, n) (xchg(&((l)->a.counter), (n)))
Implement target specific support for local_try_cmpxchg and local_cmpxchg using typed C wrappers that call their _local counterpart and provide additional checking of their input arguments. Cc: Richard Henderson <richard.henderson@linaro.org> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru> Cc: Matt Turner <mattst88@gmail.com> Cc: Huacai Chen <chenhuacai@kernel.org> Cc: WANG Xuerui <kernel@xen0n.name> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> Cc: Jun Yi <yijun@loongson.cn> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Signed-off-by: Uros Bizjak <ubizjak@gmail.com> --- arch/alpha/include/asm/local.h | 12 ++++++++++-- arch/loongarch/include/asm/local.h | 13 +++++++++++-- arch/mips/include/asm/local.h | 13 +++++++++++-- arch/powerpc/include/asm/local.h | 11 +++++++++++ arch/x86/include/asm/local.h | 13 +++++++++++-- 5 files changed, 54 insertions(+), 8 deletions(-)