Re: [PATCH v1 4/4] cpuidle: governors: teo: Decay metrics below DECAY_SHIFT threshold

[Christian Loehle <christian.loehle@arm.com>]

On 11/12/25 17:51, Rafael J. Wysocki wrote:
> On Wed, Nov 12, 2025 at 6:29 PM Christian Loehle
> <christian.loehle@arm.com> wrote:
>>
>> On 11/12/25 16:25, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> If a given governor metric falls below a certain value (8 for
>>> DECAY_SHIFT equal to 3), it will not decay any more due to the
>>> simplistic decay implementation.  This may in some cases lead to
>>> subtle inconsistencies in the governor behavior, so change the
>>> decay implementation to take it into account and set the metric
>>> at hand to 0 in that case.
>>>
>>> Suggested-by: Christian Loehle <christian.loehle@arm.com>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>  drivers/cpuidle/governors/teo.c |   20 +++++++++++++++-----
>>>  1 file changed, 15 insertions(+), 5 deletions(-)
>>>
>>> --- a/drivers/cpuidle/governors/teo.c
>>> +++ b/drivers/cpuidle/governors/teo.c
>>> @@ -148,6 +148,16 @@ struct teo_cpu {
>>>
>>>  static DEFINE_PER_CPU(struct teo_cpu, teo_cpus);
>>>
>>> +static void teo_decay(unsigned int *metric)
>>> +{
>>> +     unsigned int delta = *metric >> DECAY_SHIFT;
>>> +
>>> +     if (delta)
>>> +             *metric -= delta;
>>> +     else
>>> +             *metric = 0;
>>> +}
>>> +
>>>  /**
>>>   * teo_update - Update CPU metrics after wakeup.
>>>   * @drv: cpuidle driver containing state data.
>>> @@ -159,7 +169,7 @@ static void teo_update(struct cpuidle_dr
>>>       int i, idx_timer = 0, idx_duration = 0;
>>>       s64 target_residency_ns, measured_ns;
>>>
>>> -     cpu_data->short_idles -= cpu_data->short_idles >> DECAY_SHIFT;
>>> +     teo_decay(&cpu_data->short_idles);
>>>
>>>       if (cpu_data->artificial_wakeup) {
>>>               /*
>>> @@ -195,8 +205,8 @@ static void teo_update(struct cpuidle_dr
>>>       for (i = 0; i < drv->state_count; i++) {
>>>               struct teo_bin *bin = &cpu_data->state_bins[i];
>>>
>>> -             bin->hits -= bin->hits >> DECAY_SHIFT;
>>> -             bin->intercepts -= bin->intercepts >> DECAY_SHIFT;
>>> +             teo_decay(&bin->hits);
>>> +             teo_decay(&bin->intercepts);
>>>
>>>               target_residency_ns = drv->states[i].target_residency_ns;
>>>
>>> @@ -207,7 +217,7 @@ static void teo_update(struct cpuidle_dr
>>>               }
>>>       }
>>>
>>> -     cpu_data->tick_intercepts -= cpu_data->tick_intercepts >> DECAY_SHIFT;
>>> +     teo_decay(&cpu_data->tick_intercepts);
>>>       /*
>>>        * If the measured idle duration falls into the same bin as the sleep
>>>        * length, this is a "hit", so update the "hits" metric for that bin.
>>> @@ -222,7 +232,7 @@ static void teo_update(struct cpuidle_dr
>>>                       cpu_data->tick_intercepts += PULSE;
>>>       }
>>>
>>> -     cpu_data->total -= cpu_data->total >> DECAY_SHIFT;
>>> +     teo_decay(&cpu_data->total);
>>>       cpu_data->total += PULSE;
>>
>> This will result in total no longer being a strict sum of the bins.
> 
> Ah, good point.
> 
>> Any reason not to do something like:
> 
> Well, it would be more straightforward to just compute "total" from
> scratch instead of using total_decay (it would be the same amount of
> computation minus the teo_decay() changes AFAICS).

Duh, of course...

> 
> I'll send an update of this patch.

Thanks!