Re: [PATCH 3/3] locking/atomic: openrisc: use generic_cmpxchg[64]_local for arch_cmpxchg[64]_local
On 2023/10/28 20:49, Masami Hiramatsu (Google) wrote:
Hi Wuqiang,
On Thu, 26 Oct 2023 19:05:51 +0800
"wuqiang.matt" wrote:
On 2023/10/26 16:46, Arnd Bergmann wrote:
On Thu, Oct 26, 2023, at 09:39, wuqiang.matt wrote:
arch_cmpxchg[64]_local() are not defined for openrisc. So implement
them with generci_cmpxchg[64]_local, advised by Masami Hiramatsu.
Closes:
https://lore.kernel.org/linux-trace-kernel/169824660459.24340.14614817132696360531.stgit@devnote2
Closes:
https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com
Signed-off-by: wuqiang.matt
I think on architectures that have actual atomics, you
generally want to define this to be the same as arch_cmpxchg()
rather than the generic version.
It depends on the relative cost of doing one atomic compared
to an irq-disable/enable pair, but everyone else went with
the former if they could. The exceptions are armv4/armv5,
sparc32 and parisc, which don't have a generic cmpxchg()
or similar operation.
Sure, better native than the generic. I'll try to collect more
insights before next move.
So I will temporally remove the last change (use arch_cmpxchg_local
in objpool) until these series are rewritten with arch native code,
so that the next release will not break the kernel build.
Ok, it's fine to me. Thank you.
But this must be fixed because arch_cmpxchg_local() is required
for each arch anyway.
Yes. I'm working on the new update for arc/openrisc/hexagon. It would
be better resolve this issue first, then consider the objpool update
of using arch_cmpxchg_local.
You could do the thing that sparc64 and xtensa do, which
use the native cmpxchg for supported word sizes but the
generic version for 1- and 2-byte swaps, but that has its
own set of problems if you end up doing operations on both
the entire word and a sub-unit of the same thing.
Thank you for pointing out this. I'll do some research on these
implementations.
arc also has the LL-SC instruction but depends on the core feature,
so I think we can use it.
Right. The arc processor does have the CONFIG_ARC_HAS_LLSC option, but
I doubt the correctness of arch_cmpxchg_relaxed and arch_cmpxchg:
arch_cmpxchg_relaxed:
...
switch(sizeof((_p_))) {
case 4:
arch_cmpxchg:
...
BUILD_BUG_ON(sizeof(_p_) != 4);
...
_p is the address pointer, so I'm thinking it's a typo but I couldn't
yet confirm. There is not much about arc processors in the web :(
Thank you,
Arnd
Regards,
wuqiang
Re: [PATCH 3/3] locking/atomic: openrisc: use generic_cmpxchg[64]_local for arch_cmpxchg[64]_local
On 2023/10/30 10:22, Vineet Gupta wrote:
On 10/28/23 20:26, Masami Hiramatsu (Google) wrote:
On Sun, 29 Oct 2023 00:40:17 +0800
"wuqiang.matt" wrote:
On 2023/10/28 20:49, Masami Hiramatsu (Google) wrote:
Hi Wuqiang,
On Thu, 26 Oct 2023 19:05:51 +0800
"wuqiang.matt" wrote:
On 2023/10/26 16:46, Arnd Bergmann wrote:
On Thu, Oct 26, 2023, at 09:39, wuqiang.matt wrote:
arch_cmpxchg[64]_local() are not defined for openrisc. So implement
them with generci_cmpxchg[64]_local, advised by Masami Hiramatsu.
Closes:
https://lore.kernel.org/linux-trace-kernel/169824660459.24340.14614817132696360531.stgit@devnote2
Closes:
https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com
Signed-off-by: wuqiang.matt
I think on architectures that have actual atomics, you
generally want to define this to be the same as arch_cmpxchg()
rather than the generic version.
It depends on the relative cost of doing one atomic compared
to an irq-disable/enable pair, but everyone else went with
the former if they could. The exceptions are armv4/armv5,
sparc32 and parisc, which don't have a generic cmpxchg()
or similar operation.
Sure, better native than the generic. I'll try to collect more
insights before next move.
So I will temporally remove the last change (use arch_cmpxchg_local
in objpool) until these series are rewritten with arch native code,
so that the next release will not break the kernel build.
Ok, it's fine to me. Thank you.
But this must be fixed because arch_cmpxchg_local() is required
for each arch anyway.
Yes. I'm working on the new update for arc/openrisc/hexagon. It would
be better resolve this issue first, then consider the objpool update
of using arch_cmpxchg_local.
You could do the thing that sparc64 and xtensa do, which
use the native cmpxchg for supported word sizes but the
generic version for 1- and 2-byte swaps, but that has its
own set of problems if you end up doing operations on both
the entire word and a sub-unit of the same thing.
Thank you for pointing out this. I'll do some research on these
implementations.
arc also has the LL-SC instruction but depends on the core feature,
so I think we can use it.
Right. The arc processor does have the CONFIG_ARC_HAS_LLSC option, but
I doubt the correctness of arch_cmpxchg_relaxed and arch_cmpxchg:
arch_cmpxchg_relaxed:
...
switch(sizeof((_p_))) {
case 4:
arch_cmpxchg:
...
BUILD_BUG_ON(sizeof(_p_) != 4);
...
_p is the address pointer, so I'm thinking it's a typo but I couldn't
yet confirm. There is not much about arc processors in the web :(
Hmm, indeed. This seems like a bug but it depends on the 'llock %0, [%1]'
can take a 32bit address or 32bit data register. Usually it should
check the size of data, but need to check with ISA manual.
Vineet, can you check this suspicious bug?
ARCv2 is a 32-bit ISA and LLOCK/SCOND work on 32-bit data.
So the pointers will be 32-bit anyways. Is the issue that pointer/cmpxchg
operation could be on a smaller data type ?
For ARCv2 with CONFIG_ARC_HAS_LLSC, better add the data size checking and
only permit 32bit data size. Even for 32-bit system, data should can be
64bit 'long long'.
And In the case that CONFIG_ARC_HAS_LLSC is undefined, in arch_cmpxchg: the
pointer size checking is unnecessary, since it's using spinlock internally:
https://elixir.bootlin.com/linux/v6.6-rc7/source/arch/arc/include/asm/cmpxchg.h#L60:
BUILD_BUG_ON(sizeof(_p_) != 4); \
\
/* \
* spin lock/unlock provide the needed smp_mb() before/after\
*/ \
atomic_ops_lock(__flags); \
_prev_ = *_p_; \
if (_prev_ == _o_) \
*_p_ = _n_; \
atomic_ops_unlock(__flags);
Another question about the naming: arch_cmpxchg_relaxed() implemented if
CONFIG_ARC_HAS_LLSC is configured and arch_cmpxchg() defined for the rest.
Are there any reasons for difference names ?
As I checked, Synopsys has released 64bit ARC processors (HS66/HS68), but
I don't know the status of Linux kernel support.
-Vineet
Regards,
wuqiang
Re: [PATCH 3/3] locking/atomic: openrisc: use generic_cmpxchg[64]_local for arch_cmpxchg[64]_local
On 2023/11/2 12:53, Vineet Gupta wrote:
On 10/29/23 20:41, wuqiang.matt wrote:
arch_cmpxchg_relaxed:
...
switch(sizeof((_p_))) {
case 4:
arch_cmpxchg:
...
BUILD_BUG_ON(sizeof(_p_) != 4);
...
_p is the address pointer, so I'm thinking it's a typo but I couldn't
yet confirm. There is not much about arc processors in the web :(
Hmm, indeed. This seems like a bug but it depends on the 'llock %0, [%1]'
can take a 32bit address or 32bit data register. Usually it should
check the size of data, but need to check with ISA manual.
Vineet, can you check this suspicious bug?
ARCv2 is a 32-bit ISA and LLOCK/SCOND work on 32-bit data.
So the pointers will be 32-bit anyways. Is the issue that pointer/cmpxchg
operation could be on a smaller data type ?
For ARCv2 with CONFIG_ARC_HAS_LLSC, better add the data size checking and
only permit 32bit data size. Even for 32-bit system, data should can be
64bit 'long long'.
Right, makes sense. Can you send a patch ?
Okay, I will.
And In the case that CONFIG_ARC_HAS_LLSC is undefined, in arch_cmpxchg: the
pointer size checking is unnecessary, since it's using spinlock internally:
Correct, I had the same thought.
https://elixir.bootlin.com/linux/v6.6-rc7/source/arch/arc/include/asm/cmpxchg.h#L60:
BUILD_BUG_ON(sizeof(_p_) != 4); \
\
/* \
* spin lock/unlock provide the needed smp_mb() before/after \
*/ \
atomic_ops_lock(__flags); \
_prev_ = *_p_; \
if (_prev_ == _o_) \
*_p_ = _n_; \
atomic_ops_unlock(__flags);
Can you do that fix in same patch as well ?
Sure.
Another question about the naming: arch_cmpxchg_relaxed() implemented if
CONFIG_ARC_HAS_LLSC is configured and arch_cmpxchg() defined for the rest.
Are there any reasons for difference names ?
Yes the atomics API consists of _relaxed, _acquire, _release and unadorned.
_relaxed is the most efficient, with rest having some extra barriers.
If arch provides _relaxed, generic code can create the rest - assuming arch
hardware can support the relaxed ones.
Definitely Yes! The generic cmpxchg will prefer arch_cmpxchg_relaxed rather
than arch_cmpxchg.
That is true for LLSC.
However for !LLSC, spinlock versions already have full barriers due to
spinlock, so relaxed variant doesn't make sense.
As I checked, Synopsys has released 64bit ARC processors (HS66/HS68), but
I don't know the status of Linux kernel support.
Yes there's an internal ARC64 port running but it is not ready for upstreaming
yet.
Thanks for the update.
-Vineet
Regards,
wuqiang
Re: [PATCH 3/3] locking/atomic: openrisc: use generic_cmpxchg[64]_local for arch_cmpxchg[64]_local
On 2023/11/2 17:30, wuqiang.matt wrote:
On 2023/11/2 12:53, Vineet Gupta wrote:
On 10/29/23 20:41, wuqiang.matt wrote:
arch_cmpxchg_relaxed:
...
switch(sizeof((_p_))) {
case 4:
arch_cmpxchg:
...
BUILD_BUG_ON(sizeof(_p_) != 4);
...
_p is the address pointer, so I'm thinking it's a typo but I couldn't
yet confirm. There is not much about arc processors in the web :(
Hmm, indeed. This seems like a bug but it depends on the 'llock %0, [%1]'
can take a 32bit address or 32bit data register. Usually it should
check the size of data, but need to check with ISA manual.
Vineet, can you check this suspicious bug?
ARCv2 is a 32-bit ISA and LLOCK/SCOND work on 32-bit data.
So the pointers will be 32-bit anyways. Is the issue that pointer/cmpxchg
operation could be on a smaller data type ?
For ARCv2 with CONFIG_ARC_HAS_LLSC, better add the data size checking and
only permit 32bit data size. Even for 32-bit system, data should can be
64bit 'long long'.
Right, makes sense. Can you send a patch ?
Okay, I will.
And In the case that CONFIG_ARC_HAS_LLSC is undefined, in arch_cmpxchg: the
pointer size checking is unnecessary, since it's using spinlock internally:
Correct, I had the same thought.
https://elixir.bootlin.com/linux/v6.6-rc7/source/arch/arc/include/asm/cmpxchg.h#L60:
BUILD_BUG_ON(sizeof(_p_) != 4); \
\
/* \
* spin lock/unlock provide the needed smp_mb() before/after \
*/ \
atomic_ops_lock(__flags); \
_prev_ = *_p_; \
if (_prev_ == _o_) \
*_p_ = _n_; \
atomic_ops_unlock(__flags);
Can you do that fix in same patch as well ?
Sure.
Another question about the naming: arch_cmpxchg_relaxed() implemented if
CONFIG_ARC_HAS_LLSC is configured and arch_cmpxchg() defined for the rest.
Are there any reasons for difference names ?
Yes the atomics API consists of _relaxed, _acquire, _release and unadorned.
_relaxed is the most efficient, with rest having some extra barriers.
If arch provides _relaxed, generic code can create the rest - assuming arch
hardware can support the relaxed ones.
Definitely Yes! The generic cmpxchg will prefer arch_cmpxchg_relaxed rather
than arch_cmpxchg.
It should be arch_cmpxchg (if defined) rather than arch_cmpxchg_relaxed. I
just double checked with v6.6:
...
#if defined(arch_cmpxchg)
#define raw_cmpxchg arch_cmpxchg
#elif defined(arch_cmpxchg_relaxed)
#define raw_cmpxchg(...) \
__atomic_op_fence(arch_cmpxchg, __VA_ARGS__)
#else
extern void raw_cmpxchg_not_implemented(void);
#define raw_cmpxchg(...) raw_cmpxchg_not_implemented()
#endif
...
#if defined(arch_cmpxchg_relaxed)
#define raw_cmpxchg_relaxed arch_cmpxchg_relaxed
#elif defined(arch_cmpxchg)
#define raw_cmpxchg_relaxed arch_cmpxchg
#else
extern void raw_cmpxchg_relaxed_not_implemented(void);
#define raw_cmpxchg_relaxed(...) raw_cmpxchg_relaxed_not_implemented()
#endif
...
I made the mistake since the definitions of v5.2 confused me. The followings
are for v5.2, and these conditions are removed from v5.14:
...
#if !defined(arch_cmpxchg_relaxed) || defined(arch_cmpxchg)
#define cmpxchg(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
})
#endif
...
#if defined(arch_cmpxchg_relaxed)
#define cmpxchg_relaxed(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg_relaxed(__ai_ptr, __VA_ARGS__); \
})
#endif
...
Sorry for the noise.
That is true for LLSC.
However for !LLSC, spinlock versions already have full barriers due to
spinlock, so relaxed variant doesn't make sense.
As I checked, Synopsys has released 64bit ARC processors (HS66/HS68), but
I don't know the status of Linux kernel support.
Yes there's an internal ARC64 port running but it is not ready for
upstreaming yet.
Thanks for the update.
-Vineet
Regards,
wuqiang
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v2 0/4] locking/atomic: arch_cmpxchg[64]_local undefined
This patch series implement arch_cmpxchg[64]_local for arc, openrisc and hexagon. For architectures that support native cmpxchg, we'd like to implement arch_cmpxchg[64]_local with the native variants of supported data size. If not, the generci_cmpxchg[64]_local will be used. wuqiang.matt (4): locking/atomic: arc: data size checking in arch_cmpxchg locking/atomic: arc: arch_cmpxchg[64]_local undefined locking/atomic: openrisc: arch_cmpxchg[64]_local undefined locking/atomic: hexagon: arch_cmpxchg[64]_local undefined arch/arc/include/asm/cmpxchg.h | 40 ++ arch/hexagon/include/asm/cmpxchg.h | 51 - arch/openrisc/include/asm/cmpxchg.h | 6 3 files changed, 90 insertions(+), 7 deletions(-) -- 2.40.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v2 1/4] locking/atomic: arc: data size checking in arch_cmpxchg
Macro __cmpxchg() renamed to __cmpxchg_32() to emphasize it's explicit
support of 32bit data size, BUILD_BUG_ON() added to avoid any possible
misuses with unsupported data types.
In case CONFIG_ARC_HAS_LLSC is undefined, arch_cmpxchg() uses spinlock
to accomplish SMP-safety, so the BUILD_BUG_ON checking is uncecessary.
Signed-off-by: wuqiang.matt
---
arch/arc/include/asm/cmpxchg.h | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arc/include/asm/cmpxchg.h b/arch/arc/include/asm/cmpxchg.h
index e138fde067de..bf46514f6f12 100644
--- a/arch/arc/include/asm/cmpxchg.h
+++ b/arch/arc/include/asm/cmpxchg.h
@@ -18,14 +18,16 @@
* if (*ptr == @old)
* *ptr = @new
*/
-#define __cmpxchg(ptr, old, new) \
+#define __cmpxchg_32(ptr, old, new)\
({ \
__typeof__(*(ptr)) _prev; \
\
+ BUILD_BUG_ON(sizeof(*(ptr)) != 4); \
+ \
__asm__ __volatile__( \
- "1: llock %0, [%1] \n" \
+ "1: llock %0, [%1] \n" \
" brne %0, %2, 2f \n" \
- " scond %3, [%1] \n" \
+ " scond %3, [%1] \n" \
" bnz 1b \n" \
"2: \n" \
: "=&r"(_prev) /* Early clobber prevent reg reuse */ \
@@ -47,7 +49,7 @@
\
switch(sizeof((_p_))) { \
case 4: \
- _prev_ = __cmpxchg(_p_, _o_, _n_); \
+ _prev_ = __cmpxchg_32(_p_, _o_, _n_); \
break; \
default:\
BUILD_BUG();\
@@ -65,8 +67,6 @@
__typeof__(*(ptr)) _prev_; \
unsigned long __flags; \
\
- BUILD_BUG_ON(sizeof(_p_) != 4); \
- \
/* \
* spin lock/unlock provide the needed smp_mb() before/after\
*/ \
--
2.40.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v2 2/4] locking/atomic: arc: arch_cmpxchg[64]_local undefined
For architectures that support native cmpxchg, we'd like to
implement arch_cmpxchg[64]_local with the native variants of
supported data size. If not, the generci_cmpxchg[64]_local
will be used.
Signed-off-by: wuqiang.matt
---
arch/arc/include/asm/cmpxchg.h | 28
1 file changed, 28 insertions(+)
diff --git a/arch/arc/include/asm/cmpxchg.h b/arch/arc/include/asm/cmpxchg.h
index bf46514f6f12..91429f2350df 100644
--- a/arch/arc/include/asm/cmpxchg.h
+++ b/arch/arc/include/asm/cmpxchg.h
@@ -80,6 +80,34 @@
#endif
+/*
+ * always make arch_cmpxchg[64]_local available, native cmpxchg
+ * will be used if available, then generic_cmpxchg[64]_local
+ */
+#include
+static inline unsigned long __cmpxchg_local(volatile void *ptr,
+ unsigned long old,
+ unsigned long new, int size)
+{
+ switch (size) {
+#ifdef CONFIG_ARC_HAS_LLSC
+ case 4:
+ return __cmpxchg_32((int32_t *)ptr, old, new);
+#endif
+ default:
+ return __generic_cmpxchg_local(ptr, old, new, size);
+ }
+
+ return old;
+}
+#define arch_cmpxchg_local(ptr, o, n) ({ \
+ (__typeof__(*ptr))__cmpxchg_local((ptr),\
+ (unsigned long)(o), \
+ (unsigned long)(n), \
+ sizeof(*(ptr)));\
+})
+#define arch_cmpxchg64_local(ptr, o, n) __generic_cmpxchg64_local((ptr), (o),
(n))
+
/*
* xchg
*/
--
2.40.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v2 3/4] locking/atomic: openrisc: arch_cmpxchg[64]_local undefined
For architectures that support native cmpxchg, we'd like to implement arch_cmpxchg[64]_local with the native variants of supported data size. If not, the generci_cmpxchg[64]_local will be used. Signed-off-by: wuqiang.matt --- arch/openrisc/include/asm/cmpxchg.h | 6 ++ 1 file changed, 6 insertions(+) diff --git a/arch/openrisc/include/asm/cmpxchg.h b/arch/openrisc/include/asm/cmpxchg.h index 8ee151c072e4..f1ffe8b6f5ef 100644 --- a/arch/openrisc/include/asm/cmpxchg.h +++ b/arch/openrisc/include/asm/cmpxchg.h @@ -139,6 +139,12 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old, (unsigned long)(n), \ sizeof(*(ptr))); \ }) +#define arch_cmpxchg_local arch_cmpxchg + +/* always make arch_cmpxchg64_local available for openrisc */ +#include + +#define arch_cmpxchg64_local(ptr, o, n) __generic_cmpxchg64_local((ptr), (o), (n)) /* * This function doesn't exist, so you'll get a linker error if -- 2.40.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v2 4/4] locking/atomic: hexagon: arch_cmpxchg[64]_local undefined
For architectures that support native cmpxchg, we'd like to
implement arch_cmpxchg[64]_local with the native variants of
supported data size. If not, the generci_cmpxchg[64]_local
will be used.
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202310272207.tlpflya4-...@intel.com/
Signed-off-by: wuqiang.matt
---
arch/hexagon/include/asm/cmpxchg.h | 51 +-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/arch/hexagon/include/asm/cmpxchg.h
b/arch/hexagon/include/asm/cmpxchg.h
index bf6cf5579cf4..2b5e5bbaf807 100644
--- a/arch/hexagon/include/asm/cmpxchg.h
+++ b/arch/hexagon/include/asm/cmpxchg.h
@@ -8,6 +8,8 @@
#ifndef _ASM_CMPXCHG_H
#define _ASM_CMPXCHG_H
+#include
+
/*
* __arch_xchg - atomically exchange a register and a memory location
* @x: value to swap
@@ -51,13 +53,15 @@ __arch_xchg(unsigned long x, volatile void *ptr, int size)
* variable casting.
*/
-#define arch_cmpxchg(ptr, old, new)\
+#define __cmpxchg_32(ptr, old, new)\
({ \
__typeof__(ptr) __ptr = (ptr); \
__typeof__(*(ptr)) __old = (old); \
__typeof__(*(ptr)) __new = (new); \
__typeof__(*(ptr)) __oldval = 0;\
\
+ BUILD_BUG_ON(sizeof(*(ptr)) != 4); \
+ \
asm volatile( \
"1: %0 = memw_locked(%1);\n"\
" { P0 = cmp.eq(%0,%2);\n"\
@@ -72,4 +76,49 @@ __arch_xchg(unsigned long x, volatile void *ptr, int size)
__oldval; \
})
+#define __cmpxchg(ptr, old, val, size) \
+({ \
+ __typeof__(*(ptr)) oldval; \
+ \
+ switch (size) { \
+ case 4: \
+ oldval = __cmpxchg_32(ptr, old, val); \
+ break; \
+ default:\
+ BUILD_BUG();\
+ oldval = val; \
+ break; \
+ } \
+ \
+ oldval; \
+})
+
+#define arch_cmpxchg(ptr, o, n)__cmpxchg((ptr), (o), (n),
sizeof(*(ptr)))
+
+/*
+ * always make arch_cmpxchg[64]_local available, native cmpxchg
+ * will be used if available, then generic_cmpxchg[64]_local
+ */
+#include
+
+#define arch_cmpxchg_local(ptr, old, val) \
+({ \
+ __typeof__(*(ptr)) retval; \
+ int size = sizeof(*(ptr)); \
+ \
+ switch (size) { \
+ case 4: \
+ retval = __cmpxchg_32(ptr, old, val); \
+ break; \
+ default:\
+ retval = __generic_cmpxchg_local(ptr, old, \
+val, size);\
+ break; \
+ } \
+ \
+ retval; \
+})
+
+#define arch_cmpxchg64_local(ptr, o, n) __generic_cmpxchg64_local((ptr), (o),
(n))
+
#endif /* _ASM_CMPXCHG_H */
--
2.40.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3 0/5] arch,locking/atomic: add arch_cmpxchg[64]_local
Archtectures arc, openrisc and hexagon haven't arch_cmpxchg_local() defined, so the usecase of try_cmpxchg_local() in lib/objpool.c can not pass kernel building by the kernel test robot. Patch 1 improves the data size checking logic for arc; Patches 2/3/4 implement arch_cmpxchg[64]_local for arc/openrisc/hexagon. Patch 5 defines arch_cmpxchg_local as existing __cmpxchg_local rather the generic variant. wuqiang.matt (5): arch,locking/atomic: arc: arch_cmpxchg should check data size arch,locking/atomic: arc: add arch_cmpxchg[64]_local arch,locking/atomic: openrisc: add arch_cmpxchg[64]_local arch,locking/atomic: hexagon: add arch_cmpxchg[64]_local arch,locking/atomic: xtensa: define arch_cmpxchg_local as __cmpxchg_local arch/arc/include/asm/cmpxchg.h | 40 ++ arch/hexagon/include/asm/cmpxchg.h | 51 - arch/openrisc/include/asm/cmpxchg.h | 6 arch/xtensa/include/asm/cmpxchg.h | 2 +- 4 files changed, 91 insertions(+), 8 deletions(-) -- 2.40.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3 1/5] arch,locking/atomic: arc: arch_cmpxchg should check data size
arch_cmpxchg() should check data size rather than pointer size in case
CONFIG_ARC_HAS_LLSC is defined. So rename __cmpxchg to __cmpxchg_32 to
emphasize it's explicit support of 32bit data size with BUILD_BUG_ON()
added to avoid any possible misuses with unsupported data types.
In case CONFIG_ARC_HAS_LLSC is undefined, arch_cmpxchg() uses spinlock
to accomplish SMP-safety, so the BUILD_BUG_ON checking is uncecessary.
v2 -> v3:
- Patches regrouped and has the improvement for xtensa included
- Comments refined to address why these changes are needed
v1 -> v2:
- Try using native cmpxchg variants if avaialble, as Arnd advised
Signed-off-by: wuqiang.matt
Reviewed-by: Masami Hiramatsu (Google)
---
arch/arc/include/asm/cmpxchg.h | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arc/include/asm/cmpxchg.h b/arch/arc/include/asm/cmpxchg.h
index e138fde067de..bf46514f6f12 100644
--- a/arch/arc/include/asm/cmpxchg.h
+++ b/arch/arc/include/asm/cmpxchg.h
@@ -18,14 +18,16 @@
* if (*ptr == @old)
* *ptr = @new
*/
-#define __cmpxchg(ptr, old, new) \
+#define __cmpxchg_32(ptr, old, new)\
({ \
__typeof__(*(ptr)) _prev; \
\
+ BUILD_BUG_ON(sizeof(*(ptr)) != 4); \
+ \
__asm__ __volatile__( \
- "1: llock %0, [%1] \n" \
+ "1: llock %0, [%1] \n" \
" brne %0, %2, 2f \n" \
- " scond %3, [%1] \n" \
+ " scond %3, [%1] \n" \
" bnz 1b \n" \
"2: \n" \
: "=&r"(_prev) /* Early clobber prevent reg reuse */ \
@@ -47,7 +49,7 @@
\
switch(sizeof((_p_))) { \
case 4: \
- _prev_ = __cmpxchg(_p_, _o_, _n_); \
+ _prev_ = __cmpxchg_32(_p_, _o_, _n_); \
break; \
default:\
BUILD_BUG();\
@@ -65,8 +67,6 @@
__typeof__(*(ptr)) _prev_; \
unsigned long __flags; \
\
- BUILD_BUG_ON(sizeof(_p_) != 4); \
- \
/* \
* spin lock/unlock provide the needed smp_mb() before/after\
*/ \
--
2.40.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3 2/5] arch,locking/atomic: arc: add arch_cmpxchg[64]_local
arc doesn't have arch_cmpxhg_local implemented, which causes
building failures for any references of try_cmpxchg_local,
reported by the kernel test robot.
This patch implements arch_cmpxchg[64]_local with the native
cmpxchg variant if the corresponding data size is supported,
otherwise generci_cmpxchg[64]_local is to be used.
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202310272207.tlpflya4-...@intel.com/
Signed-off-by: wuqiang.matt
Reviewed-by: Masami Hiramatsu (Google)
---
arch/arc/include/asm/cmpxchg.h | 28
1 file changed, 28 insertions(+)
diff --git a/arch/arc/include/asm/cmpxchg.h b/arch/arc/include/asm/cmpxchg.h
index bf46514f6f12..91429f2350df 100644
--- a/arch/arc/include/asm/cmpxchg.h
+++ b/arch/arc/include/asm/cmpxchg.h
@@ -80,6 +80,34 @@
#endif
+/*
+ * always make arch_cmpxchg[64]_local available, native cmpxchg
+ * will be used if available, then generic_cmpxchg[64]_local
+ */
+#include
+static inline unsigned long __cmpxchg_local(volatile void *ptr,
+ unsigned long old,
+ unsigned long new, int size)
+{
+ switch (size) {
+#ifdef CONFIG_ARC_HAS_LLSC
+ case 4:
+ return __cmpxchg_32((int32_t *)ptr, old, new);
+#endif
+ default:
+ return __generic_cmpxchg_local(ptr, old, new, size);
+ }
+
+ return old;
+}
+#define arch_cmpxchg_local(ptr, o, n) ({ \
+ (__typeof__(*ptr))__cmpxchg_local((ptr),\
+ (unsigned long)(o), \
+ (unsigned long)(n), \
+ sizeof(*(ptr)));\
+})
+#define arch_cmpxchg64_local(ptr, o, n) __generic_cmpxchg64_local((ptr), (o),
(n))
+
/*
* xchg
*/
--
2.40.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3 3/5] arch,locking/atomic: openrisc: add arch_cmpxchg[64]_local
openrisc hasn't arch_cmpxhg_local implemented, which causes building failures for any references of try_cmpxchg_local, reported by the kernel test robot. This patch implements arch_cmpxchg[64]_local with the native cmpxchg variant if the corresponding data size is supported, otherwise generci_cmpxchg[64]_local is to be used. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202310272207.tlpflya4-...@intel.com/ Signed-off-by: wuqiang.matt Reviewed-by: Masami Hiramatsu (Google) --- arch/openrisc/include/asm/cmpxchg.h | 6 ++ 1 file changed, 6 insertions(+) diff --git a/arch/openrisc/include/asm/cmpxchg.h b/arch/openrisc/include/asm/cmpxchg.h index 8ee151c072e4..f1ffe8b6f5ef 100644 --- a/arch/openrisc/include/asm/cmpxchg.h +++ b/arch/openrisc/include/asm/cmpxchg.h @@ -139,6 +139,12 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old, (unsigned long)(n), \ sizeof(*(ptr))); \ }) +#define arch_cmpxchg_local arch_cmpxchg + +/* always make arch_cmpxchg64_local available for openrisc */ +#include + +#define arch_cmpxchg64_local(ptr, o, n) __generic_cmpxchg64_local((ptr), (o), (n)) /* * This function doesn't exist, so you'll get a linker error if -- 2.40.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3 4/5] arch,locking/atomic: hexagon: add arch_cmpxchg[64]_local
hexagonc hasn't arch_cmpxhg_local implemented, which causes
building failures for any references of try_cmpxchg_local,
reported by the kernel test robot.
This patch implements arch_cmpxchg[64]_local with the native
cmpxchg variant if the corresponding data size is supported,
otherwise generci_cmpxchg[64]_local is to be used.
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202310272207.tlpflya4-...@intel.com/
Signed-off-by: wuqiang.matt
Reviewed-by: Masami Hiramatsu (Google)
---
arch/hexagon/include/asm/cmpxchg.h | 51 +-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/arch/hexagon/include/asm/cmpxchg.h
b/arch/hexagon/include/asm/cmpxchg.h
index bf6cf5579cf4..302fa30f25aa 100644
--- a/arch/hexagon/include/asm/cmpxchg.h
+++ b/arch/hexagon/include/asm/cmpxchg.h
@@ -8,6 +8,8 @@
#ifndef _ASM_CMPXCHG_H
#define _ASM_CMPXCHG_H
+#include
+
/*
* __arch_xchg - atomically exchange a register and a memory location
* @x: value to swap
@@ -51,13 +53,15 @@ __arch_xchg(unsigned long x, volatile void *ptr, int size)
* variable casting.
*/
-#define arch_cmpxchg(ptr, old, new)\
+#define __cmpxchg_32(ptr, old, new)\
({ \
__typeof__(ptr) __ptr = (ptr); \
__typeof__(*(ptr)) __old = (old); \
__typeof__(*(ptr)) __new = (new); \
__typeof__(*(ptr)) __oldval = 0;\
\
+ BUILD_BUG_ON(sizeof(*(ptr)) != 4); \
+ \
asm volatile( \
"1: %0 = memw_locked(%1);\n"\
" { P0 = cmp.eq(%0,%2);\n"\
@@ -72,4 +76,49 @@ __arch_xchg(unsigned long x, volatile void *ptr, int size)
__oldval; \
})
+#define __cmpxchg(ptr, old, val, size) \
+({ \
+ __typeof__(*(ptr)) oldval; \
+ \
+ switch (size) { \
+ case 4: \
+ oldval = __cmpxchg_32(ptr, old, val); \
+ break; \
+ default:\
+ BUILD_BUG();\
+ oldval = val; \
+ break; \
+ } \
+ \
+ oldval; \
+})
+
+#define arch_cmpxchg(ptr, o, n)__cmpxchg((ptr), (o), (n),
sizeof(*(ptr)))
+
+/*
+ * always make arch_cmpxchg[64]_local available, native cmpxchg
+ * will be used if available, then generic_cmpxchg[64]_local
+ */
+#include
+
+#define arch_cmpxchg_local(ptr, old, val) \
+({ \
+ __typeof__(*(ptr)) __retval;\
+ int __size = sizeof(*(ptr));\
+ \
+ switch (__size) { \
+ case 4: \
+ __retval = __cmpxchg_32(ptr, old, val); \
+ break; \
+ default:\
+ __retval = __generic_cmpxchg_local(ptr, old,\
+ val, __size); \
+ break; \
+ } \
+ \
+ __retval; \
+})
+
+#define arch_cmpxchg64_local(ptr, o, n) __generic_cmpxchg64_local((ptr), (o),
(n))
+
#endif /* _ASM_CMPXCHG_H */
--
2.40.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v3 5/5] arch,locking/atomic: xtensa: define arch_cmpxchg_local as __cmpxchg_local
The xtensa architecture already has __cmpxchg_local defined but not used. The purpose of __cmpxchg_local() is solely for arch_cmpxchg_local(), just as the definition of arch_cmpxchg_local() for other architectures like x86, arm and powerpc. Signed-off-by: wuqiang.matt --- arch/xtensa/include/asm/cmpxchg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/xtensa/include/asm/cmpxchg.h b/arch/xtensa/include/asm/cmpxchg.h index 675a11ea8de7..956c9925df1c 100644 --- a/arch/xtensa/include/asm/cmpxchg.h +++ b/arch/xtensa/include/asm/cmpxchg.h @@ -108,7 +108,7 @@ static inline unsigned long __cmpxchg_local(volatile void *ptr, * them available. */ #define arch_cmpxchg_local(ptr, o, n) \ - ((__typeof__(*(ptr)))__generic_cmpxchg_local((ptr), (unsigned long)(o),\ + ((__typeof__(*(ptr)))__cmpxchg_local((ptr), (unsigned long)(o),\ (unsigned long)(n), sizeof(*(ptr #define arch_cmpxchg64_local(ptr, o, n) __generic_cmpxchg64_local((ptr), (o), (n)) #define arch_cmpxchg64(ptr, o, n)arch_cmpxchg64_local((ptr), (o), (n)) -- 2.40.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v3 1/5] arch,locking/atomic: arc: arch_cmpxchg should check data size
Hello Andi,
On 2023/11/23 06:17, Andi Shyti wrote:
Hi Wuqiang,
On Tue, Nov 21, 2023 at 10:23:43PM +0800, wuqiang.matt wrote:
arch_cmpxchg() should check data size rather than pointer size in case
CONFIG_ARC_HAS_LLSC is defined. So rename __cmpxchg to __cmpxchg_32 to
emphasize it's explicit support of 32bit data size with BUILD_BUG_ON()
added to avoid any possible misuses with unsupported data types.
In case CONFIG_ARC_HAS_LLSC is undefined, arch_cmpxchg() uses spinlock
to accomplish SMP-safety, so the BUILD_BUG_ON checking is uncecessary.
v2 -> v3:
- Patches regrouped and has the improvement for xtensa included
- Comments refined to address why these changes are needed
v1 -> v2:
- Try using native cmpxchg variants if avaialble, as Arnd advised
BTW, the changelog should be in the cover letter. I'll correct it in next
version, so don't bother resending to make more noises.
Signed-off-by: wuqiang.matt
Reviewed-by: Masami Hiramatsu (Google)
---
arch/arc/include/asm/cmpxchg.h | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arc/include/asm/cmpxchg.h b/arch/arc/include/asm/cmpxchg.h
index e138fde067de..bf46514f6f12 100644
--- a/arch/arc/include/asm/cmpxchg.h
+++ b/arch/arc/include/asm/cmpxchg.h
@@ -18,14 +18,16 @@
* if (*ptr == @old)
* *ptr = @new
*/
-#define __cmpxchg(ptr, old, new) \
+#define __cmpxchg_32(ptr, old, new)\
({\
__typeof__(*(ptr)) _prev; \
\
+ BUILD_BUG_ON(sizeof(*(ptr)) != 4); \
+ \
__asm__ __volatile__( \
- "1:llock %0, [%1] \n"\
+ "1:llock %0, [%1] \n"\
" brne %0, %2, 2f \n"\
- " scond %3, [%1] \n"\
+ " scond %3, [%1] \n"\
" bnz 1b \n"\
"2:\n"\
: "=&r"(_prev)/* Early clobber prevent reg reuse */ \
@@ -47,7 +49,7 @@
\
switch(sizeof((_p_))) { \
case 4: \
- _prev_ = __cmpxchg(_p_, _o_, _n_); \
+ _prev_ = __cmpxchg_32(_p_, _o_, _n_); \
break; \
default:\
BUILD_BUG();\
@@ -65,8 +67,6 @@
__typeof__(*(ptr)) _prev_; \
unsigned long __flags; \
\
- BUILD_BUG_ON(sizeof(_p_) != 4); \
- \
I think I made some comments here that have not been addressed or
replied.
Sorry that I haven't seen your message. Could you resend ? I rechecked
my mailbox and the mailing lists, but no luck.
Thanks,
Andi
Regards,
Wuqiang
/* \
* spin lock/unlock provide the needed smp_mb() before/after\
*/ \
--
2.40.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
