Re: [PATCH RFC] cpuidle/drivers/menu: Remove get_loadavg in the performance multiplier

[Rafael J. Wysocki]

On Wed, Oct 3, 2018 at 12:25 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
>
> On 03/10/2018 10:58, Rafael J. Wysocki wrote:
> > On Tuesday, September 25, 2018 11:38:44 AM CEST Daniel Lezcano wrote:
> >> The function get_loadavg() returns almost always zero. To be more
> >> precise, statistically speaking for a total of 1023379 times passing
> >> to the function, the load is equal to zero 1020728 times, greater than
> >> 100, 610 times, the remaining is between 0 and 5.
> >>
> >> I'm putting in question this metric. Is it worth to keep it?
> >
> > The honest answer is that I'm not sure.
> >
> > Using the CPU load for driving idle state selection is questionable in general
> > (there need not be any connection between the two in principle) and this
> > particular function is questionable either.  If it really makes no difference
> > in addition to that, then yes it would be good to get rid of it.
> >
> > That said it looks like that may be quite workload-dependent, so maybe there
> > are workloads where it does make a difference?  Like networking or similar?
>
> Perhaps initially the load was not the same:
>
>
> commit 69d25870f20c4b2563304f2b79c5300dd60a067e
> Author: Arjan van de Ven <arjan@xxxxxxxxxxxxx>
> Date:   Mon Sep 21 17:04:08 2009 -0700
>
>     cpuidle: fix the menu governor to boost IO performance
>
>
> The function get_loadavg() was:
>
> static int get_loadavg(void)
> {
>        unsigned long this = this_cpu_load();
>
>        return LOAD_INT(this) * 10 + LOAD_FRAC(this) / 10;
> }
>
> With:
>
> unsigned long this_cpu_load(void)
> {
>         struct rq *this = this_rq();
>         return this->cpu_load[0];
> }
>
>
> Now we pass the load we got from get_iowait_load() which is:
>
> void get_iowait_load(unsigned long *nr_waiters, unsigned long *load)
> {
>         struct rq *rq = this_rq();
>         *nr_waiters = atomic_read(&rq->nr_iowait);
>         *load = rq->load.weight;
> }

This would mean that commit 372ba8cb46b2 (cpuidle: menu: Lookup CPU
runqueues less) changed the behavior more than expected, though (CC
Mel).

In any case that change was made in 2014 which is after Android
stopped using get_loadavg().

> Colin Cross did some measurements a few years ago and concluded it is
> zero most of the time, otherwise the number is so high it has no
> meaning. The get_loadavg() call is removed from Android since 2011 [1].

This is a good indication that it doesn't really matter (any more perhaps).

> I can hack my server (bi Xeon - 6 cores), add statistics regarding this
> metric and let it run during several days, sure a lot of different
> workloads will happen on it (compilation, network, throughput computing,
> ...) in a real use case manner. That is what I did with an ARM64
> evaluation board.
>
> However I won't be able to do that right now and that will take a bit of
> time. If you think it is really useful, I can plan to do that in the
> next weeks.

Well, we may as well just make this change to follow Android and let
it go through all of the CI we have for one cycle.

> [1] https://android.googlesource.com/kernel/common/+/4dedd9f124703207895777ac6e91dacde0f7cc17

If you send a non-RFC patch to drop get_loadavg() (but you can drop it
altogether then, there are no other callers of it AFAICS), I'll queue
it up after the 4.20 (or whatever it turns out to be in the end) merge
window so all of the CIs out there have a chance to report issues (if
any).

Thanks,
Rafael