Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
On Wed, 25 Oct 2023 15:16:16 +0200 Geert Uytterhoeven wrote: > Hi Adrian, > > On Wed, Oct 25, 2023 at 12:32 PM John Paul Adrian Glaubitz > wrote: > > On Wed, 2023-10-25 at 11:30 +0100, Mark Rutland wrote: > > > On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote: > > > > On Tue, 24 Oct 2023 16:08:12 +0100 > > > > Mark Rutland wrote: > > > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) > > > > > wrote: > > > > > > From: Masami Hiramatsu (Google) > > > > > > > > > > > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation > > > > > > in SH architecture because it does not implement > > > > > > arch_cmpxchg_local(). > > > > > > > > > > I do not think this is correct. > > > > > > > > > > The implementation in is UP-only (and > > > > > it only > > > > > disables interrupts), whereas arch/sh can be built SMP. We should > > > > > probably add > > > > > some guards into for that as we have in > > > > > . > > > > > > > > Isn't cmpxchg_local for the data which only needs to ensure to do > > > > cmpxchg > > > > on local CPU? > > > > So I think it doesn't care about the other CPUs (IOW, it should not > > > > touched by > > > > other CPUs), so it only considers UP case. E.g. on x86, > > > > arch_cmpxchg_local() is > > > > defined as raw "cmpxchg" without lock prefix. > > > > > > > > #define __cmpxchg_local(ptr, old, new, size) > > > > \ > > > > __raw_cmpxchg((ptr), (old), (new), (size), "") > > > > > > > > > > Yes, you're right; sorry for the noise. > > > > > > For your original patch: > > > > > > Acked-by: Mark Rutland > > > > Geert, what's your opinion on this? > > While this looks OK on first sight (ARM includes the same file, even > on SMP), it does not seem to work? > > For sh-allnoconfig, as reported by kernel test robot: > > $ make ARCH=sh CROSS_COMPILE=sh2-linux- allnoconfig lib/objpool.o > lib/objpool.c: In function 'objpool_try_add_slot': > ./include/linux/atomic/atomic-arch-fallback.h:384:27: error: implicit > declaration of function 'arch_cmpxchg_local'; did you mean > 'raw_cmpxchg_local'? [-Werror=implicit-function-declaration] > 384 | #define raw_cmpxchg_local arch_cmpxchg_local > | ^~ > ./include/linux/atomic/atomic-arch-fallback.h:392:16: note: in > expansion of macro 'raw_cmpxchg_local' > 392 | ___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \ > |^ > ./include/linux/atomic/atomic-instrumented.h:4980:9: note: in > expansion of macro 'raw_try_cmpxchg_local' > 4980 | raw_try_cmpxchg_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \ > | ^ > lib/objpool.c:169:19: note: in expansion of macro 'try_cmpxchg_local' > 169 | } while (!try_cmpxchg_local(>tail, , tail + 1)); > | ^ > > For an SMP defconfig: > > $ make ARCH=sh CROSS_COMPILE=sh4-linux-gnu- sdk7786_defconfig lib/objpool.o > > ./include/linux/atomic/atomic-arch-fallback.h:384:27: error: implicit > declaration of function ‘arch_cmpxchg_local’; did you mean > ‘try_cmpxchg_local’? [-Werror=implicit-function-declaration] > 384 | #define raw_cmpxchg_local arch_cmpxchg_local > | ^~ > ./include/linux/atomic/atomic-arch-fallback.h:392:16: note: in > expansion of macro ‘raw_cmpxchg_local’ > 392 | ___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \ > |^ > ./include/linux/atomic/atomic-instrumented.h:4980:9: note: in > expansion of macro ‘raw_try_cmpxchg_local’ > 4980 | raw_try_cmpxchg_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \ > | ^ > lib/objpool.c:169:19: note: in expansion of macro ‘try_cmpxchg_local’ > 169 | } while (!try_cmpxchg_local(>tail, , tail + 1)); > | ^ > > Hiramatsu-san: do these build for you? Thanks for pointing. I thought I just need to include the header file. That's my fault. Let me fix that. Thank you! > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > ge...@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like > that. > -- Linus Torvalds -- Masami Hiramatsu (Google)
Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
Hi Adrian, On Wed, Oct 25, 2023 at 12:32 PM John Paul Adrian Glaubitz wrote: > On Wed, 2023-10-25 at 11:30 +0100, Mark Rutland wrote: > > On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote: > > > On Tue, 24 Oct 2023 16:08:12 +0100 > > > Mark Rutland wrote: > > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) > > > > wrote: > > > > > From: Masami Hiramatsu (Google) > > > > > > > > > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation > > > > > in SH architecture because it does not implement arch_cmpxchg_local(). > > > > > > > > I do not think this is correct. > > > > > > > > The implementation in is UP-only (and it > > > > only > > > > disables interrupts), whereas arch/sh can be built SMP. We should > > > > probably add > > > > some guards into for that as we have in > > > > . > > > > > > Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg > > > on local CPU? > > > So I think it doesn't care about the other CPUs (IOW, it should not > > > touched by > > > other CPUs), so it only considers UP case. E.g. on x86, > > > arch_cmpxchg_local() is > > > defined as raw "cmpxchg" without lock prefix. > > > > > > #define __cmpxchg_local(ptr, old, new, size)\ > > > __raw_cmpxchg((ptr), (old), (new), (size), "") > > > > > > > Yes, you're right; sorry for the noise. > > > > For your original patch: > > > > Acked-by: Mark Rutland > > Geert, what's your opinion on this? While this looks OK on first sight (ARM includes the same file, even on SMP), it does not seem to work? For sh-allnoconfig, as reported by kernel test robot: $ make ARCH=sh CROSS_COMPILE=sh2-linux- allnoconfig lib/objpool.o lib/objpool.c: In function 'objpool_try_add_slot': ./include/linux/atomic/atomic-arch-fallback.h:384:27: error: implicit declaration of function 'arch_cmpxchg_local'; did you mean 'raw_cmpxchg_local'? [-Werror=implicit-function-declaration] 384 | #define raw_cmpxchg_local arch_cmpxchg_local | ^~ ./include/linux/atomic/atomic-arch-fallback.h:392:16: note: in expansion of macro 'raw_cmpxchg_local' 392 | ___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \ |^ ./include/linux/atomic/atomic-instrumented.h:4980:9: note: in expansion of macro 'raw_try_cmpxchg_local' 4980 | raw_try_cmpxchg_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \ | ^ lib/objpool.c:169:19: note: in expansion of macro 'try_cmpxchg_local' 169 | } while (!try_cmpxchg_local(>tail, , tail + 1)); | ^ For an SMP defconfig: $ make ARCH=sh CROSS_COMPILE=sh4-linux-gnu- sdk7786_defconfig lib/objpool.o ./include/linux/atomic/atomic-arch-fallback.h:384:27: error: implicit declaration of function ‘arch_cmpxchg_local’; did you mean ‘try_cmpxchg_local’? [-Werror=implicit-function-declaration] 384 | #define raw_cmpxchg_local arch_cmpxchg_local | ^~ ./include/linux/atomic/atomic-arch-fallback.h:392:16: note: in expansion of macro ‘raw_cmpxchg_local’ 392 | ___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \ |^ ./include/linux/atomic/atomic-instrumented.h:4980:9: note: in expansion of macro ‘raw_try_cmpxchg_local’ 4980 | raw_try_cmpxchg_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \ | ^ lib/objpool.c:169:19: note: in expansion of macro ‘try_cmpxchg_local’ 169 | } while (!try_cmpxchg_local(>tail, , tail + 1)); | ^ Hiramatsu-san: do these build for you? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
On 2023/10/25 09:51, wuqiang.matt wrote: On 2023/10/25 07:42, Masami Hiramatsu (Google) wrote: On Tue, 24 Oct 2023 16:08:12 +0100 Mark Rutland wrote: On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: From: Masami Hiramatsu (Google) Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation in SH architecture because it does not implement arch_cmpxchg_local(). I do not think this is correct. The implementation in is UP-only (and it only disables interrupts), whereas arch/sh can be built SMP. We should probably add some guards into for that as we have in . Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg on local CPU? asm-generic/cmpxchg.h is only for UP, will throw an error for SMP building: #ifdef CONFIG_SMP #error "Cannot use generic cmpxchg on SMP" #endif Sorry that I just noticed Masami's patch has asm-generic/cmpxchg-local.h included, not asm-generic/cmpxchg.h. cmpxchg.h does throw an error for SMP configs, but cmpxchg-local.h doesn't. SH arch seems it does have SMP systems. The arch/sh/include/asm/cmpxchg.h has the following codes: #if defined(CONFIG_GUSA_RB) #include #elif defined(CONFIG_CPU_SH4A) #include #elif defined(CONFIG_CPU_J2) && defined(CONFIG_SMP) #include #else #include #endif So I think it doesn't care about the other CPUs (IOW, it should not touched by other CPUs), so it only considers UP case. E.g. on x86, arch_cmpxchg_local() is defined as raw "cmpxchg" without lock prefix. #define __cmpxchg_local(ptr, old, new, size) \ __raw_cmpxchg((ptr), (old), (new), (size), "") Thank you, I think the right thing to do here is to define arch_cmpxchg_local() in terms of arch_cmpxchg(), i.e. at the bottom of arch/sh's add: #define arch_cmpxchg_local arch_cmpxchg I agree too. Might not be performance optimized but guarantees correctness. Mark. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/ Signed-off-by: Masami Hiramatsu (Google) --- arch/sh/include/asm/cmpxchg.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h index 288f6f38d98f..e920e61fb817 100644 --- a/arch/sh/include/asm/cmpxchg.h +++ b/arch/sh/include/asm/cmpxchg.h @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old, (unsigned long)_n_, sizeof(*(ptr))); \ }) +#include + #endif /* __ASM_SH_CMPXCHG_H */
Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
On Wed, 2023-10-25 at 11:30 +0100, Mark Rutland wrote: > On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote: > > On Tue, 24 Oct 2023 16:08:12 +0100 > > Mark Rutland wrote: > > > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > > > > From: Masami Hiramatsu (Google) > > > > > > > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation > > > > in SH architecture because it does not implement arch_cmpxchg_local(). > > > > > > I do not think this is correct. > > > > > > The implementation in is UP-only (and it > > > only > > > disables interrupts), whereas arch/sh can be built SMP. We should > > > probably add > > > some guards into for that as we have in > > > . > > > > Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg > > on local CPU? > > So I think it doesn't care about the other CPUs (IOW, it should not touched > > by > > other CPUs), so it only considers UP case. E.g. on x86, > > arch_cmpxchg_local() is > > defined as raw "cmpxchg" without lock prefix. > > > > #define __cmpxchg_local(ptr, old, new, size)\ > > __raw_cmpxchg((ptr), (old), (new), (size), "") > > > > Yes, you're right; sorry for the noise. > > For your original patch: > > Acked-by: Mark Rutland > Geert, what's your opinion on this? Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote: > On Tue, 24 Oct 2023 16:08:12 +0100 > Mark Rutland wrote: > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > > > From: Masami Hiramatsu (Google) > > > > > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation > > > in SH architecture because it does not implement arch_cmpxchg_local(). > > > > I do not think this is correct. > > > > The implementation in is UP-only (and it only > > disables interrupts), whereas arch/sh can be built SMP. We should probably > > add > > some guards into for that as we have in > > . > > Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg > on local CPU? > So I think it doesn't care about the other CPUs (IOW, it should not touched by > other CPUs), so it only considers UP case. E.g. on x86, arch_cmpxchg_local() > is > defined as raw "cmpxchg" without lock prefix. > > #define __cmpxchg_local(ptr, old, new, size)\ > __raw_cmpxchg((ptr), (old), (new), (size), "") > Yes, you're right; sorry for the noise. For your original patch: Acked-by: Mark Rutland Mark. > > Thank you, > > > > > > I think the right thing to do here is to define arch_cmpxchg_local() in > > terms > > of arch_cmpxchg(), i.e. at the bottom of arch/sh's add: > > > > #define arch_cmpxchg_local arch_cmpxchg > > > > Mark. > > > > > > > > Reported-by: kernel test robot > > > Closes: > > > https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/ > > > Signed-off-by: Masami Hiramatsu (Google) > > > --- > > > arch/sh/include/asm/cmpxchg.h |2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h > > > index 288f6f38d98f..e920e61fb817 100644 > > > --- a/arch/sh/include/asm/cmpxchg.h > > > +++ b/arch/sh/include/asm/cmpxchg.h > > > @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * > > > ptr, unsigned long old, > > > (unsigned long)_n_, sizeof(*(ptr))); \ > > >}) > > > > > > +#include > > > + > > > #endif /* __ASM_SH_CMPXCHG_H */ > > > > > > -- > Masami Hiramatsu (Google)
Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
On 2023/10/25 07:42, Masami Hiramatsu (Google) wrote: On Tue, 24 Oct 2023 16:08:12 +0100 Mark Rutland wrote: On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: From: Masami Hiramatsu (Google) Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation in SH architecture because it does not implement arch_cmpxchg_local(). I do not think this is correct. The implementation in is UP-only (and it only disables interrupts), whereas arch/sh can be built SMP. We should probably add some guards into for that as we have in . Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg on local CPU? asm-generic/cmpxchg.h is only for UP, will throw an error for SMP building: #ifdef CONFIG_SMP #error "Cannot use generic cmpxchg on SMP" #endif SH arch seems it does have SMP systems. The arch/sh/include/asm/cmpxchg.h has the following codes: #if defined(CONFIG_GUSA_RB) #include #elif defined(CONFIG_CPU_SH4A) #include #elif defined(CONFIG_CPU_J2) && defined(CONFIG_SMP) #include #else #include #endif So I think it doesn't care about the other CPUs (IOW, it should not touched by other CPUs), so it only considers UP case. E.g. on x86, arch_cmpxchg_local() is defined as raw "cmpxchg" without lock prefix. #define __cmpxchg_local(ptr, old, new, size)\ __raw_cmpxchg((ptr), (old), (new), (size), "") Thank you, I think the right thing to do here is to define arch_cmpxchg_local() in terms of arch_cmpxchg(), i.e. at the bottom of arch/sh's add: #define arch_cmpxchg_local arch_cmpxchg I agree too. Might not be performance optimized but guarantees correctness. Mark. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/ Signed-off-by: Masami Hiramatsu (Google) --- arch/sh/include/asm/cmpxchg.h |2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h index 288f6f38d98f..e920e61fb817 100644 --- a/arch/sh/include/asm/cmpxchg.h +++ b/arch/sh/include/asm/cmpxchg.h @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old, (unsigned long)_n_, sizeof(*(ptr))); \ }) +#include + #endif /* __ASM_SH_CMPXCHG_H */
Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
On Tue, 24 Oct 2023 16:08:12 +0100 Mark Rutland wrote: > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > > From: Masami Hiramatsu (Google) > > > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation > > in SH architecture because it does not implement arch_cmpxchg_local(). > > I do not think this is correct. > > The implementation in is UP-only (and it only > disables interrupts), whereas arch/sh can be built SMP. We should probably add > some guards into for that as we have in > . Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg on local CPU? So I think it doesn't care about the other CPUs (IOW, it should not touched by other CPUs), so it only considers UP case. E.g. on x86, arch_cmpxchg_local() is defined as raw "cmpxchg" without lock prefix. #define __cmpxchg_local(ptr, old, new, size)\ __raw_cmpxchg((ptr), (old), (new), (size), "") Thank you, > > I think the right thing to do here is to define arch_cmpxchg_local() in terms > of arch_cmpxchg(), i.e. at the bottom of arch/sh's add: > > #define arch_cmpxchg_local arch_cmpxchg > > Mark. > > > > > Reported-by: kernel test robot > > Closes: > > https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/ > > Signed-off-by: Masami Hiramatsu (Google) > > --- > > arch/sh/include/asm/cmpxchg.h |2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h > > index 288f6f38d98f..e920e61fb817 100644 > > --- a/arch/sh/include/asm/cmpxchg.h > > +++ b/arch/sh/include/asm/cmpxchg.h > > @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * > > ptr, unsigned long old, > > (unsigned long)_n_, sizeof(*(ptr))); \ > >}) > > > > +#include > > + > > #endif /* __ASM_SH_CMPXCHG_H */ > > -- Masami Hiramatsu (Google)
Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
On Tue, 2023-10-24 at 23:52 +0900, Masami Hiramatsu (Google) wrote: > From: Masami Hiramatsu (Google) > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation > in SH architecture because it does not implement arch_cmpxchg_local(). > > Reported-by: kernel test robot > Closes: > https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/ > Signed-off-by: Masami Hiramatsu (Google) > --- > arch/sh/include/asm/cmpxchg.h |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h > index 288f6f38d98f..e920e61fb817 100644 > --- a/arch/sh/include/asm/cmpxchg.h > +++ b/arch/sh/include/asm/cmpxchg.h > @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * ptr, > unsigned long old, > (unsigned long)_n_, sizeof(*(ptr))); \ >}) > > +#include > + > #endif /* __ASM_SH_CMPXCHG_H */ Reviewed-by: John Paul Adrian Glaubitz -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()
On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > From: Masami Hiramatsu (Google) > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation > in SH architecture because it does not implement arch_cmpxchg_local(). I do not think this is correct. The implementation in is UP-only (and it only disables interrupts), whereas arch/sh can be built SMP. We should probably add some guards into for that as we have in . I think the right thing to do here is to define arch_cmpxchg_local() in terms of arch_cmpxchg(), i.e. at the bottom of arch/sh's add: #define arch_cmpxchg_local arch_cmpxchg Mark. > > Reported-by: kernel test robot > Closes: > https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/ > Signed-off-by: Masami Hiramatsu (Google) > --- > arch/sh/include/asm/cmpxchg.h |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h > index 288f6f38d98f..e920e61fb817 100644 > --- a/arch/sh/include/asm/cmpxchg.h > +++ b/arch/sh/include/asm/cmpxchg.h > @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * ptr, > unsigned long old, > (unsigned long)_n_, sizeof(*(ptr))); \ >}) > > +#include > + > #endif /* __ASM_SH_CMPXCHG_H */ >