Re: [RFC PATCH v3 2/2] cpuidle: teo: Introduce util-awareness

[Kajetan Puchalski <kajetan.puchalski@arm.com>]

On Fri, Nov 25, 2022 at 07:27:13PM +0100, Rafael J. Wysocki wrote:
> > +/*
> > + * The number of bits to shift the cpu's capacity by in order to determine
> > + * the utilized threshold
> > + */
> > +#define UTIL_THRESHOLD_SHIFT 6
>
> Why is this particular number regarded as the best one?

Based on my testing this number achieved the best balance of power and
performance on average. It also makes sense from looking at the util
plots. The resulting threshold is high enough to not be triggered by
background noise and low enough to react quickly when activity starts
ramping up.

> > +static void teo_get_util(struct cpuidle_device *dev, struct teo_cpu *cpu_data)
> > +{
> > +       unsigned long util = sched_cpu_util(dev->cpu);
> > +
> > +       cpu_data->utilized = util > cpu_data->util_threshold;
>
> Why exactly do you need the local variable here?

It's not necessarily needed, I can replace it with comparing the result
of the call directly.

> Then, if there's only one caller, maybe this could be folded into it?

I do think it's nicer to have it separated in its own helper function, that
way if anything more has to be done with the util it'll all be
self-contained. Having only one caller shouldn't be a big issue, it's
also the case for teo_middle_of_bin and teo_find_shallower_state in the
current TEO implementation.

> > +               /* don't update metrics if the cpu was utilized during the last sleep */
> 
> Why?
> 
> The metrics are related to idle duration and cpu_data->utilized
> indicates whether or not latency should be reduced.  These are
> different things.
> 
> Moreover, this is just one data point and there may not be any direct
> connection between it and the decision made in this particular cycle.

Agreed, v4 already has this part removed.

> > +               if (!cpu_data->utilized)
> > +                       teo_update(drv, dev);
> >                 dev->last_state_idx = -1;
> >         }
> >
> > @@ -323,6 +381,21 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> >                         goto end;
> >         }
> >
> > +       teo_get_util(dev, cpu_data);
> > +       /* the cpu is being utilized and there's only 2 states to choose from */
> > +       /* no need to consider metrics, choose the shallowest non-polling state and exit */
> 
> A proper kernel-coding-style 2-line comment, please!
> 
> Also I would say "utilized beyond the threshold" instead of just
> "utilized" and "there are only 2 states" (plural).

Both good points, I'll fix that.

> > +       if (drv->state_count < 3 && cpu_data->utilized) {
> > +               for (i = 0; i < drv->state_count; ++i) {
> > +                       if (dev->states_usage[i].disable ||
> > +                                       drv->states[i].flags & CPUIDLE_FLAG_POLLING)
> > +                               continue;
> > +                       break;
> 
> This looks odd.  It would be more straightforward to do
> 
> if (!dev->states_usage[i].disable && !(drv->states[i].flags &
> CPUIDLE_FLAG_POLLING)) {
>         idx = i;
>         goto end;
> }
> 
> without the "break" and "continue".

Fair enough, this works as well.

> I've changed my mind with respect to adding the idx == 1 check to
> this.  If the goal is to reduce latency for the "loaded" CPUs, this
> applies to deeper idle states too.

I see, this has no effect on arm devices one way or the other so I don't
mind, it's completely up to you. In light of Doug's test results
regarding this change, should I remove the check in v5?

Thanks,
Kajetan