* [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout()
@ 2025-09-11 3:46 Ankur Arora
2025-09-11 3:46 ` [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout() Ankur Arora
` (5 more replies)
0 siblings, 6 replies; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 3:46 UTC (permalink / raw)
To: linux-kernel, linux-arch, linux-arm-kernel, bpf
Cc: arnd, catalin.marinas, will, peterz, akpm, mark.rutland, harisokn,
cl, ast, memxor, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
This series adds waited variants of the smp_cond_load() primitives:
smp_cond_load_relaxed_timeout(), and smp_cond_load_acquire_timeout().
As the name suggests, the new interfaces are meant for contexts where
you want to wait on a condition variable for a finite duration. This
is easy enough to do with a loop around cpu_relax() and a periodic
timeout check (pretty much what we do in poll_idle(). However, some
architectures (ex. arm64) also allow waiting on a cacheline. So,
smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr)
smp_cond_load_acquire_timeout(ptr, cond_expr, time_check_expr)
do a mixture of spin/wait with a smp_cond_load() thrown in.
The added parameter, time_check_expr, determines the bail out condition.
There are two current users for these interfaces. poll_idle() with
the change:
poll_idle() {
...
time_end = local_clock_noinstr() + cpuidle_poll_time(drv, dev);
raw_local_irq_enable();
if (!current_set_polling_and_test())
flags = smp_cond_load_relaxed_timeout(¤t_thread_info()->flags,
(VAL & _TIF_NEED_RESCHED),
((local_clock_noinstr() >= time_end)));
dev->poll_time_limit = !(flags & _TIF_NEED_RESCHED);
raw_local_irq_disable();
...
}
where smp_cond_load_relaxed_timeout() replaces the inner loop in
poll_idle() (on x86 the generated code for both is similar):
poll_idle() {
...
raw_local_irq_enable();
if (!current_set_polling_and_test()) {
unsigned int loop_count = 0;
u64 limit;
limit = cpuidle_poll_time(drv, dev);
while (!need_resched()) {
cpu_relax();
if (loop_count++ < POLL_IDLE_RELAX_COUNT)
continue;
loop_count = 0;
if (local_clock_noinstr() - time_start > limit) {
dev->poll_time_limit = true;
break;
}
}
}
raw_local_irq_disable();
...
}
And resilient queued spinlocks:
resilient_queued_spin_lock_slowpath() {
...
if (val & _Q_LOCKED_MASK) {
RES_RESET_TIMEOUT(ts, RES_DEF_TIMEOUT);
smp_cond_load_acquire_timeout(&lock->locked, !VAL,
(ret = check_timeout(lock, _Q_LOCKED_MASK, &ts)));
}
...
}
Changelog:
v4 [1]:
- naming change 's/timewait/timeout/'
- resilient spinlocks: get rid of res_smp_cond_load_acquire_waiting()
and fixup use of RES_CHECK_TIMEOUT().
(Both suggested by Catalin Marinas)
v3 [2]:
- further interface simplifications (suggested by Catalin Marinas)
v2 [3]:
- simplified the interface (suggested by Catalin Marinas)
- get rid of wait_policy, and a multitude of constants
- adds a slack parameter
This helped remove a fair amount of duplicated code duplication and in
hindsight unnecessary constants.
v1 [4]:
- add wait_policy (coarse and fine)
- derive spin-count etc at runtime instead of using arbitrary
constants.
Haris Okanovic tested v4 of this series with poll_idle()/haltpoll patches. [5]
Any comments appreciated!
Thanks
Ankur
[1] https://lore.kernel.org/lkml/20250829080735.3598416-1-ankur.a.arora@oracle.com/
[2] https://lore.kernel.org/lkml/20250627044805.945491-1-ankur.a.arora@oracle.com/
[3] https://lore.kernel.org/lkml/20250502085223.1316925-1-ankur.a.arora@oracle.com/
[4] https://lore.kernel.org/lkml/20250203214911.898276-1-ankur.a.arora@oracle.com/
[5] https://lore.kernel.org/lkml/2cecbf7fb23ee83a4ce027e1be3f46f97efd585c.camel@amazon.com/
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: linux-arch@vger.kernel.org
Ankur Arora (5):
asm-generic: barrier: Add smp_cond_load_relaxed_timeout()
arm64: barrier: Add smp_cond_load_relaxed_timeout()
arm64: rqspinlock: Remove private copy of
smp_cond_load_acquire_timewait
asm-generic: barrier: Add smp_cond_load_acquire_timeout()
rqspinlock: use smp_cond_load_acquire_timeout()
arch/arm64/include/asm/barrier.h | 23 ++++++++
arch/arm64/include/asm/rqspinlock.h | 85 -----------------------------
include/asm-generic/barrier.h | 57 +++++++++++++++++++
kernel/bpf/rqspinlock.c | 23 +++-----
4 files changed, 87 insertions(+), 101 deletions(-)
--
2.43.5
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()
2025-09-11 3:46 [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Ankur Arora
@ 2025-09-11 3:46 ` Ankur Arora
2025-09-18 19:42 ` Will Deacon
2025-09-11 3:46 ` [PATCH v5 2/5] arm64: " Ankur Arora
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 3:46 UTC (permalink / raw)
To: linux-kernel, linux-arch, linux-arm-kernel, bpf
Cc: arnd, catalin.marinas, will, peterz, akpm, mark.rutland, harisokn,
cl, ast, memxor, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
Add smp_cond_load_relaxed_timeout(), which extends
smp_cond_load_relaxed() to allow waiting for a duration.
The additional parameter allows for the timeout check.
The waiting is done via the usual cpu_relax() spin-wait around the
condition variable with periodic evaluation of the time-check.
The number of times we spin is defined by SMP_TIMEOUT_SPIN_COUNT
(chosen to be 200 by default) which, assuming each cpu_relax()
iteration takes around 20-30 cycles (measured on a variety of x86
platforms), amounts to around 4000-6000 cycles.
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-arch@vger.kernel.org
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Haris Okanovic <harisokn@amazon.com>
Tested-by: Haris Okanovic <harisokn@amazon.com>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
include/asm-generic/barrier.h | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index d4f581c1e21d..8483e139954f 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -273,6 +273,41 @@ do { \
})
#endif
+#ifndef SMP_TIMEOUT_SPIN_COUNT
+#define SMP_TIMEOUT_SPIN_COUNT 200
+#endif
+
+/**
+ * smp_cond_load_relaxed_timeout() - (Spin) wait for cond with no ordering
+ * guarantees until a timeout expires.
+ * @ptr: pointer to the variable to wait on
+ * @cond: boolean expression to wait for
+ * @time_check_expr: expression to decide when to bail out
+ *
+ * Equivalent to using READ_ONCE() on the condition variable.
+ */
+#ifndef smp_cond_load_relaxed_timeout
+#define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
+({ \
+ typeof(ptr) __PTR = (ptr); \
+ __unqual_scalar_typeof(*ptr) VAL; \
+ u32 __n = 0, __spin = SMP_TIMEOUT_SPIN_COUNT; \
+ \
+ for (;;) { \
+ VAL = READ_ONCE(*__PTR); \
+ if (cond_expr) \
+ break; \
+ cpu_relax(); \
+ if (++__n < __spin) \
+ continue; \
+ if (time_check_expr) \
+ break; \
+ __n = 0; \
+ } \
+ (typeof(*ptr))VAL; \
+})
+#endif
+
/*
* pmem_wmb() ensures that all stores for which the modification
* are written to persistent storage by preceding instructions have
--
2.43.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 2/5] arm64: barrier: Add smp_cond_load_relaxed_timeout()
2025-09-11 3:46 [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Ankur Arora
2025-09-11 3:46 ` [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout() Ankur Arora
@ 2025-09-11 3:46 ` Ankur Arora
2025-09-18 20:05 ` Will Deacon
2025-09-11 3:46 ` [PATCH v5 3/5] arm64: rqspinlock: Remove private copy of smp_cond_load_acquire_timewait Ankur Arora
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 3:46 UTC (permalink / raw)
To: linux-kernel, linux-arch, linux-arm-kernel, bpf
Cc: arnd, catalin.marinas, will, peterz, akpm, mark.rutland, harisokn,
cl, ast, memxor, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
Add smp_cond_load_relaxed_timeout(), a timed variant of
smp_cond_load_relaxed().
This uses __cmpwait_relaxed() to do the actual waiting, with the
event-stream guaranteeing that we wake up from WFE periodically
and not block forever in case there are no stores to the cacheline.
For cases when the event-stream is unavailable, fallback to
spin-waiting.
Cc: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Haris Okanovic <harisokn@amazon.com>
Tested-by: Haris Okanovic <harisokn@amazon.com>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
arch/arm64/include/asm/barrier.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
index f5801b0ba9e9..4f0d9ed7a072 100644
--- a/arch/arm64/include/asm/barrier.h
+++ b/arch/arm64/include/asm/barrier.h
@@ -219,6 +219,29 @@ do { \
(typeof(*ptr))VAL; \
})
+/* Re-declared here to avoid include dependency. */
+extern bool arch_timer_evtstrm_available(void);
+
+#define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
+({ \
+ typeof(ptr) __PTR = (ptr); \
+ __unqual_scalar_typeof(*ptr) VAL; \
+ bool __wfe = arch_timer_evtstrm_available(); \
+ \
+ for (;;) { \
+ VAL = READ_ONCE(*__PTR); \
+ if (cond_expr) \
+ break; \
+ if (time_check_expr) \
+ break; \
+ if (likely(__wfe)) \
+ __cmpwait_relaxed(__PTR, VAL); \
+ else \
+ cpu_relax(); \
+ } \
+ (typeof(*ptr)) VAL; \
+})
+
#include <asm-generic/barrier.h>
#endif /* __ASSEMBLY__ */
--
2.43.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 3/5] arm64: rqspinlock: Remove private copy of smp_cond_load_acquire_timewait
2025-09-11 3:46 [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Ankur Arora
2025-09-11 3:46 ` [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout() Ankur Arora
2025-09-11 3:46 ` [PATCH v5 2/5] arm64: " Ankur Arora
@ 2025-09-11 3:46 ` Ankur Arora
2025-09-11 3:46 ` [PATCH v5 4/5] asm-generic: barrier: Add smp_cond_load_acquire_timeout() Ankur Arora
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 3:46 UTC (permalink / raw)
To: linux-kernel, linux-arch, linux-arm-kernel, bpf
Cc: arnd, catalin.marinas, will, peterz, akpm, mark.rutland, harisokn,
cl, ast, memxor, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
In preparation for defining smp_cond_load_acquire_timeout(), remove
the private copy. Lacking this, the rqspinlock code falls back to using
smp_cond_load_acquire().
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Haris Okanovic <harisokn@amazon.com>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
arch/arm64/include/asm/rqspinlock.h | 85 -----------------------------
1 file changed, 85 deletions(-)
diff --git a/arch/arm64/include/asm/rqspinlock.h b/arch/arm64/include/asm/rqspinlock.h
index 9ea0a74e5892..a385603436e9 100644
--- a/arch/arm64/include/asm/rqspinlock.h
+++ b/arch/arm64/include/asm/rqspinlock.h
@@ -3,91 +3,6 @@
#define _ASM_RQSPINLOCK_H
#include <asm/barrier.h>
-
-/*
- * Hardcode res_smp_cond_load_acquire implementations for arm64 to a custom
- * version based on [0]. In rqspinlock code, our conditional expression involves
- * checking the value _and_ additionally a timeout. However, on arm64, the
- * WFE-based implementation may never spin again if no stores occur to the
- * locked byte in the lock word. As such, we may be stuck forever if
- * event-stream based unblocking is not available on the platform for WFE spin
- * loops (arch_timer_evtstrm_available).
- *
- * Once support for smp_cond_load_acquire_timewait [0] lands, we can drop this
- * copy-paste.
- *
- * While we rely on the implementation to amortize the cost of sampling
- * cond_expr for us, it will not happen when event stream support is
- * unavailable, time_expr check is amortized. This is not the common case, and
- * it would be difficult to fit our logic in the time_expr_ns >= time_limit_ns
- * comparison, hence just let it be. In case of event-stream, the loop is woken
- * up at microsecond granularity.
- *
- * [0]: https://lore.kernel.org/lkml/20250203214911.898276-1-ankur.a.arora@oracle.com
- */
-
-#ifndef smp_cond_load_acquire_timewait
-
-#define smp_cond_time_check_count 200
-
-#define __smp_cond_load_relaxed_spinwait(ptr, cond_expr, time_expr_ns, \
- time_limit_ns) ({ \
- typeof(ptr) __PTR = (ptr); \
- __unqual_scalar_typeof(*ptr) VAL; \
- unsigned int __count = 0; \
- for (;;) { \
- VAL = READ_ONCE(*__PTR); \
- if (cond_expr) \
- break; \
- cpu_relax(); \
- if (__count++ < smp_cond_time_check_count) \
- continue; \
- if ((time_expr_ns) >= (time_limit_ns)) \
- break; \
- __count = 0; \
- } \
- (typeof(*ptr))VAL; \
-})
-
-#define __smp_cond_load_acquire_timewait(ptr, cond_expr, \
- time_expr_ns, time_limit_ns) \
-({ \
- typeof(ptr) __PTR = (ptr); \
- __unqual_scalar_typeof(*ptr) VAL; \
- for (;;) { \
- VAL = smp_load_acquire(__PTR); \
- if (cond_expr) \
- break; \
- __cmpwait_relaxed(__PTR, VAL); \
- if ((time_expr_ns) >= (time_limit_ns)) \
- break; \
- } \
- (typeof(*ptr))VAL; \
-})
-
-#define smp_cond_load_acquire_timewait(ptr, cond_expr, \
- time_expr_ns, time_limit_ns) \
-({ \
- __unqual_scalar_typeof(*ptr) _val; \
- int __wfe = arch_timer_evtstrm_available(); \
- \
- if (likely(__wfe)) { \
- _val = __smp_cond_load_acquire_timewait(ptr, cond_expr, \
- time_expr_ns, \
- time_limit_ns); \
- } else { \
- _val = __smp_cond_load_relaxed_spinwait(ptr, cond_expr, \
- time_expr_ns, \
- time_limit_ns); \
- smp_acquire__after_ctrl_dep(); \
- } \
- (typeof(*ptr))_val; \
-})
-
-#endif
-
-#define res_smp_cond_load_acquire(v, c) smp_cond_load_acquire_timewait(v, c, 0, 1)
-
#include <asm-generic/rqspinlock.h>
#endif /* _ASM_RQSPINLOCK_H */
--
2.43.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 4/5] asm-generic: barrier: Add smp_cond_load_acquire_timeout()
2025-09-11 3:46 [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Ankur Arora
` (2 preceding siblings ...)
2025-09-11 3:46 ` [PATCH v5 3/5] arm64: rqspinlock: Remove private copy of smp_cond_load_acquire_timewait Ankur Arora
@ 2025-09-11 3:46 ` Ankur Arora
2025-09-11 3:46 ` [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout() Ankur Arora
2025-09-11 14:34 ` [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Catalin Marinas
5 siblings, 0 replies; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 3:46 UTC (permalink / raw)
To: linux-kernel, linux-arch, linux-arm-kernel, bpf
Cc: arnd, catalin.marinas, will, peterz, akpm, mark.rutland, harisokn,
cl, ast, memxor, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
Add the acquire variant of smp_cond_load_relaxed_timeout(). This
reuses the relaxed variant, with an additional LOAD->LOAD
ordering.
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-arch@vger.kernel.org
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Haris Okanovic <harisokn@amazon.com>
Tested-by: Haris Okanovic <harisokn@amazon.com>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
include/asm-generic/barrier.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index 8483e139954f..f2e90c993ead 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -308,6 +308,28 @@ do { \
})
#endif
+/**
+ * smp_cond_load_acquire_timeout() - (Spin) wait for cond with ACQUIRE ordering
+ * until a timeout expires.
+ *
+ * Arguments: same as smp_cond_load_relaxed_timeout().
+ *
+ * Equivalent to using smp_cond_load_acquire() on the condition variable with
+ * a timeout.
+ */
+#ifndef smp_cond_load_acquire_timeout
+#define smp_cond_load_acquire_timeout(ptr, cond_expr, time_check_expr) \
+({ \
+ __unqual_scalar_typeof(*ptr) _val; \
+ _val = smp_cond_load_relaxed_timeout(ptr, cond_expr, \
+ time_check_expr); \
+ \
+ /* Depends on the control dependency of the wait above. */ \
+ smp_acquire__after_ctrl_dep(); \
+ (typeof(*ptr))_val; \
+})
+#endif
+
/*
* pmem_wmb() ensures that all stores for which the modification
* are written to persistent storage by preceding instructions have
--
2.43.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout()
2025-09-11 3:46 [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Ankur Arora
` (3 preceding siblings ...)
2025-09-11 3:46 ` [PATCH v5 4/5] asm-generic: barrier: Add smp_cond_load_acquire_timeout() Ankur Arora
@ 2025-09-11 3:46 ` Ankur Arora
2025-09-11 14:32 ` Catalin Marinas
2025-09-11 14:34 ` [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Catalin Marinas
5 siblings, 1 reply; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 3:46 UTC (permalink / raw)
To: linux-kernel, linux-arch, linux-arm-kernel, bpf
Cc: arnd, catalin.marinas, will, peterz, akpm, mark.rutland, harisokn,
cl, ast, memxor, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
Switch out the conditional load inerfaces used by rqspinlock
to smp_cond_read_acquire_timeout().
This interface handles the timeout check explicitly and does any
necessary amortization, so use check_timeout() directly.
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
kernel/bpf/rqspinlock.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
index 5ab354d55d82..4d2c12d131ae 100644
--- a/kernel/bpf/rqspinlock.c
+++ b/kernel/bpf/rqspinlock.c
@@ -241,20 +241,14 @@ static noinline int check_timeout(rqspinlock_t *lock, u32 mask,
}
/*
- * Do not amortize with spins when res_smp_cond_load_acquire is defined,
- * as the macro does internal amortization for us.
+ * Amortize timeout check for busy-wait loops.
*/
-#ifndef res_smp_cond_load_acquire
#define RES_CHECK_TIMEOUT(ts, ret, mask) \
({ \
if (!(ts).spin++) \
(ret) = check_timeout((lock), (mask), &(ts)); \
(ret); \
})
-#else
-#define RES_CHECK_TIMEOUT(ts, ret, mask) \
- ({ (ret) = check_timeout((lock), (mask), &(ts)); })
-#endif
/*
* Initialize the 'spin' member.
@@ -313,11 +307,8 @@ EXPORT_SYMBOL_GPL(resilient_tas_spin_lock);
*/
static DEFINE_PER_CPU_ALIGNED(struct qnode, rqnodes[_Q_MAX_NODES]);
-#ifndef res_smp_cond_load_acquire
-#define res_smp_cond_load_acquire(v, c) smp_cond_load_acquire(v, c)
-#endif
-
-#define res_atomic_cond_read_acquire(v, c) res_smp_cond_load_acquire(&(v)->counter, (c))
+#define res_atomic_cond_read_acquire_timeout(v, c, t) \
+ smp_cond_load_acquire_timeout(&(v)->counter, (c), (t))
/**
* resilient_queued_spin_lock_slowpath - acquire the queued spinlock
@@ -418,7 +409,8 @@ int __lockfunc resilient_queued_spin_lock_slowpath(rqspinlock_t *lock, u32 val)
*/
if (val & _Q_LOCKED_MASK) {
RES_RESET_TIMEOUT(ts, RES_DEF_TIMEOUT);
- res_smp_cond_load_acquire(&lock->locked, !VAL || RES_CHECK_TIMEOUT(ts, ret, _Q_LOCKED_MASK));
+ smp_cond_load_acquire_timeout(&lock->locked, !VAL,
+ (ret = check_timeout(lock, _Q_LOCKED_MASK, &ts)));
}
if (ret) {
@@ -572,9 +564,8 @@ int __lockfunc resilient_queued_spin_lock_slowpath(rqspinlock_t *lock, u32 val)
* us.
*/
RES_RESET_TIMEOUT(ts, RES_DEF_TIMEOUT * 2);
- val = res_atomic_cond_read_acquire(&lock->val, !(VAL & _Q_LOCKED_PENDING_MASK) ||
- RES_CHECK_TIMEOUT(ts, ret, _Q_LOCKED_PENDING_MASK));
-
+ val = res_atomic_cond_read_acquire_timeout(&lock->val, !(VAL & _Q_LOCKED_PENDING_MASK),
+ (ret = check_timeout(lock, _Q_LOCKED_PENDING_MASK, &ts)));
waitq_timeout:
if (ret) {
/*
--
2.43.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout()
2025-09-11 3:46 ` [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout() Ankur Arora
@ 2025-09-11 14:32 ` Catalin Marinas
2025-09-11 18:54 ` Kumar Kartikeya Dwivedi
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Catalin Marinas @ 2025-09-11 14:32 UTC (permalink / raw)
To: Ankur Arora
Cc: linux-kernel, linux-arch, linux-arm-kernel, bpf, arnd, will,
peterz, akpm, mark.rutland, harisokn, cl, ast, memxor,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Wed, Sep 10, 2025 at 08:46:55PM -0700, Ankur Arora wrote:
> Switch out the conditional load inerfaces used by rqspinlock
> to smp_cond_read_acquire_timeout().
> This interface handles the timeout check explicitly and does any
> necessary amortization, so use check_timeout() directly.
It's worth mentioning that the default smp_cond_load_acquire_timeout()
implementation (without hardware support) only spins 200 times instead
of 16K times in the rqspinlock code. That's probably fine but it would
be good to have confirmation from Kumar or Alexei.
> diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
> index 5ab354d55d82..4d2c12d131ae 100644
> --- a/kernel/bpf/rqspinlock.c
> +++ b/kernel/bpf/rqspinlock.c
[...]
> @@ -313,11 +307,8 @@ EXPORT_SYMBOL_GPL(resilient_tas_spin_lock);
> */
> static DEFINE_PER_CPU_ALIGNED(struct qnode, rqnodes[_Q_MAX_NODES]);
>
> -#ifndef res_smp_cond_load_acquire
> -#define res_smp_cond_load_acquire(v, c) smp_cond_load_acquire(v, c)
> -#endif
> -
> -#define res_atomic_cond_read_acquire(v, c) res_smp_cond_load_acquire(&(v)->counter, (c))
> +#define res_atomic_cond_read_acquire_timeout(v, c, t) \
> + smp_cond_load_acquire_timeout(&(v)->counter, (c), (t))
BTW, we have atomic_cond_read_acquire() which accesses the 'counter' of
an atomic_t. You might as well add an atomic_cond_read_acquire_timeout()
in atomic.h than open-code the atomic_t internals here.
Otherwise the patch looks fine to me, much simpler than the previous
attempt.
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout()
2025-09-11 3:46 [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Ankur Arora
` (4 preceding siblings ...)
2025-09-11 3:46 ` [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout() Ankur Arora
@ 2025-09-11 14:34 ` Catalin Marinas
2025-09-11 21:57 ` Ankur Arora
5 siblings, 1 reply; 23+ messages in thread
From: Catalin Marinas @ 2025-09-11 14:34 UTC (permalink / raw)
To: Ankur Arora
Cc: linux-kernel, linux-arch, linux-arm-kernel, bpf, arnd, will,
peterz, akpm, mark.rutland, harisokn, cl, ast, memxor,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Wed, Sep 10, 2025 at 08:46:50PM -0700, Ankur Arora wrote:
> This series adds waited variants of the smp_cond_load() primitives:
> smp_cond_load_relaxed_timeout(), and smp_cond_load_acquire_timeout().
>
> As the name suggests, the new interfaces are meant for contexts where
> you want to wait on a condition variable for a finite duration. This
> is easy enough to do with a loop around cpu_relax() and a periodic
> timeout check (pretty much what we do in poll_idle(). However, some
> architectures (ex. arm64) also allow waiting on a cacheline. So,
>
> smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr)
> smp_cond_load_acquire_timeout(ptr, cond_expr, time_check_expr)
>
> do a mixture of spin/wait with a smp_cond_load() thrown in.
>
> The added parameter, time_check_expr, determines the bail out condition.
>
> There are two current users for these interfaces. poll_idle() with
> the change:
>
> poll_idle() {
> ...
> time_end = local_clock_noinstr() + cpuidle_poll_time(drv, dev);
>
> raw_local_irq_enable();
> if (!current_set_polling_and_test())
> flags = smp_cond_load_relaxed_timeout(¤t_thread_info()->flags,
> (VAL & _TIF_NEED_RESCHED),
> ((local_clock_noinstr() >= time_end)));
> dev->poll_time_limit = !(flags & _TIF_NEED_RESCHED);
> raw_local_irq_disable();
> ...
> }
You should have added this as a patch in the series than include the
implementation in the cover letter.
--
Catalin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout()
2025-09-11 14:32 ` Catalin Marinas
@ 2025-09-11 18:54 ` Kumar Kartikeya Dwivedi
2025-09-11 21:58 ` Ankur Arora
2025-09-11 18:56 ` Kumar Kartikeya Dwivedi
2025-09-11 21:57 ` Ankur Arora
2 siblings, 1 reply; 23+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2025-09-11 18:54 UTC (permalink / raw)
To: Catalin Marinas
Cc: Ankur Arora, linux-kernel, linux-arch, linux-arm-kernel, bpf,
arnd, will, peterz, akpm, mark.rutland, harisokn, cl, ast,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Thu, 11 Sept 2025 at 16:32, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, Sep 10, 2025 at 08:46:55PM -0700, Ankur Arora wrote:
> > Switch out the conditional load inerfaces used by rqspinlock
> > to smp_cond_read_acquire_timeout().
> > This interface handles the timeout check explicitly and does any
> > necessary amortization, so use check_timeout() directly.
>
> It's worth mentioning that the default smp_cond_load_acquire_timeout()
> implementation (without hardware support) only spins 200 times instead
> of 16K times in the rqspinlock code. That's probably fine but it would
> be good to have confirmation from Kumar or Alexei.
>
This looks good, but I would still redefine the spin count from 200 to
16k for rqspinlock.c, especially because we need to keep
RES_CHECK_TIMEOUT around which still uses 16k spins to amortize
check_timeout.
> > [...]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout()
2025-09-11 14:32 ` Catalin Marinas
2025-09-11 18:54 ` Kumar Kartikeya Dwivedi
@ 2025-09-11 18:56 ` Kumar Kartikeya Dwivedi
2025-09-11 21:57 ` Ankur Arora
2 siblings, 0 replies; 23+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2025-09-11 18:56 UTC (permalink / raw)
To: Catalin Marinas
Cc: Ankur Arora, linux-kernel, linux-arch, linux-arm-kernel, bpf,
arnd, will, peterz, akpm, mark.rutland, harisokn, cl, ast,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Thu, 11 Sept 2025 at 16:32, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, Sep 10, 2025 at 08:46:55PM -0700, Ankur Arora wrote:
> > Switch out the conditional load inerfaces used by rqspinlock
> > to smp_cond_read_acquire_timeout().
> > This interface handles the timeout check explicitly and does any
> > necessary amortization, so use check_timeout() directly.
>
> It's worth mentioning that the default smp_cond_load_acquire_timeout()
> implementation (without hardware support) only spins 200 times instead
> of 16K times in the rqspinlock code. That's probably fine but it would
> be good to have confirmation from Kumar or Alexei.
>
> > diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
> > index 5ab354d55d82..4d2c12d131ae 100644
> > --- a/kernel/bpf/rqspinlock.c
> > +++ b/kernel/bpf/rqspinlock.c
> [...]
> > @@ -313,11 +307,8 @@ EXPORT_SYMBOL_GPL(resilient_tas_spin_lock);
> > */
> > static DEFINE_PER_CPU_ALIGNED(struct qnode, rqnodes[_Q_MAX_NODES]);
> >
> > -#ifndef res_smp_cond_load_acquire
> > -#define res_smp_cond_load_acquire(v, c) smp_cond_load_acquire(v, c)
> > -#endif
> > -
> > -#define res_atomic_cond_read_acquire(v, c) res_smp_cond_load_acquire(&(v)->counter, (c))
> > +#define res_atomic_cond_read_acquire_timeout(v, c, t) \
> > + smp_cond_load_acquire_timeout(&(v)->counter, (c), (t))
>
> BTW, we have atomic_cond_read_acquire() which accesses the 'counter' of
> an atomic_t. You might as well add an atomic_cond_read_acquire_timeout()
> in atomic.h than open-code the atomic_t internals here.
>
+1, and then drop res_atomic_cond_read_acquire_timeout from this file.
> Otherwise the patch looks fine to me, much simpler than the previous
> attempt.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout()
2025-09-11 14:34 ` [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Catalin Marinas
@ 2025-09-11 21:57 ` Ankur Arora
2025-09-15 11:12 ` Catalin Marinas
0 siblings, 1 reply; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 21:57 UTC (permalink / raw)
To: Catalin Marinas
Cc: Ankur Arora, linux-kernel, linux-arch, linux-arm-kernel, bpf,
arnd, will, peterz, akpm, mark.rutland, harisokn, cl, ast, memxor,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
Catalin Marinas <catalin.marinas@arm.com> writes:
> On Wed, Sep 10, 2025 at 08:46:50PM -0700, Ankur Arora wrote:
>> This series adds waited variants of the smp_cond_load() primitives:
>> smp_cond_load_relaxed_timeout(), and smp_cond_load_acquire_timeout().
>>
>> As the name suggests, the new interfaces are meant for contexts where
>> you want to wait on a condition variable for a finite duration. This
>> is easy enough to do with a loop around cpu_relax() and a periodic
>> timeout check (pretty much what we do in poll_idle(). However, some
>> architectures (ex. arm64) also allow waiting on a cacheline. So,
>>
>> smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr)
>> smp_cond_load_acquire_timeout(ptr, cond_expr, time_check_expr)
>>
>> do a mixture of spin/wait with a smp_cond_load() thrown in.
>>
>> The added parameter, time_check_expr, determines the bail out condition.
>>
>> There are two current users for these interfaces. poll_idle() with
>> the change:
>>
>> poll_idle() {
>> ...
>> time_end = local_clock_noinstr() + cpuidle_poll_time(drv, dev);
>>
>> raw_local_irq_enable();
>> if (!current_set_polling_and_test())
>> flags = smp_cond_load_relaxed_timeout(¤t_thread_info()->flags,
>> (VAL & _TIF_NEED_RESCHED),
>> ((local_clock_noinstr() >= time_end)));
>> dev->poll_time_limit = !(flags & _TIF_NEED_RESCHED);
>> raw_local_irq_disable();
>> ...
>> }
>
> You should have added this as a patch in the series than include the
> implementation in the cover letter.
This was probably an overkill but I wanted to not add another subsystem
to this series.
Will take care of the cpuidle changes in the arm64 polling in idle series.
--
ankur
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout()
2025-09-11 14:32 ` Catalin Marinas
2025-09-11 18:54 ` Kumar Kartikeya Dwivedi
2025-09-11 18:56 ` Kumar Kartikeya Dwivedi
@ 2025-09-11 21:57 ` Ankur Arora
2 siblings, 0 replies; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 21:57 UTC (permalink / raw)
To: Catalin Marinas
Cc: Ankur Arora, linux-kernel, linux-arch, linux-arm-kernel, bpf,
arnd, will, peterz, akpm, mark.rutland, harisokn, cl, ast, memxor,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
Catalin Marinas <catalin.marinas@arm.com> writes:
> On Wed, Sep 10, 2025 at 08:46:55PM -0700, Ankur Arora wrote:
>> Switch out the conditional load inerfaces used by rqspinlock
>> to smp_cond_read_acquire_timeout().
>> This interface handles the timeout check explicitly and does any
>> necessary amortization, so use check_timeout() directly.
>
> It's worth mentioning that the default smp_cond_load_acquire_timeout()
> implementation (without hardware support) only spins 200 times instead
> of 16K times in the rqspinlock code. That's probably fine but it would
> be good to have confirmation from Kumar or Alexei.
As Kumar mentions, I'll redefine the count locally in rqspinlock.c to 16k.
>> diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
>> index 5ab354d55d82..4d2c12d131ae 100644
>> --- a/kernel/bpf/rqspinlock.c
>> +++ b/kernel/bpf/rqspinlock.c
> [...]
>> @@ -313,11 +307,8 @@ EXPORT_SYMBOL_GPL(resilient_tas_spin_lock);
>> */
>> static DEFINE_PER_CPU_ALIGNED(struct qnode, rqnodes[_Q_MAX_NODES]);
>>
>> -#ifndef res_smp_cond_load_acquire
>> -#define res_smp_cond_load_acquire(v, c) smp_cond_load_acquire(v, c)
>> -#endif
>> -
>> -#define res_atomic_cond_read_acquire(v, c) res_smp_cond_load_acquire(&(v)->counter, (c))
>> +#define res_atomic_cond_read_acquire_timeout(v, c, t) \
>> + smp_cond_load_acquire_timeout(&(v)->counter, (c), (t))
>
> BTW, we have atomic_cond_read_acquire() which accesses the 'counter' of
> an atomic_t. You might as well add an atomic_cond_read_acquire_timeout()
> in atomic.h than open-code the atomic_t internals here.
Good point. That also keeps it close to the locking/qspinlock.c
use of atomic_cond_read_acquire().
Will add atomic_cond_read_acquire_timeout() (and other variants that
we define in include/linux/atomic.h).
> Otherwise the patch looks fine to me, much simpler than the previous
> attempt.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Thanks!
--
ankur
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout()
2025-09-11 18:54 ` Kumar Kartikeya Dwivedi
@ 2025-09-11 21:58 ` Ankur Arora
2025-09-12 10:14 ` Catalin Marinas
0 siblings, 1 reply; 23+ messages in thread
From: Ankur Arora @ 2025-09-11 21:58 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: Catalin Marinas, Ankur Arora, linux-kernel, linux-arch,
linux-arm-kernel, bpf, arnd, will, peterz, akpm, mark.rutland,
harisokn, cl, ast, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:
> On Thu, 11 Sept 2025 at 16:32, Catalin Marinas <catalin.marinas@arm.com> wrote:
>>
>> On Wed, Sep 10, 2025 at 08:46:55PM -0700, Ankur Arora wrote:
>> > Switch out the conditional load inerfaces used by rqspinlock
>> > to smp_cond_read_acquire_timeout().
>> > This interface handles the timeout check explicitly and does any
>> > necessary amortization, so use check_timeout() directly.
>>
>> It's worth mentioning that the default smp_cond_load_acquire_timeout()
>> implementation (without hardware support) only spins 200 times instead
>> of 16K times in the rqspinlock code. That's probably fine but it would
>> be good to have confirmation from Kumar or Alexei.
>>
>
> This looks good, but I would still redefine the spin count from 200 to
> 16k for rqspinlock.c, especially because we need to keep
> RES_CHECK_TIMEOUT around which still uses 16k spins to amortize
> check_timeout.
By my count that amounts to ~100us per check_timeout() on x86
systems I've tested with cpu_relax(). Which seems quite reasonable.
16k also seems safer on CPUs where cpu_relax() is basically a NOP.
--
ankur
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout()
2025-09-11 21:58 ` Ankur Arora
@ 2025-09-12 10:14 ` Catalin Marinas
2025-09-12 18:06 ` Ankur Arora
0 siblings, 1 reply; 23+ messages in thread
From: Catalin Marinas @ 2025-09-12 10:14 UTC (permalink / raw)
To: Ankur Arora
Cc: Kumar Kartikeya Dwivedi, linux-kernel, linux-arch,
linux-arm-kernel, bpf, arnd, will, peterz, akpm, mark.rutland,
harisokn, cl, ast, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
On Thu, Sep 11, 2025 at 02:58:22PM -0700, Ankur Arora wrote:
>
> Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:
>
> > On Thu, 11 Sept 2025 at 16:32, Catalin Marinas <catalin.marinas@arm.com> wrote:
> >>
> >> On Wed, Sep 10, 2025 at 08:46:55PM -0700, Ankur Arora wrote:
> >> > Switch out the conditional load inerfaces used by rqspinlock
> >> > to smp_cond_read_acquire_timeout().
> >> > This interface handles the timeout check explicitly and does any
> >> > necessary amortization, so use check_timeout() directly.
> >>
> >> It's worth mentioning that the default smp_cond_load_acquire_timeout()
> >> implementation (without hardware support) only spins 200 times instead
> >> of 16K times in the rqspinlock code. That's probably fine but it would
> >> be good to have confirmation from Kumar or Alexei.
> >>
> >
> > This looks good, but I would still redefine the spin count from 200 to
> > 16k for rqspinlock.c, especially because we need to keep
> > RES_CHECK_TIMEOUT around which still uses 16k spins to amortize
> > check_timeout.
>
> By my count that amounts to ~100us per check_timeout() on x86
> systems I've tested with cpu_relax(). Which seems quite reasonable.
>
> 16k also seems safer on CPUs where cpu_relax() is basically a NOP.
Does this spin count work for poll_idle()? I don't remember where the
200 value came from.
--
Catalin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout()
2025-09-12 10:14 ` Catalin Marinas
@ 2025-09-12 18:06 ` Ankur Arora
0 siblings, 0 replies; 23+ messages in thread
From: Ankur Arora @ 2025-09-12 18:06 UTC (permalink / raw)
To: Catalin Marinas
Cc: Ankur Arora, Kumar Kartikeya Dwivedi, linux-kernel, linux-arch,
linux-arm-kernel, bpf, arnd, will, peterz, akpm, mark.rutland,
harisokn, cl, ast, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
Catalin Marinas <catalin.marinas@arm.com> writes:
> On Thu, Sep 11, 2025 at 02:58:22PM -0700, Ankur Arora wrote:
>>
>> Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:
>>
>> > On Thu, 11 Sept 2025 at 16:32, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> >>
>> >> On Wed, Sep 10, 2025 at 08:46:55PM -0700, Ankur Arora wrote:
>> >> > Switch out the conditional load inerfaces used by rqspinlock
>> >> > to smp_cond_read_acquire_timeout().
>> >> > This interface handles the timeout check explicitly and does any
>> >> > necessary amortization, so use check_timeout() directly.
>> >>
>> >> It's worth mentioning that the default smp_cond_load_acquire_timeout()
>> >> implementation (without hardware support) only spins 200 times instead
>> >> of 16K times in the rqspinlock code. That's probably fine but it would
>> >> be good to have confirmation from Kumar or Alexei.
>> >>
>> >
>> > This looks good, but I would still redefine the spin count from 200 to
>> > 16k for rqspinlock.c, especially because we need to keep
>> > RES_CHECK_TIMEOUT around which still uses 16k spins to amortize
>> > check_timeout.
>>
>> By my count that amounts to ~100us per check_timeout() on x86
>> systems I've tested with cpu_relax(). Which seems quite reasonable.
>>
>> 16k also seems safer on CPUs where cpu_relax() is basically a NOP.
>
> Does this spin count work for poll_idle()? I don't remember where the
> 200 value came from.
Just reusing the value of POLL_IDLE_RELAX_COUNT which is is defined as
200.
For the poll_idle() case I don't think the value of 200 makes sense
for all architectures, so they'll need to redefine it (before defining
ARCH_HAS_OPTIMIZED_POLL which gates poll_idle().)
--
ankur
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout()
2025-09-11 21:57 ` Ankur Arora
@ 2025-09-15 11:12 ` Catalin Marinas
2025-09-16 5:29 ` Ankur Arora
0 siblings, 1 reply; 23+ messages in thread
From: Catalin Marinas @ 2025-09-15 11:12 UTC (permalink / raw)
To: Ankur Arora
Cc: linux-kernel, linux-arch, linux-arm-kernel, bpf, arnd, will,
peterz, akpm, mark.rutland, harisokn, cl, ast, memxor,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Thu, Sep 11, 2025 at 02:57:52PM -0700, Ankur Arora wrote:
> Catalin Marinas <catalin.marinas@arm.com> writes:
> > On Wed, Sep 10, 2025 at 08:46:50PM -0700, Ankur Arora wrote:
> >> This series adds waited variants of the smp_cond_load() primitives:
> >> smp_cond_load_relaxed_timeout(), and smp_cond_load_acquire_timeout().
> >>
> >> As the name suggests, the new interfaces are meant for contexts where
> >> you want to wait on a condition variable for a finite duration. This
> >> is easy enough to do with a loop around cpu_relax() and a periodic
> >> timeout check (pretty much what we do in poll_idle(). However, some
> >> architectures (ex. arm64) also allow waiting on a cacheline. So,
> >>
> >> smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr)
> >> smp_cond_load_acquire_timeout(ptr, cond_expr, time_check_expr)
> >>
> >> do a mixture of spin/wait with a smp_cond_load() thrown in.
> >>
> >> The added parameter, time_check_expr, determines the bail out condition.
> >>
> >> There are two current users for these interfaces. poll_idle() with
> >> the change:
> >>
> >> poll_idle() {
> >> ...
> >> time_end = local_clock_noinstr() + cpuidle_poll_time(drv, dev);
> >>
> >> raw_local_irq_enable();
> >> if (!current_set_polling_and_test())
> >> flags = smp_cond_load_relaxed_timeout(¤t_thread_info()->flags,
> >> (VAL & _TIF_NEED_RESCHED),
> >> ((local_clock_noinstr() >= time_end)));
> >> dev->poll_time_limit = !(flags & _TIF_NEED_RESCHED);
> >> raw_local_irq_disable();
> >> ...
> >> }
> >
> > You should have added this as a patch in the series than include the
> > implementation in the cover letter.
>
> This was probably an overkill but I wanted to not add another subsystem
> to this series.
If you include it, it's easier to poke the cpuidle maintainers and ask
if they are ok with the proposed API as I want to avoid changing it
afterwards. It doesn't mean they'll have to be merged together, they can
go upstream via separate routes.
> Will take care of the cpuidle changes in the arm64 polling in idle series.
Thanks. We also need Will, Peter Z and Arnd to ack the API and the
generic changes (probably once you added the linux/atomic.h changes).
--
Catalin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout()
2025-09-15 11:12 ` Catalin Marinas
@ 2025-09-16 5:29 ` Ankur Arora
0 siblings, 0 replies; 23+ messages in thread
From: Ankur Arora @ 2025-09-16 5:29 UTC (permalink / raw)
To: Catalin Marinas
Cc: Ankur Arora, linux-kernel, linux-arch, linux-arm-kernel, bpf,
arnd, will, peterz, akpm, mark.rutland, harisokn, cl, ast, memxor,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
Catalin Marinas <catalin.marinas@arm.com> writes:
> On Thu, Sep 11, 2025 at 02:57:52PM -0700, Ankur Arora wrote:
>> Catalin Marinas <catalin.marinas@arm.com> writes:
>> > On Wed, Sep 10, 2025 at 08:46:50PM -0700, Ankur Arora wrote:
>> >> This series adds waited variants of the smp_cond_load() primitives:
>> >> smp_cond_load_relaxed_timeout(), and smp_cond_load_acquire_timeout().
>> >>
>> >> As the name suggests, the new interfaces are meant for contexts where
>> >> you want to wait on a condition variable for a finite duration. This
>> >> is easy enough to do with a loop around cpu_relax() and a periodic
>> >> timeout check (pretty much what we do in poll_idle(). However, some
>> >> architectures (ex. arm64) also allow waiting on a cacheline. So,
>> >>
>> >> smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr)
>> >> smp_cond_load_acquire_timeout(ptr, cond_expr, time_check_expr)
>> >>
>> >> do a mixture of spin/wait with a smp_cond_load() thrown in.
>> >>
>> >> The added parameter, time_check_expr, determines the bail out condition.
>> >>
>> >> There are two current users for these interfaces. poll_idle() with
>> >> the change:
>> >>
>> >> poll_idle() {
>> >> ...
>> >> time_end = local_clock_noinstr() + cpuidle_poll_time(drv, dev);
>> >>
>> >> raw_local_irq_enable();
>> >> if (!current_set_polling_and_test())
>> >> flags = smp_cond_load_relaxed_timeout(¤t_thread_info()->flags,
>> >> (VAL & _TIF_NEED_RESCHED),
>> >> ((local_clock_noinstr() >= time_end)));
>> >> dev->poll_time_limit = !(flags & _TIF_NEED_RESCHED);
>> >> raw_local_irq_disable();
>> >> ...
>> >> }
>> >
>> > You should have added this as a patch in the series than include the
>> > implementation in the cover letter.
>>
>> This was probably an overkill but I wanted to not add another subsystem
>> to this series.
>
> If you include it, it's easier to poke the cpuidle maintainers and ask
> if they are ok with the proposed API as I want to avoid changing it
> afterwards. It doesn't mean they'll have to be merged together, they can
> go upstream via separate routes.
That makes a lot of sense. Will include this patch as well.
>> Will take care of the cpuidle changes in the arm64 polling in idle series.
>
> Thanks. We also need Will, Peter Z and Arnd to ack the API and the
> generic changes (probably once you added the linux/atomic.h changes).
Makes sense.
Thanks
--
ankur
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()
2025-09-11 3:46 ` [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout() Ankur Arora
@ 2025-09-18 19:42 ` Will Deacon
2025-09-19 23:41 ` Ankur Arora
0 siblings, 1 reply; 23+ messages in thread
From: Will Deacon @ 2025-09-18 19:42 UTC (permalink / raw)
To: Ankur Arora
Cc: linux-kernel, linux-arch, linux-arm-kernel, bpf, arnd,
catalin.marinas, peterz, akpm, mark.rutland, harisokn, cl, ast,
memxor, zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Wed, Sep 10, 2025 at 08:46:51PM -0700, Ankur Arora wrote:
> Add smp_cond_load_relaxed_timeout(), which extends
> smp_cond_load_relaxed() to allow waiting for a duration.
>
> The additional parameter allows for the timeout check.
>
> The waiting is done via the usual cpu_relax() spin-wait around the
> condition variable with periodic evaluation of the time-check.
>
> The number of times we spin is defined by SMP_TIMEOUT_SPIN_COUNT
> (chosen to be 200 by default) which, assuming each cpu_relax()
> iteration takes around 20-30 cycles (measured on a variety of x86
> platforms), amounts to around 4000-6000 cycles.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Will Deacon <will@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-arch@vger.kernel.org
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Reviewed-by: Haris Okanovic <harisokn@amazon.com>
> Tested-by: Haris Okanovic <harisokn@amazon.com>
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
> include/asm-generic/barrier.h | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index d4f581c1e21d..8483e139954f 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -273,6 +273,41 @@ do { \
> })
> #endif
>
> +#ifndef SMP_TIMEOUT_SPIN_COUNT
> +#define SMP_TIMEOUT_SPIN_COUNT 200
> +#endif
> +
> +/**
> + * smp_cond_load_relaxed_timeout() - (Spin) wait for cond with no ordering
> + * guarantees until a timeout expires.
> + * @ptr: pointer to the variable to wait on
> + * @cond: boolean expression to wait for
> + * @time_check_expr: expression to decide when to bail out
> + *
> + * Equivalent to using READ_ONCE() on the condition variable.
> + */
> +#ifndef smp_cond_load_relaxed_timeout
> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
> +({ \
> + typeof(ptr) __PTR = (ptr); \
> + __unqual_scalar_typeof(*ptr) VAL; \
> + u32 __n = 0, __spin = SMP_TIMEOUT_SPIN_COUNT; \
> + \
> + for (;;) { \
> + VAL = READ_ONCE(*__PTR); \
> + if (cond_expr) \
> + break; \
> + cpu_relax(); \
> + if (++__n < __spin) \
> + continue; \
> + if (time_check_expr) \
> + break; \
There's a funny discrepancy here when compared to the arm64 version in
the next patch. Here, if we time out, then the value returned is
potentially quite stale because it was read before the last cpu_relax().
In the arm64 patch, the timeout check is before the cmpwait/cpu_relax(),
which I think is better.
Regardless, I think having the same behaviour for the two implementations
would be a good idea.
Will
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/5] arm64: barrier: Add smp_cond_load_relaxed_timeout()
2025-09-11 3:46 ` [PATCH v5 2/5] arm64: " Ankur Arora
@ 2025-09-18 20:05 ` Will Deacon
2025-09-19 16:18 ` Catalin Marinas
0 siblings, 1 reply; 23+ messages in thread
From: Will Deacon @ 2025-09-18 20:05 UTC (permalink / raw)
To: Ankur Arora
Cc: linux-kernel, linux-arch, linux-arm-kernel, bpf, arnd,
catalin.marinas, peterz, akpm, mark.rutland, harisokn, cl, ast,
memxor, zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Wed, Sep 10, 2025 at 08:46:52PM -0700, Ankur Arora wrote:
> Add smp_cond_load_relaxed_timeout(), a timed variant of
> smp_cond_load_relaxed().
>
> This uses __cmpwait_relaxed() to do the actual waiting, with the
> event-stream guaranteeing that we wake up from WFE periodically
> and not block forever in case there are no stores to the cacheline.
>
> For cases when the event-stream is unavailable, fallback to
> spin-waiting.
>
> Cc: Will Deacon <will@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Reviewed-by: Haris Okanovic <harisokn@amazon.com>
> Tested-by: Haris Okanovic <harisokn@amazon.com>
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
> arch/arm64/include/asm/barrier.h | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index f5801b0ba9e9..4f0d9ed7a072 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h
> @@ -219,6 +219,29 @@ do { \
> (typeof(*ptr))VAL; \
> })
>
> +/* Re-declared here to avoid include dependency. */
> +extern bool arch_timer_evtstrm_available(void);
> +
> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
> +({ \
> + typeof(ptr) __PTR = (ptr); \
> + __unqual_scalar_typeof(*ptr) VAL; \
> + bool __wfe = arch_timer_evtstrm_available(); \
> + \
> + for (;;) { \
> + VAL = READ_ONCE(*__PTR); \
> + if (cond_expr) \
> + break; \
> + if (time_check_expr) \
> + break; \
> + if (likely(__wfe)) \
> + __cmpwait_relaxed(__PTR, VAL); \
> + else \
> + cpu_relax(); \
It'd be an awful lot nicer if we could just use the generic code if
wfe isn't available. One option would be to make that available as
e.g. __smp_cond_load_relaxed_timeout_cpu_relax() and call it from the
arch code when !arch_timer_evtstrm_available() but a potentially cleaner
version would be to introduce something like cpu_poll_relax() and use
that in the core code.
So arm64 would do:
#define SMP_TIMEOUT_SPIN_COUNT 1
#define cpu_poll_relax(ptr, val) do { \
if (arch_timer_evtstrm_available()) \
__cmpwait_relaxed(ptr, val); \
else \
cpu_relax(); \
} while (0)
and then the core code would have:
#ifndef cpu_poll_relax
#define cpu_poll_relax(p, v) cpu_relax()
#endif
and could just use cpu_poll_relax() in the generic implementation of
smp_cond_load_relaxed_timeout().
Will
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/5] arm64: barrier: Add smp_cond_load_relaxed_timeout()
2025-09-18 20:05 ` Will Deacon
@ 2025-09-19 16:18 ` Catalin Marinas
2025-09-19 22:39 ` Ankur Arora
0 siblings, 1 reply; 23+ messages in thread
From: Catalin Marinas @ 2025-09-19 16:18 UTC (permalink / raw)
To: Will Deacon
Cc: Ankur Arora, linux-kernel, linux-arch, linux-arm-kernel, bpf,
arnd, peterz, akpm, mark.rutland, harisokn, cl, ast, memxor,
zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Thu, Sep 18, 2025 at 09:05:22PM +0100, Will Deacon wrote:
> > +/* Re-declared here to avoid include dependency. */
> > +extern bool arch_timer_evtstrm_available(void);
> > +
> > +#define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
> > +({ \
> > + typeof(ptr) __PTR = (ptr); \
> > + __unqual_scalar_typeof(*ptr) VAL; \
> > + bool __wfe = arch_timer_evtstrm_available(); \
> > + \
> > + for (;;) { \
> > + VAL = READ_ONCE(*__PTR); \
> > + if (cond_expr) \
> > + break; \
> > + if (time_check_expr) \
> > + break; \
> > + if (likely(__wfe)) \
> > + __cmpwait_relaxed(__PTR, VAL); \
> > + else \
> > + cpu_relax(); \
>
> It'd be an awful lot nicer if we could just use the generic code if
> wfe isn't available. One option would be to make that available as
> e.g. __smp_cond_load_relaxed_timeout_cpu_relax() and call it from the
> arch code when !arch_timer_evtstrm_available() but a potentially cleaner
> version would be to introduce something like cpu_poll_relax() and use
> that in the core code.
>
> So arm64 would do:
>
> #define SMP_TIMEOUT_SPIN_COUNT 1
> #define cpu_poll_relax(ptr, val) do { \
> if (arch_timer_evtstrm_available()) \
> __cmpwait_relaxed(ptr, val); \
> else \
> cpu_relax(); \
> } while (0)
>
> and then the core code would have:
>
> #ifndef cpu_poll_relax
> #define cpu_poll_relax(p, v) cpu_relax()
> #endif
A slight problem here is that we have two users that want different spin
counts: poll_idle() uses 200, rqspinlock wants 16K. They've been
empirically chosen but I guess it also depends on what they call in
time_check_expr and the resolution they need. From the discussion on
patch 5, Kumar would like to override the spin count to 16K from the
current one of 200 (or if poll_idle works with 16K, we just set that as
the default; we have yet to hear from the cpuidle folk).
I guess on arm64 we'd first #undef it and redefine it as 1. The
non-event stream variant is for debug only really, I'd expect it to
always have it on in production (or go for WFET).
So yeah, I think the above would work. Ankur proposed something similar
in the early versions but I found it too complicated (a spin and wait
policy callback populating the spin variable). Your proposal looks a lot
simpler.
Thanks.
--
Catalin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/5] arm64: barrier: Add smp_cond_load_relaxed_timeout()
2025-09-19 16:18 ` Catalin Marinas
@ 2025-09-19 22:39 ` Ankur Arora
0 siblings, 0 replies; 23+ messages in thread
From: Ankur Arora @ 2025-09-19 22:39 UTC (permalink / raw)
To: Catalin Marinas
Cc: Will Deacon, Ankur Arora, linux-kernel, linux-arch,
linux-arm-kernel, bpf, arnd, peterz, akpm, mark.rutland, harisokn,
cl, ast, memxor, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
Catalin Marinas <catalin.marinas@arm.com> writes:
> On Thu, Sep 18, 2025 at 09:05:22PM +0100, Will Deacon wrote:
>> > +/* Re-declared here to avoid include dependency. */
>> > +extern bool arch_timer_evtstrm_available(void);
>> > +
>> > +#define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
>> > +({ \
>> > + typeof(ptr) __PTR = (ptr); \
>> > + __unqual_scalar_typeof(*ptr) VAL; \
>> > + bool __wfe = arch_timer_evtstrm_available(); \
>> > + \
>> > + for (;;) { \
>> > + VAL = READ_ONCE(*__PTR); \
>> > + if (cond_expr) \
>> > + break; \
>> > + if (time_check_expr) \
>> > + break; \
>> > + if (likely(__wfe)) \
>> > + __cmpwait_relaxed(__PTR, VAL); \
>> > + else \
>> > + cpu_relax(); \
>>
>> It'd be an awful lot nicer if we could just use the generic code if
>> wfe isn't available. One option would be to make that available as
>> e.g. __smp_cond_load_relaxed_timeout_cpu_relax() and call it from the
>> arch code when !arch_timer_evtstrm_available() but a potentially cleaner
>> version would be to introduce something like cpu_poll_relax() and use
>> that in the core code.
>>
>> So arm64 would do:
>>
>> #define SMP_TIMEOUT_SPIN_COUNT 1
>> #define cpu_poll_relax(ptr, val) do { \
>> if (arch_timer_evtstrm_available()) \
>> __cmpwait_relaxed(ptr, val); \
>> else \
>> cpu_relax(); \
>> } while (0)
>>
>> and then the core code would have:
>>
>> #ifndef cpu_poll_relax
>> #define cpu_poll_relax(p, v) cpu_relax()
>> #endif
>
> A slight problem here is that we have two users that want different spin
> counts: poll_idle() uses 200, rqspinlock wants 16K. They've been
> empirically chosen but I guess it also depends on what they call in
> time_check_expr and the resolution they need. From the discussion on
> patch 5, Kumar would like to override the spin count to 16K from the
> current one of 200 (or if poll_idle works with 16K, we just set that as
> the default; we have yet to hear from the cpuidle folk).
>
> I guess on arm64 we'd first #undef it and redefine it as 1.
I think you mean 16k? I have some (small) misgivings about that code
choosing the same value for all platforms but that could easily be
addressed if it becomes an issue at some point.
> The
> non-event stream variant is for debug only really, I'd expect it to
> always have it on in production (or go for WFET).
> So yeah, I think the above would work. Ankur proposed something similar
> in the early versions but I found it too complicated (a spin and wait
> policy callback populating the spin variable). Your proposal looks a lot
> simpler.
Yeah. This looks a much simpler way of abstracting the choice of the mechanism,
polling/waiting/some mixture to the architecture without needing any separate
policy etc.
--
ankur
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()
2025-09-18 19:42 ` Will Deacon
@ 2025-09-19 23:41 ` Ankur Arora
2025-09-22 10:47 ` Will Deacon
0 siblings, 1 reply; 23+ messages in thread
From: Ankur Arora @ 2025-09-19 23:41 UTC (permalink / raw)
To: Will Deacon
Cc: Ankur Arora, linux-kernel, linux-arch, linux-arm-kernel, bpf,
arnd, catalin.marinas, peterz, akpm, mark.rutland, harisokn, cl,
ast, memxor, zhenglifeng1, xueshuai, joao.m.martins,
boris.ostrovsky, konrad.wilk
Will Deacon <will@kernel.org> writes:
> On Wed, Sep 10, 2025 at 08:46:51PM -0700, Ankur Arora wrote:
>> Add smp_cond_load_relaxed_timeout(), which extends
>> smp_cond_load_relaxed() to allow waiting for a duration.
>>
>> The additional parameter allows for the timeout check.
>>
>> The waiting is done via the usual cpu_relax() spin-wait around the
>> condition variable with periodic evaluation of the time-check.
>>
>> The number of times we spin is defined by SMP_TIMEOUT_SPIN_COUNT
>> (chosen to be 200 by default) which, assuming each cpu_relax()
>> iteration takes around 20-30 cycles (measured on a variety of x86
>> platforms), amounts to around 4000-6000 cycles.
>>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: linux-arch@vger.kernel.org
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>> Reviewed-by: Haris Okanovic <harisokn@amazon.com>
>> Tested-by: Haris Okanovic <harisokn@amazon.com>
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> ---
>> include/asm-generic/barrier.h | 35 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
>> index d4f581c1e21d..8483e139954f 100644
>> --- a/include/asm-generic/barrier.h
>> +++ b/include/asm-generic/barrier.h
>> @@ -273,6 +273,41 @@ do { \
>> })
>> #endif
>>
>> +#ifndef SMP_TIMEOUT_SPIN_COUNT
>> +#define SMP_TIMEOUT_SPIN_COUNT 200
>> +#endif
>> +
>> +/**
>> + * smp_cond_load_relaxed_timeout() - (Spin) wait for cond with no ordering
>> + * guarantees until a timeout expires.
>> + * @ptr: pointer to the variable to wait on
>> + * @cond: boolean expression to wait for
>> + * @time_check_expr: expression to decide when to bail out
>> + *
>> + * Equivalent to using READ_ONCE() on the condition variable.
>> + */
>> +#ifndef smp_cond_load_relaxed_timeout
>> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
>> +({ \
>> + typeof(ptr) __PTR = (ptr); \
>> + __unqual_scalar_typeof(*ptr) VAL; \
>> + u32 __n = 0, __spin = SMP_TIMEOUT_SPIN_COUNT; \
>> + \
>> + for (;;) { \
>> + VAL = READ_ONCE(*__PTR); \
>> + if (cond_expr) \
>> + break; \
>> + cpu_relax(); \
>> + if (++__n < __spin) \
>> + continue; \
>> + if (time_check_expr) \
>> + break; \
>
> There's a funny discrepancy here when compared to the arm64 version in
> the next patch. Here, if we time out, then the value returned is
> potentially quite stale because it was read before the last cpu_relax().
> In the arm64 patch, the timeout check is before the cmpwait/cpu_relax(),
> which I think is better.
So, that's a good point. But, the return value being stale also seems to
be incorrect.
> Regardless, I think having the same behaviour for the two implementations
> would be a good idea.
Yeah agreed.
As you outlined in the other mail, how about something like this:
#ifndef smp_cond_load_relaxed_timeout
#define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
({ \
typeof(ptr) __PTR = (ptr); \
__unqual_scalar_typeof(*ptr) VAL; \
u32 __n = 0, __poll = SMP_TIMEOUT_POLL_COUNT; \
\
for (;;) { \
VAL = READ_ONCE(*__PTR); \
if (cond_expr) \
break; \
cpu_poll_relax(); \
if (++__n < __poll) \
continue; \
if (time_check_expr) { \
VAL = READ_ONCE(*__PTR); \
break; \
} \
__n = 0; \
} \
(typeof(*ptr))VAL; \
})
#endif
A bit uglier but if the cpu_poll_relax() was a successful WFE then the
value might be ~100us out of date.
Another option might be to just set some state in the time check and
bail out due to a "if (cond_expr || __timed_out)", but I don't want
to add more instructions in the spin path.
--
ankur
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()
2025-09-19 23:41 ` Ankur Arora
@ 2025-09-22 10:47 ` Will Deacon
0 siblings, 0 replies; 23+ messages in thread
From: Will Deacon @ 2025-09-22 10:47 UTC (permalink / raw)
To: Ankur Arora
Cc: linux-kernel, linux-arch, linux-arm-kernel, bpf, arnd,
catalin.marinas, peterz, akpm, mark.rutland, harisokn, cl, ast,
memxor, zhenglifeng1, xueshuai, joao.m.martins, boris.ostrovsky,
konrad.wilk
On Fri, Sep 19, 2025 at 04:41:56PM -0700, Ankur Arora wrote:
> Will Deacon <will@kernel.org> writes:
> > On Wed, Sep 10, 2025 at 08:46:51PM -0700, Ankur Arora wrote:
> >> + for (;;) { \
> >> + VAL = READ_ONCE(*__PTR); \
> >> + if (cond_expr) \
> >> + break; \
> >> + cpu_relax(); \
> >> + if (++__n < __spin) \
> >> + continue; \
> >> + if (time_check_expr) \
> >> + break; \
> >
> > There's a funny discrepancy here when compared to the arm64 version in
> > the next patch. Here, if we time out, then the value returned is
> > potentially quite stale because it was read before the last cpu_relax().
> > In the arm64 patch, the timeout check is before the cmpwait/cpu_relax(),
> > which I think is better.
>
> So, that's a good point. But, the return value being stale also seems to
> be incorrect.
>
> > Regardless, I think having the same behaviour for the two implementations
> > would be a good idea.
>
> Yeah agreed.
>
> As you outlined in the other mail, how about something like this:
>
> #ifndef smp_cond_load_relaxed_timeout
> #define smp_cond_load_relaxed_timeout(ptr, cond_expr, time_check_expr) \
> ({ \
> typeof(ptr) __PTR = (ptr); \
> __unqual_scalar_typeof(*ptr) VAL; \
> u32 __n = 0, __poll = SMP_TIMEOUT_POLL_COUNT; \
> \
> for (;;) { \
> VAL = READ_ONCE(*__PTR); \
> if (cond_expr) \
> break; \
> cpu_poll_relax(); \
> if (++__n < __poll) \
> continue; \
> if (time_check_expr) { \
> VAL = READ_ONCE(*__PTR); \
> break; \
> } \
> __n = 0; \
> } \
> (typeof(*ptr))VAL; \
> })
> #endif
That looks better to me, thanks.
Will
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2025-09-22 10:47 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-11 3:46 [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Ankur Arora
2025-09-11 3:46 ` [PATCH v5 1/5] asm-generic: barrier: Add smp_cond_load_relaxed_timeout() Ankur Arora
2025-09-18 19:42 ` Will Deacon
2025-09-19 23:41 ` Ankur Arora
2025-09-22 10:47 ` Will Deacon
2025-09-11 3:46 ` [PATCH v5 2/5] arm64: " Ankur Arora
2025-09-18 20:05 ` Will Deacon
2025-09-19 16:18 ` Catalin Marinas
2025-09-19 22:39 ` Ankur Arora
2025-09-11 3:46 ` [PATCH v5 3/5] arm64: rqspinlock: Remove private copy of smp_cond_load_acquire_timewait Ankur Arora
2025-09-11 3:46 ` [PATCH v5 4/5] asm-generic: barrier: Add smp_cond_load_acquire_timeout() Ankur Arora
2025-09-11 3:46 ` [PATCH v5 5/5] rqspinlock: Use smp_cond_load_acquire_timeout() Ankur Arora
2025-09-11 14:32 ` Catalin Marinas
2025-09-11 18:54 ` Kumar Kartikeya Dwivedi
2025-09-11 21:58 ` Ankur Arora
2025-09-12 10:14 ` Catalin Marinas
2025-09-12 18:06 ` Ankur Arora
2025-09-11 18:56 ` Kumar Kartikeya Dwivedi
2025-09-11 21:57 ` Ankur Arora
2025-09-11 14:34 ` [PATCH v5 0/5] barrier: Add smp_cond_load_*_timeout() Catalin Marinas
2025-09-11 21:57 ` Ankur Arora
2025-09-15 11:12 ` Catalin Marinas
2025-09-16 5:29 ` Ankur Arora
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).