Re: [PATCH v13 02/15] arm64: barrier: Support smp_cond_load_relaxed_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The macro `cpu_poll_relax()` references `arch_timer_evtstrm_available()`, which is not exported to loadable modules, causing module build failures.
- [Medium] Hardcoding `SMP_TIMEOUT_POLL_COUNT` to 1 severely degrades spin-wait performance in the fallback path when the event stream is disabled.
--

commit 906c27d3b9d6d75543755daf442e8912be30a6da
Author: Ankur Arora <ankur.a.arora@oracle.com>

arm64: barrier: Support smp_cond_load_relaxed_timeout()

This commit adds support for smp_cond_load_relaxed_timeout() on arm64 by
utilizing __cmpwait_relaxed(). It restricts this path to when the event-stream
is enabled to prevent blocking indefinitely, falling back to cpu_relax()
otherwise. It also sets SMP_TIMEOUT_POLL_COUNT to 1.

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

[ ... ]

> @@ -219,6 +220,26 @@ 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 the ptr value to change.
> + *
> + * Since this period is reasonably long, choose SMP_TIMEOUT_POLL_COUNT
> + * to be 1, so smp_cond_load_{relaxed,acquire}_timeout() does a
> + * time-check in each iteration.
> + */
> +#define SMP_TIMEOUT_POLL_COUNT	1

[Severity: Medium]
Does setting SMP_TIMEOUT_POLL_COUNT to 1 cause severe performance degradation
when the event stream is disabled?

If arch_timer_evtstrm_available() returns false, cpu_poll_relax() falls back
to cpu_relax(), which spins very fast. With SMP_TIMEOUT_POLL_COUNT set to 1,
the generic loop in smp_cond_load_relaxed_timeout() bypasses any batching and
evaluates the time expression on every single iteration.

If the time expression accesses a clock like ktime_get_mono_fast_ns(), could
spinning CPUs repeatedly read the timekeeper seqcount and hardware clock
without backoff, causing cache-line contention and pipeline saturation?

> +
> +#define cpu_poll_relax(ptr, val, timeout_ns) do {			\
> +	if (arch_timer_evtstrm_available())				\
> +		__cmpwait_relaxed(ptr, val);				\
> +	else								\
> +		cpu_relax();						\
> +} while (0)

[Severity: High]
Will this cause module build failures when loadable modules use
smp_cond_load_relaxed_timeout() or its atomic wrappers?

Because cpu_poll_relax() is an inline macro, it forces any caller, including
modules, to link against arch_timer_evtstrm_available():

drivers/clocksource/arm_arch_timer.c:arch_timer_evtstrm_available() {
    ...
    return cpumask_test_cpu(raw_smp_processor_id(), &evtstrm_available);
}

Since arch_timer_evtstrm_available() lacks an EXPORT_SYMBOL() or
EXPORT_SYMBOL_GPL(), will standard modules fail to compile at the modpost
stage with an undefined symbol error?

> +
>  #include <asm-generic/barrier.h>
>  
>  #endif	/* __ASSEMBLER__ */

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