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

[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:
> - [Medium] The timeout for the idle polling loop is erroneously extended because the generic wait macro calculates the timeout start time only after the first 200 spin iterations. Additionally, the commit relies on an architecture-specific optimization that does not actually exist on the target platform.
> --
>
> commit 9b952e08004e7de674a695400c7ab21ff69df4db
> Author: Ankur Arora <ankur.a.arora@oracle.com>
>
> cpuidle/poll_state: Wait for need-resched via tif_need_resched_relaxed_wait()
>
> This commit replaces the manual polling loop in poll_idle() with a call to
> tif_need_resched_relaxed_wait() to optimize idle power consumption.
>
>> 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
> [ ... ]
>> @@ -21,15 +20,7 @@ static int __cpuidle poll_idle(struct cpuidle_device *dev,
>>  		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: Medium]
> Does this unintentionally extend the polling timeout?
>
> When tif_need_resched_relaxed_wait() uses the generic
> __smp_cond_load_relaxed_timeout() macro, it first spins for 200 iterations
> before capturing the start time for the timeout:
>
> include/asm-generic/barrier.h:__smp_cond_load_relaxed_timeout() {
>     ...
>     cpu_poll_relax(__PTR, VAL, (u64)__scl_timeout); \
>     if (++__scl_count < __scl_spin)                 \
>             continue;                               \
>     __scl_time_now = (s64)(time_expr_ns);           \
>     if (unlikely(__scl_time_end == 0)) {            \
>             __scl_timeout = (s64)(timeout_ns);      \
>             __scl_time_end = __scl_time_now + __scl_timeout; \
>     ...
> }
>
> Since the timeout calculation is deferred until after the initial spin loop,
> the total wait time becomes the requested limit plus the duration of those
> first 200 iterations.

Yes, not meant to be a precise timeout. As mentioned in the commit
message of patch-1:

  This also means that there will always be some hardware dependent
  duration that has passed in cpu_poll_relax() iterations at the time
  of first evaluation. Additionally cpu_poll_relax() is not guaranteed
  to return at timeout boundary. In sum, expect timeout overshoot when
  we exit due to expiration of the timeout.

> On platforms where 200 cpu_relax() iterations can take a significant amount
> of time (e.g. ~14 microseconds on modern x86), could this effectively delay
> entry into deeper C-states and cause a power regression for short polling
> intervals?

"Power regression for short polling intervals": would be curious to see
this workload.

Rafael confirmed in [1] that a delay like this (or potentially worse
with WFE on arm64) is okay:

   > Just that with waited mode on arm64 the timeout might be delayed depending
   > on granularity of the event stream.

   That's fine.  cpuidle_poll_time() is not exact anyway.

[1] https://lore.kernel.org/lkml/CAJZ5v0izSBR0_DeH5HVnSLFGRfV9WoSzbu9Mh5yvvuyrvw7fLg@mail.gmail.com/

--
ankur