Re: [PATCH] sched/cpufreq: Reevaluate frequency before tickless idle

On 8/25/26 13:52, Vincent Guittot wrote:
> On Tue, 25 Aug 2026 at 11:00, Christian Loehle <[email protected]> wrote:
>>
>> On 8/25/26 03:51, Hongyan Xia wrote:
>>> On 8/24/2026 9:18 PM, Christian Loehle wrote:
>>>> sugov_hold_freq() can preserve a UCLAMP_MIN-driven high frequency when
>>>> the runqueue goes idle. If cpuidle then stops the tick, no later
>>>> utilization update is guaranteed and a CPU using WFI can remain at an
>>>> unnecessarily high voltage for the entire idle period.
>>>>
>>>> Issue a final cpufreq update when the idle tick actually transitions to
>>>> stopped and force single-policy schedutil past its rate limit. Keep the
>>>> existing hold behavior when the tick is retained.
>>>>
>>>> Signed-off-by: Christian Loehle <[email protected]>
>>>> ---
>>>>   include/linux/sched/cpufreq.h    |  1 +
>>>>   kernel/sched/cpufreq_schedutil.c |  5 ++++-
>>>>   kernel/sched/idle.c              | 29 +++++++++++++++++++++++++++--
>>>>   3 files changed, 32 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
>>>> index bdd31ab93bc5..0814f6c79315 100644
>>>> --- a/include/linux/sched/cpufreq.h
>>>> +++ b/include/linux/sched/cpufreq.h
>>>> @@ -9,6 +9,7 @@
>>>>    */
>>>>
>>>>   #define SCHED_CPUFREQ_IOWAIT       (1U << 0)
>>>> +#define SCHED_CPUFREQ_IDLE  (1U << 1)
>>>>
>>>>   #ifdef CONFIG_CPU_FREQ
>>>>   struct cpufreq_policy;
>>>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>>>> index a1782755efcc..90bf8d8bffb7 100644
>>>> --- a/kernel/sched/cpufreq_schedutil.c
>>>> +++ b/kernel/sched/cpufreq_schedutil.c
>>>> @@ -100,7 +100,7 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
>>>>
>>>>              return true;
>>>>      } else if (sg_policy->need_freq_update) {
>>>> -            /* ignore_dl_rate_limit() wants a new frequency to be found. */
>>>> +            /* A forced update needs a new frequency to be found. */
>>>>              return true;
>>>>      }
>>>>
>>>> @@ -407,6 +407,9 @@ static inline bool sugov_update_single_common(struct sugov_cpu *sg_cpu,
>>>>      sugov_iowait_boost(sg_cpu, time, flags);
>>>>      sg_cpu->last_update = time;
>>>>
>>>> +    if (flags & SCHED_CPUFREQ_IDLE)
>>>> +            sg_cpu->sg_policy->need_freq_update = true;
>>>> +
>>>>      ignore_dl_rate_limit(sg_cpu);
>>>>
>>>>      if (!sugov_should_update_freq(sg_cpu->sg_policy, time))
>>>> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
>>>> index eb73b65ce6c4..36f8840d0562 100644
>>>> --- a/kernel/sched/idle.c
>>>> +++ b/kernel/sched/idle.c
>>>> @@ -161,10 +161,35 @@ static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>>>      return cpuidle_enter(drv, dev, next_state);
>>>>   }
>>>>
>>>> +static void idle_stop_tick(void)
>>>> +{
>>>> +#ifdef CONFIG_CPU_FREQ
>>>> +    bool was_stopped = tick_nohz_tick_stopped();
>>>> +#endif
>>>> +
>>>> +    tick_nohz_idle_stop_tick();
>>>> +
>>>> +#ifdef CONFIG_CPU_FREQ
>>>> +    /*
>>>> +     * Run one last cpufreq update before entering idle with the tick
>>>> +     * stopped, because no later update is guaranteed.
>>>> +     */
>>>> +    if (!was_stopped && tick_nohz_tick_stopped()) {
>>>> +            struct rq *rq = this_rq();
>>>> +            struct rq_flags rf;
>>>> +
>>>> +            rq_lock(rq, &rf);
>>>> +            update_rq_clock(rq);
>>>> +            cpufreq_update_util(rq, SCHED_CPUFREQ_IDLE);
>>>> +            rq_unlock(rq, &rf);
>>>> +    }
>>>> +#endif
>>>> +}
>>>> +
>>>>   static void idle_call_stop_or_retain_tick(bool stop_tick)
>>>>   {
>>>>      if (stop_tick || tick_nohz_tick_stopped())
>>>> -            tick_nohz_idle_stop_tick();
>>>> +            idle_stop_tick();
>>>>      else
>>>>              tick_nohz_idle_retain_tick();
>>>>   }
>>>> @@ -225,7 +250,7 @@ static void cpuidle_idle_call(bool stop_tick)
>>>>                      max_latency_ns = dev->forced_idle_latency_limit_ns;
>>>>              }
>>>>
>>>> -            tick_nohz_idle_stop_tick();
>>>> +            idle_stop_tick();
>>>>
>>>>              next_state = cpuidle_find_deepest_state(drv, dev, max_latency_ns);
>>>>              call_cpuidle(drv, dev, next_state);
>>>
>>> LGTM, minus the Sashiko issue.
>>>
>>> One side note is that I briefly looked at similar things before, not
>>> just for sugov_hold_freq() but for going idle in general. It might be
>>> desirable to predict the util *after* wake-up and use it to drive
>>> frequency one last time before going idle for any CPU. This avoids
>>> holding high frequencies in update_single() and also avoids idling CPUs
>>> holding high frequencies for the entire cluster in update_shared().
>>>
>>> But predictions are hard and the energy savings aren't much so I didn't
>>> spend much time on it. Not sure if there are better ideas.
>>
>> Funnily enough the slow-switch case makes this a lot more complicated,
>> in particular with PREEMPT_RT :/
>> My current approach is to have an irq work for the next tick for
>> the sugov_hold_freq() case, that checks if it hasn't seen another update
>> for the tick duration and then drops the hold_freq.
>> It's not ideal because of course it disabled the tick-stop by one tick,
>> but it's the best approach I can come up with now.
>> We could do something similar for decayed blocked utilization (i.e. have
>> an irq work for when utilization would reach lowest OPP (or an intermediate
>> step?) and decay that then).
>>
>> That being said I do think the hold_freq and the blocked utilization are
>> separate issues, they just might have a similar-looking solution...
>> I can post an RFC for the latter, that being said I'm not entirely convinced
>> myself that it's worth it, while I think the hold_freq is just clearly wrong
>> behavior.
> 
> IIRC some past discussions, keeping the frequency when going idle was
> a desired behavior because switching back to a high frequency took
> time and slow down the wakeup (in particular with the cpufreq slow
> path), and the power cost was not huge because long idle times would
> power off the power domain, and idle time in WFI would be corrected by
> the next tick. Do I understand correctly that in your case you only
> have WFI, so you never power down domain for long sleep ?
> 
> I'm not sure I get why you want to decrease frequency in WFI instead
> of letting the tick update the utilization. Or Am I missing something
> in your use case ?

I'm specifically worried about WFI + stopped tick, I think the rest
is acceptable.