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

[Ankur Arora <ankur.a.arora@oracle.com>]

sashiko-bot@kernel.org writes:

> 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

And as the Kconfig doc for CONFIG_ARM_ARCH_TIMER_EVTSTREAM says:
  "This must be disabled for hardware validation purposes to detect any
   hardware anomalies of missing events."

> (clocksource.arm_arch_timer.evtstrm=0),

Same thing for this where the kernel-parameters.txt says "so that code
using WFE-based polling loops can be debugged more effectively on
production systems"

> or under hypervisors that lack virtual event stream support.

Common hypervisors KVM/QEMU, Xen support this. Even beyond this
cpu_relax() on arm64 doesn't do much (is a yield) so increasing the
SMP_TIMEOUT_POLL_COUNT doesn't do much.

The correct solution for a case like this would be to detect this
case (or have a new kernel parameter) that disables polling at init.

That's out of scope for this series.

>> +
>> +#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.

Yes. Can be fixed when there are users.

> [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.

Oddly enough I didn't see any objtool errors.

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

Will check.

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


--
ankur