Re: [PATCH v15 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 1 potential issue(s) to consider:
> - [Low] Module linkage failure due to unexported symbol `arch_timer_evtstrm_available`.
> --
>
> commit 6f96330ea583f1ff6aede67a83cdc552196dad11
> Author: Ankur Arora <ankur.a.arora@oracle.com>
>
> arm64: barrier: Support smp_cond_load_relaxed_timeout()
>
> This commit introduces an optimized cpu_poll_relax() for ARM64 that uses
> __cmpwait_relaxed() to wait for a value change. It gates this logic on
> arch_timer_evtstrm_available() being true to avoid blocking forever without
> periodic wakeups from the event stream.
>
>> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
>> --- a/arch/arm64/include/asm/barrier.h
>> +++ b/arch/arm64/include/asm/barrier.h
>
> [ ... ]
>
>> +/* 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 which enables a time-check
>> + * optimization in smp_cond_load_{relaxed,acquire}_timeout().
>> + */
>> +#define CPU_POLL_RELAX_WAITS
>> +
>> +#define cpu_poll_relax(ptr, val, timeout_ns) do {			\
>> +	if (arch_timer_evtstrm_available())				\
>> +		__cmpwait_relaxed(ptr, val);				\
>> +	else								\
>> +		cpu_relax();						\
>> +} while (0)
>
> [Severity: Low]
> Will this cause a module linkage failure if a loadable kernel module
> uses smp_cond_load_relaxed_timeout() or similar APIs?
>
> Since cpu_poll_relax() is a macro, it expands directly in the caller's
> code. However, looking at drivers/clocksource/arm_arch_timer.c, it appears
> that arch_timer_evtstrm_available() is not exported to loadable modules:
>
> drivers/clocksource/arm_arch_timer.c:arch_timer_evtstrm_available() {
>         /*
>          * We might get called from a preemptible context.
>          ...
> }
>
> Without an EXPORT_SYMBOL declaration, any loadable module utilizing this
> polling macro will fail to link with an unknown symbol error. Should
> arch_timer_evtstrm_available be exported to support loadable modules?

As mentioned in https://lore.kernel.org/all/87qzkk2hxz.fsf@oracle.com/:

This interface mostly exists (or wants to) to unify spin-wait with
timeout where the waiting can be done via a spinning cpu_relax() or
arm64 style waiting on a cacheline (WFE/WFET).

Currently, almost all of the users of smp_cond_load_{acquire,relaxed}
are in kernel/

$ git grep -E 'smp_cond_load_acquire|smp_cond_load_relaxed' kernel/ | wc
27     145    2191

The only non kernel/ users are in:
$ git grep -E 'smp_cond_load_acquire|smp_cond_load_relaxed' arch/ drivers/ mm/ fs/
arch/arm64/include/asm/mte.h:   smp_cond_load_acquire(&page->flags.f, VAL & (1UL << PG_mte_tagged));
arch/arm64/include/asm/mte.h:   smp_cond_load_acquire(&folio->flags.f, VAL & (1UL << PG_mte_tagged));
arch/arm64/mm/mmu.c:            smp_cond_load_acquire(&idmap_kpti_bbml2_flag, VAL == num_online_cpus());
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:    smp_cond_load_relaxed(cmd, !VAL || (ret = queue_poll(&qp)));

So I don't expect this to be needed in kernel modules any time soon.
And if it is, we can export the symbol then.

--
ankur