arch/sh/include/asm/cmpxchg.h | 2 ++ 1 file changed, 2 insertions(+)
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 */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 */ >
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 */ > > > -- Masami Hiramatsu (Google) <mhiramat@kernel.org>
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> -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
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, 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 */
> >
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
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 -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
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
--
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 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
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
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 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 */ >>>> >> >> >
© 2016 - 2026 Red Hat, Inc.