Re: [PATCH v12 12/15] cpuidle/poll_state: Wait for need-resched via tif_need_resched_relaxed_wait()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] smp_cond_load_relaxed_timeout() doubles the timeout duration on architectures where cpu_poll_relax() suspends execution (e.g., arm64 using WFET).
--

commit 9f74e9110ff72d86c2f74843434b01c10db16a0c
Author: Ankur Arora <ankur.a.arora@oracle.com>

cpuidle/poll_state: Wait for need-resched via tif_need_resched_relaxed_wait()

The inner loop in poll_idle() currently polls over the thread_info flags using
cpu_relax(), waiting for TIF_NEED_RESCHED. This commit switches to using
tif_need_resched_relaxed_wait() instead. This aims to minimize power
consumption by utilizing optimized platform-specific waiting mechanisms.

> diff --git a/drivers/cpuidle/poll_state.c b/drivers/cpuidle/poll_state.c
> index c7524e4c522a2..7443b3e971baf 100644
> --- a/drivers/cpuidle/poll_state.c
> +++ b/drivers/cpuidle/poll_state.c
[ ... ]
> @@ -6,41 +6,22 @@
>  static int __cpuidle poll_idle(struct cpuidle_device *dev,
>  			       struct cpuidle_driver *drv, int index)
>  {
[ ... ]
>  	raw_local_irq_enable();
>  	if (!current_set_polling_and_test()) {
> -		unsigned int loop_count = 0;
>  		u64 limit;
>  
>  		limit = cpuidle_poll_time(drv, dev);
>  
> -		while (!need_resched()) {
> -			cpu_relax();
> -			if (loop_count++ < POLL_IDLE_RELAX_COUNT)
> -				continue;
> -
> -			loop_count = 0;
> -			if (local_clock_noinstr() - time_start > limit) {
> -				dev->poll_time_limit = true;
> -				break;
> -			}
> -		}
> +		dev->poll_time_limit = !tif_need_resched_relaxed_wait(limit);

[Severity: High]
Does this double the timeout duration on architectures where cpu_poll_relax()
suspends execution (such as arm64 using WFET)?

Looking at the generic macro smp_cond_load_relaxed_timeout() called by
tif_need_resched_relaxed_wait(), the absolute deadline __time_end is lazily
initialized after the first batch of polling iterations:

include/asm-generic/barrier.h:
    ...
		cpu_poll_relax(__PTR, VAL, (u64)__timeout);		\
		if (++__count < __spin)					\
			continue;					\
		__time_now = (s64)(time_expr_ns);			\
		if (unlikely(__time_end == 0))				\
			__time_end = __time_now + __timeout;		\
    ...

On arm64, SMP_TIMEOUT_POLL_COUNT is 1, and cpu_poll_relax() maps to
__cmpwait_relaxed_timeout(), which uses the WFET instruction to sleep for the
requested timeout:

arch/arm64/include/asm/barrier.h:
#define SMP_TIMEOUT_POLL_COUNT	1

#define cpu_poll_relax(ptr, val, timeout_ns) do {			\
	if (alternative_has_cap_unlikely(ARM64_HAS_WFXT))		\
		__cmpwait_relaxed_timeout(ptr, val, timeout_ns);	\
    ...
}

Because __time_end is calculated as __time_now + __timeout after waking up
from this first sleep, wouldn't the deadline be erroneously set to
start_time + 2 * timeout_ns?

If so, the loop would continue, calling cpu_poll_relax() again with a
remaining timeout of timeout_ns, causing a second sleep and breaking the
timeout contract.

>  	}
>  	raw_local_irq_disable();
>

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