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

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

bot+bpf-ci@kernel.org writes:

>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index fe60738e5943..fa676428ec3f 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -1606,6 +1606,9 @@ config ARCH_SUPPORTS_CRASH_DUMP
>>  config ARCH_DEFAULT_CRASH_DUMP
>>  	def_bool y
>>
>> +config ARCH_HAS_CPU_RELAX
>> +	def_bool y
>> +
>
> ARCH_HAS_CPU_RELAX isn't private to the barrier code.  Before this it was
> defined only by arch/x86, and the generic kernel already keys cpuidle
> behavior off it in two places that arm64 builds.
>
> In drivers/cpuidle/Makefile:
>
>   obj-$(CONFIG_ARCH_HAS_CPU_RELAX)	  += poll_state.o
>
> and in drivers/acpi/processor_idle.c:
>
>   #define ACPI_IDLE_STATE_START	(IS_ENABLED(CONFIG_ARCH_HAS_CPU_RELAX) ? 1 : 0)
>   ...
>   if (IS_ENABLED(CONFIG_ARCH_HAS_CPU_RELAX)) {
>           cpuidle_poll_state_init(drv);
>           count = 1;
>   }
>
> On arm64, ACPI_PROCESSOR is "default y" and depends on ARM64, and it
> selects ACPI_PROCESSOR_IDLE which selects CPU_IDLE.  So on an ACPI arm64
> system this now builds poll_state.o and makes acpi_processor_setup_cstates()
> register a POLL idle state as C-state index 0, shifting the firmware
> C-states up by one.

Yes, this is a bug. The additional POLL idle state change wasn't intended
to be registered as part of this patch.

With this change the first c-state will end up executing a NOP poll_idle()
since current_set_polling_and_test() is tif_need_resched() when
TIF_POLLING_NRFLAG is not defined but this does unnecessarily register
the additional c-state.

I missed that in my testing because my test machine has LPI states which
need an explicit change to enable this.
(https://lore.kernel.org/lkml/20250218213337.377987-7-ankur.a.arora@oracle.com/)

Ankur


> The commit message describes this symbol only as indicating "an optimized
> implementation of cpu_poll_relax()", and the generic patch describes it as
> "a cpu_poll_relax() implementation that is expected to be cheaper".  Neither
> mentions the cpuidle/ACPI poll_state side effect.
>
>
> Is enabling the cpuidle polling idle state on arm64 intended here, or is
> ARCH_HAS_CPU_RELAX only meant to feed smp_cond_load_relaxed_timeout()?  If
> the latter, would a dedicated symbol (or decoupling poll_state/ACPI from
> ARCH_HAS_CPU_RELAX) avoid the unintended idle-path change?

>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/27125050324


--
ankur