Re: [PATCH v16 02/16] arm64: barrier: Support smp_cond_load_relaxed_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64/barrier: massive timekeeping contention in spin loops without event stream
- [Medium] arm64: barrier: missing export of arch_timer_evtstrm_available() breaks modules
- [Medium] arm64: noinstr violation in cpuidle via cpu_poll_relax()
--

commit 7a1bb257570118eb8f711a76f172539e20eeaac8
Author: Ankur Arora <ankur.a.arora@oracle.com>

arm64: barrier: Support smp_cond_load_relaxed_timeout()

This commit supports waiting in smp_cond_load_relaxed_timeout() via
__cmpwait_relaxed() when the event-stream is available, falling back to
cpu_relax() otherwise. It defines CPU_POLL_RELAX_WAITS to indicate that
there is an optimized wait implementation.

> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index 9495c4441a46b..e2263c754a550 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h

[ ... ]

> @@ -219,6 +220,27 @@ do {									\
>  	(typeof(*ptr))VAL;						\
>  })
>  
> +/* Re-declared here to avoid include dependency. */
> +extern bool arch_timer_evtstrm_available(void);
> +
> +/*
> + * In the common case, cpu_poll_relax() sits waiting in __cmpwait_relaxed()
> + * for @ptr value to change.
> + *
> + * State this by defining CPU_POLL_RELAX_WAITS. This enables a time-check
> + * optimization in smp_cond_load_{relaxed,acquire}_timeout(). Note that this
> + * is not ideal for the cpu_relax() fallback but that's not a production
> + * configuration.
> + */
> +#define CPU_POLL_RELAX_WAITS

[Severity: High]
Does defining CPU_POLL_RELAX_WAITS here cause massive timekeeping contention
when the event stream is disabled?

When CPU_POLL_RELAX_WAITS is defined, include/asm-generic/barrier.h sets
SMP_TIMEOUT_POLL_COUNT to 1. This causes the loop in
__smp_cond_load_relaxed_timeout() to evaluate the expensive time_expr_ns
on every single iteration.

If the event stream is disabled, cpu_poll_relax() drops back to a fast
cpu_relax() yield. Evaluating the time expression (e.g. reading timer hardware
or seqcounts) on every fast spin loop iteration across multiple CPUs can cause
massive lock contention and interconnect flooding.

Is it accurate that disabling the event stream is "not a production
configuration"? It can be disabled via Kconfig
(CONFIG_ARM_ARCH_TIMER_EVTSTREAM=n), kernel command line
(clocksource.arm_arch_timer.evtstrm=0), or under hypervisors that lack
virtual event stream support.

> +
> +#define cpu_poll_relax(ptr, val, timeout_ns) do {			\
> +	if (arch_timer_evtstrm_available())				\

[Severity: Medium]
Does this break the build for loadable kernel modules that use the new
smp_cond_load_relaxed_timeout() or smp_cond_load_acquire_timeout() macros?

This macro injects a direct call to arch_timer_evtstrm_available() into the
caller's context. However, arch_timer_evtstrm_available() is defined in
drivers/clocksource/arm_arch_timer.c and lacks an EXPORT_SYMBOL_GPL directive.

Without the export, standard loadable modules using the generic barrier
timeout API will fail to build with undefined symbol errors.

[Severity: Medium]
Can this introduce a noinstr violation in the idle entry path?

arch_timer_evtstrm_available() is an out-of-line, instrumentable function
since it is not marked noinstr.

When smp_cond_load_relaxed_timeout() is used in poll_idle()
(in drivers/cpuidle/poll_state.c), it is executed in a noinstr context
because poll_idle() is marked __cpuidle, which places it in the
.cpuidle.text section. Objtool strictly validates .cpuidle.text as noinstr.

Calling an instrumentable function directly from the idle spin-loop macro
risks tracing recursion and objtool validation errors.

> +		__cmpwait_relaxed(ptr, val);				\
> +	else								\
> +		cpu_relax();						\
> +} while (0)
> +
>  #include <asm-generic/barrier.h>
>  
>  #endif	/* __ASSEMBLER__ */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016070837.440748-1-ankur.a.arora@oracle.com?part=2