| Message ID | 169815917362.8695.13904684741526725648.stgit@devnote2 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local() | expand |
On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > 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 <asm-generic/cmpxchg-local.h> is UP-only (and it only disables interrupts), whereas arch/sh can be built SMP. We should probably add some guards into <asm-generic/cmpxchg-local.h> for that as we have in <asm-generic/cmpxchg.h>. 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 <asm/cmpxchg.h> add: #define arch_cmpxchg_local arch_cmpxchg Mark. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com/ > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > 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 <asm-generic/cmpxchg-local.h> > + > #endif /* __ASM_SH_CMPXCHG_H */ >
On Tue, 2023-10-24 at 23:52 +0900, Masami Hiramatsu (Google) wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > 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 <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com/ > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > 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 <asm-generic/cmpxchg-local.h> > + > #endif /* __ASM_SH_CMPXCHG_H */ Reviewed-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
On Tue, 24 Oct 2023 16:08:12 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > 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 <asm-generic/cmpxchg-local.h> is UP-only (and it only > disables interrupts), whereas arch/sh can be built SMP. We should probably add > some guards into <asm-generic/cmpxchg-local.h> for that as we have in > <asm-generic/cmpxchg.h>. 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 <asm/cmpxchg.h> add: > > #define arch_cmpxchg_local arch_cmpxchg > > Mark. > > > > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com/ > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > --- > > 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 <asm-generic/cmpxchg-local.h> > > + > > #endif /* __ASM_SH_CMPXCHG_H */ > >
On 2023/10/25 07:42, Masami Hiramatsu (Google) wrote: > On Tue, 24 Oct 2023 16:08:12 +0100 > Mark Rutland <mark.rutland@arm.com> wrote: > >> On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: >>> From: Masami Hiramatsu (Google) <mhiramat@kernel.org> >>> >>> 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 <asm-generic/cmpxchg-local.h> is UP-only (and it only >> disables interrupts), whereas arch/sh can be built SMP. We should probably add >> some guards into <asm-generic/cmpxchg-local.h> for that as we have in >> <asm-generic/cmpxchg.h>. > > 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 <asm/cmpxchg-grb.h> #elif defined(CONFIG_CPU_SH4A) #include <asm/cmpxchg-llsc.h> #elif defined(CONFIG_CPU_J2) && defined(CONFIG_SMP) #include <asm/cmpxchg-cas.h> #else #include <asm/cmpxchg-irq.h> #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 <asm/cmpxchg.h> add: >> >> #define arch_cmpxchg_local arch_cmpxchg I agree too. Might not be performance optimized but guarantees correctness. >> Mark. >> >>> >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com/ >>> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> >>> --- >>> 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 <asm-generic/cmpxchg-local.h> >>> + >>> #endif /* __ASM_SH_CMPXCHG_H */ >>> > >
On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote: > On Tue, 24 Oct 2023 16:08:12 +0100 > Mark Rutland <mark.rutland@arm.com> wrote: > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > 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 <asm-generic/cmpxchg-local.h> is UP-only (and it only > > disables interrupts), whereas arch/sh can be built SMP. We should probably add > > some guards into <asm-generic/cmpxchg-local.h> for that as we have in > > <asm-generic/cmpxchg.h>. > > 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.rutland@arm.com> 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 <asm/cmpxchg.h> add: > > > > #define arch_cmpxchg_local arch_cmpxchg > > > > Mark. > > > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com/ > > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > --- > > > 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 <asm-generic/cmpxchg-local.h> > > > + > > > #endif /* __ASM_SH_CMPXCHG_H */ > > > > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
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 <mark.rutland@arm.com> wrote: > > > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > > > 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 <asm-generic/cmpxchg-local.h> is UP-only (and it only > > > disables interrupts), whereas arch/sh can be built SMP. We should probably add > > > some guards into <asm-generic/cmpxchg-local.h> for that as we have in > > > <asm-generic/cmpxchg.h>. > > > > 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.rutland@arm.com> > Geert, what's your opinion on this? Adrian
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 <mark.rutland@arm.com> wrote: >> >>> On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: >>>> From: Masami Hiramatsu (Google) <mhiramat@kernel.org> >>>> >>>> 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 <asm-generic/cmpxchg-local.h> is UP-only (and it only >>> disables interrupts), whereas arch/sh can be built SMP. We should probably add >>> some guards into <asm-generic/cmpxchg-local.h> for that as we have in >>> <asm-generic/cmpxchg.h>. >> >> 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 <asm/cmpxchg-grb.h> > #elif defined(CONFIG_CPU_SH4A) > #include <asm/cmpxchg-llsc.h> > #elif defined(CONFIG_CPU_J2) && defined(CONFIG_SMP) > #include <asm/cmpxchg-cas.h> > #else > #include <asm/cmpxchg-irq.h> > #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 <asm/cmpxchg.h> add: >>> >>> #define arch_cmpxchg_local arch_cmpxchg > > I agree too. Might not be performance optimized but guarantees correctness. > >>> Mark. >>> >>>> >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> Closes: >>>> https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com/ >>>> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> >>>> --- >>>> 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 <asm-generic/cmpxchg-local.h> >>>> + >>>> #endif /* __ASM_SH_CMPXCHG_H */ >>>> >> >> >
On 2023/10/24 22:52, Masami Hiramatsu (Google) wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > 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 <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com/ > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > 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 <asm-generic/cmpxchg-local.h> > + asm-generic/cmpxchg-local.h defines only 2 routines: __generic_cmpxchg_local and __generic_cmpxchg64_local. Shall add the definition of arch_cmpxchg_local into arch/sh/include/asm/cmpxchg.h, or group arch_cmpxchg_local and arch_cmpxchg64_local into asm-generic/cmpxchg-local.h ? > #endif /* __ASM_SH_CMPXCHG_H */ >
Hi Adrian, On Wed, Oct 25, 2023 at 12:32 PM John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> 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 <mark.rutland@arm.com> wrote: > > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > > > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > > > > > 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 <asm-generic/cmpxchg-local.h> is UP-only (and it only > > > > disables interrupts), whereas arch/sh can be built SMP. We should probably add > > > > some guards into <asm-generic/cmpxchg-local.h> for that as we have in > > > > <asm-generic/cmpxchg.h>. > > > > > > 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.rutland@arm.com> > > 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(&slot->tail, &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(&slot->tail, &tail, tail + 1)); | ^~~~~~~~~~~~~~~~~ Hiramatsu-san: do these build for you? Gr{oetje,eeting}s, Geert
On Wed, 25 Oct 2023 15:16:16 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Adrian, > > On Wed, Oct 25, 2023 at 12:32 PM John Paul Adrian Glaubitz > <glaubitz@physik.fu-berlin.de> 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 <mark.rutland@arm.com> wrote: > > > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote: > > > > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > > > > > > > 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 <asm-generic/cmpxchg-local.h> is UP-only (and it only > > > > > disables interrupts), whereas arch/sh can be built SMP. We should probably add > > > > > some guards into <asm-generic/cmpxchg-local.h> for that as we have in > > > > > <asm-generic/cmpxchg.h>. > > > > > > > > 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.rutland@arm.com> > > > > 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(&slot->tail, &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(&slot->tail, &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 -- geert@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
On Wed, 25 Oct 2023 19:26:37 +0800 "wuqiang.matt" <wuqiang.matt@bytedance.com> wrote: > On 2023/10/24 22:52, Masami Hiramatsu (Google) wrote: > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > 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 <lkp@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com/ > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > --- > > 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 <asm-generic/cmpxchg-local.h> > > + > > asm-generic/cmpxchg-local.h defines only 2 routines: __generic_cmpxchg_local > and __generic_cmpxchg64_local. Thanks Wuqiang, I found how I can fix that from your message. > > Shall add the definition of arch_cmpxchg_local into > arch/sh/include/asm/cmpxchg.h, or group arch_cmpxchg_local and > arch_cmpxchg64_local into > asm-generic/cmpxchg-local.h ? No, maybe it depends on the arch that which __generic function need to use. Thank you, > > > #endif /* __ASM_SH_CMPXCHG_H */ > > >
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 <asm-generic/cmpxchg-local.h> + #endif /* __ASM_SH_CMPXCHG_H */