Re: [PATCH v14 02/15] 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:
> - [High] The generic macro `smp_cond_load_relaxed_timeout()` is broken for loadable modules on ARM64 because it uses the unexported symbol `arch_timer_evtstrm_available`.

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

> --
>
> --- Patch [2]: [PATCH v14 02/15] arm64: barrier: Support smp_cond_load_relaxed_timeout() ---
> (Note: The format of this review has been altered to provide a free-form summary rather than inline quotes due to recitation restrictions.)
>
> commit 8ff6ac5954b14995e35ca4865c27742e37086e23
> 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
> using __cmpwait_relaxed() when the event-stream is available. It introduces
> cpu_poll_relax() which checks arch_timer_evtstrm_available() before deciding
> to wait or fallback to cpu_relax().
>
> [Severity: High]
> Will loadable modules be able to build if they use the new timeout barrier API?
>
> The new cpu_poll_relax() macro in arch/arm64/include/asm/barrier.h calls
> arch_timer_evtstrm_available(), which is defined in
> drivers/clocksource/arm_arch_timer.c.
>
> Since cpu_poll_relax() is embedded inline into callers of
> smp_cond_load_relaxed_timeout(), any loadable module that uses this generic
> API will now require the arch_timer_evtstrm_available symbol.
>
> However, it does not appear that arch_timer_evtstrm_available is exported with
> EXPORT_SYMBOL or EXPORT_SYMBOL_GPL.
>
> Does this cause an undefined reference error during the modpost step for
> device drivers that attempt to use smp_cond_load_relaxed_timeout()?


--
ankur