* [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg
@ 2024-07-19 2:40 Bibo Mao
2024-07-19 10:15 ` Uros Bizjak
0 siblings, 1 reply; 4+ messages in thread
From: Bibo Mao @ 2024-07-19 2:40 UTC (permalink / raw)
To: Will Deacon, Peter Zijlstra
Cc: Boqun Feng, Ingo Molnar, Paul E . McKenney, Carlos Llamas,
Uros Bizjak, linux-kernel
When porting pv spinlock function on LoongArch system, there is
compiling error such as:
from linux/include/linux/smp.h:13,
from linux/kernel/locking/qspinlock.c:16:
linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
242 | typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
| ^
linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
4908 | raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
| ^~~~~~~~~~~~~~~~~~~~~~~
linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
377 | if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))
| ^~~~~~~~~~~~~~~~~~~
In file included from linux/kernel/locking/qspinlock.c:583:
linux/kernel/locking/qspinlock_paravirt.h: At top level:
linux/kernel/locking/qspinlock_paravirt.h:499:1: warning: no previous prototype for '__pv_queued_spin_unlock_slowpath' [-Wmissing-prototypes]
499 | __pv_queued_spin_unlock_slowpath(struct qspinlock *lock, u8 locked)
Macro try_cmpxchg_relaxed() is used by paravirt qspinlock, on LoongArch
it is defined as raw_try_cmpxchg_relaxed(). And there is different type
conversion in marco raw_try_cmpxchg_relaxed(). Here type declaration is
added beore type conversion.
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
include/linux/atomic/atomic-arch-fallback.h | 50 ++++++++++++++-------
scripts/atomic/gen-atomic-fallback.sh | 3 +-
2 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/include/linux/atomic/atomic-arch-fallback.h b/include/linux/atomic/atomic-arch-fallback.h
index 2f9d36b72bd8..8273c53321c3 100644
--- a/include/linux/atomic/atomic-arch-fallback.h
+++ b/include/linux/atomic/atomic-arch-fallback.h
@@ -188,7 +188,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -206,7 +207,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg_acquire(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg_acquire((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -224,7 +226,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg_release(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg_release((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -239,7 +242,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg_relaxed(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg_relaxed((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -255,7 +259,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg64(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg64((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -273,7 +278,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg64_acquire(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg64_acquire((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -291,7 +297,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg64_release(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg64_release((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -306,7 +313,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg64_relaxed(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg64_relaxed((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -322,7 +330,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg128(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg128((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -340,7 +349,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg128_acquire(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg128_acquire((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -358,7 +368,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg128_release(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg128_release((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -373,7 +384,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg128_relaxed(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg128_relaxed((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -388,7 +400,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg_local(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -403,7 +416,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg64_local(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg64_local((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -418,7 +432,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_try_cmpxchg128_local(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_cmpxchg128_local((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -433,7 +448,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
#else
#define raw_sync_try_cmpxchg(_ptr, _oldp, _new) \
({ \
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+ typeof(*(_ptr)) ___o = *___op, ___r; \
___r = raw_sync_cmpxchg((_ptr), ___o, (_new)); \
if (unlikely(___r != ___o)) \
*___op = ___r; \
@@ -4690,4 +4706,4 @@ raw_atomic64_dec_if_positive(atomic64_t *v)
}
#endif /* _LINUX_ATOMIC_FALLBACK_H */
-// b565db590afeeff0d7c9485ccbca5bb6e155749f
+// 7b23ddca3c50c5869e68ded50748ffe111123156
diff --git a/scripts/atomic/gen-atomic-fallback.sh b/scripts/atomic/gen-atomic-fallback.sh
index f80d69cfeb1f..f3a9b084a176 100755
--- a/scripts/atomic/gen-atomic-fallback.sh
+++ b/scripts/atomic/gen-atomic-fallback.sh
@@ -230,7 +230,8 @@ gen_try_cmpxchg_fallback()
cat <<EOF
#define raw_${prefix}try_${cmpxchg}${suffix}(_ptr, _oldp, _new) \\
({ \\
- typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \\
+ typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \\
+ typeof(*(_ptr)) ___o = *___op, ___r; \\
___r = raw_${prefix}${cmpxchg}${suffix}((_ptr), ___o, (_new)); \\
if (unlikely(___r != ___o)) \\
*___op = ___r; \\
base-commit: 720261cfc7329406a50c2a8536e0039b9dd9a4e5
--
2.39.3
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg
2024-07-19 2:40 [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg Bibo Mao
@ 2024-07-19 10:15 ` Uros Bizjak
2024-07-19 16:18 ` Boqun Feng
0 siblings, 1 reply; 4+ messages in thread
From: Uros Bizjak @ 2024-07-19 10:15 UTC (permalink / raw)
To: Bibo Mao
Cc: Will Deacon, Peter Zijlstra, Boqun Feng, Ingo Molnar,
Paul E . McKenney, Carlos Llamas, linux-kernel
On Fri, Jul 19, 2024 at 4:40 AM Bibo Mao <maobibo@loongson.cn> wrote:
>
> When porting pv spinlock function on LoongArch system, there is
> compiling error such as:
> from linux/include/linux/smp.h:13,
> from linux/kernel/locking/qspinlock.c:16:
> linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
> linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
> 242 | typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
> | ^
> linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
> 4908 | raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> | ^~~~~~~~~~~~~~~~~~~~~~~
> linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
> 377 | if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))
This points to the mismatch between "pn->state" and "old" variable.
The correct fix is:
--cut here--
diff --git a/kernel/locking/qspinlock_paravirt.h
b/kernel/locking/qspinlock_paravirt.h
index f5a36e67b593..ac2e22502741 100644
--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h
@@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
*node, struct mcs_spinlock *prev)
static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
{
struct pv_node *pn = (struct pv_node *)node;
- enum vcpu_state old = vcpu_halted;
+ u8 old = vcpu_halted;
/*
* If the vCPU is indeed halted, advance its state to match that of
* pv_wait_node(). If OTOH this fails, the vCPU was running and will
--cut here--
Uros.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg
2024-07-19 10:15 ` Uros Bizjak
@ 2024-07-19 16:18 ` Boqun Feng
2024-07-20 8:35 ` Uros Bizjak
0 siblings, 1 reply; 4+ messages in thread
From: Boqun Feng @ 2024-07-19 16:18 UTC (permalink / raw)
To: Uros Bizjak
Cc: Bibo Mao, Will Deacon, Peter Zijlstra, Ingo Molnar,
Paul E . McKenney, Carlos Llamas, linux-kernel
On Fri, Jul 19, 2024 at 12:15:28PM +0200, Uros Bizjak wrote:
> On Fri, Jul 19, 2024 at 4:40 AM Bibo Mao <maobibo@loongson.cn> wrote:
> >
> > When porting pv spinlock function on LoongArch system, there is
> > compiling error such as:
> > from linux/include/linux/smp.h:13,
> > from linux/kernel/locking/qspinlock.c:16:
> > linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
> > linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
> > 242 | typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
> > | ^
> > linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
> > 4908 | raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
> > 377 | if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))
>
> This points to the mismatch between "pn->state" and "old" variable.
> The correct fix is:
>
> --cut here--
> diff --git a/kernel/locking/qspinlock_paravirt.h
> b/kernel/locking/qspinlock_paravirt.h
> index f5a36e67b593..ac2e22502741 100644
> --- a/kernel/locking/qspinlock_paravirt.h
> +++ b/kernel/locking/qspinlock_paravirt.h
> @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
> *node, struct mcs_spinlock *prev)
> static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
> {
> struct pv_node *pn = (struct pv_node *)node;
> - enum vcpu_state old = vcpu_halted;
> + u8 old = vcpu_halted;
> /*
Looks reasonable to me, we should also add static_assert() for
try_cmpxhg_*() to make sure the old has the same size of the cmpxchged
field.
Regards,
Boqun
> * If the vCPU is indeed halted, advance its state to match that of
> * pv_wait_node(). If OTOH this fails, the vCPU was running and will
> --cut here--
>
> Uros.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg
2024-07-19 16:18 ` Boqun Feng
@ 2024-07-20 8:35 ` Uros Bizjak
0 siblings, 0 replies; 4+ messages in thread
From: Uros Bizjak @ 2024-07-20 8:35 UTC (permalink / raw)
To: Boqun Feng
Cc: Bibo Mao, Will Deacon, Peter Zijlstra, Ingo Molnar,
Paul E . McKenney, Carlos Llamas, linux-kernel
On Fri, Jul 19, 2024 at 6:18 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Fri, Jul 19, 2024 at 12:15:28PM +0200, Uros Bizjak wrote:
> > On Fri, Jul 19, 2024 at 4:40 AM Bibo Mao <maobibo@loongson.cn> wrote:
> > >
> > > When porting pv spinlock function on LoongArch system, there is
> > > compiling error such as:
> > > from linux/include/linux/smp.h:13,
> > > from linux/kernel/locking/qspinlock.c:16:
> > > linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
> > > linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
> > > 242 | typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
> > > | ^
> > > linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
> > > 4908 | raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> > > | ^~~~~~~~~~~~~~~~~~~~~~~
> > > linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
> > > 377 | if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))
> >
> > This points to the mismatch between "pn->state" and "old" variable.
> > The correct fix is:
> >
> > --cut here--
> > diff --git a/kernel/locking/qspinlock_paravirt.h
> > b/kernel/locking/qspinlock_paravirt.h
> > index f5a36e67b593..ac2e22502741 100644
> > --- a/kernel/locking/qspinlock_paravirt.h
> > +++ b/kernel/locking/qspinlock_paravirt.h
> > @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
> > *node, struct mcs_spinlock *prev)
> > static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
> > {
> > struct pv_node *pn = (struct pv_node *)node;
> > - enum vcpu_state old = vcpu_halted;
> > + u8 old = vcpu_halted;
> > /*
>
> Looks reasonable to me, we should also add static_assert() for
> try_cmpxhg_*() to make sure the old has the same size of the cmpxchged
> field.
This is what -Wincompatible-pointer-types should detect, and it does
for the LoongArch target. Apparently, x86_64 targets use
-fshort-enums, so the mismatch was not detected on this particular
target.
Bibo Mao, does the proposed change works for you, so I can propose a
formal patch submission with the fix?
Thanks,
Uros.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-07-20 8:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-19 2:40 [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg Bibo Mao
2024-07-19 10:15 ` Uros Bizjak
2024-07-19 16:18 ` Boqun Feng
2024-07-20 8:35 ` Uros Bizjak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®