linux-kernel - Re: [PATCH v3] sched/fair: unlink misfit task from cpu overutilized

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7a6182dd-89f5-69c5-4331-2f102dc0418d@arm.com>
Date:   Mon, 16 Jan 2023 14:56:19 +0000
From:   Dietmar Eggemann <dietmar.eggemann@....com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     mingo@...nel.org, peterz@...radead.org, qyousef@...alina.io,
        rafael@...nel.org, viresh.kumar@...aro.org, vschneid@...hat.com,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        lukasz.luba@....com, wvw@...gle.com, xuewen.yan94@...il.com,
        han.lin@...iatek.com, Jonathan.JMChen@...iatek.com
Subject: Re: [PATCH v3] sched/fair: unlink misfit task from cpu overutilized

On 16/01/2023 12:23, Vincent Guittot wrote:
> On Mon, 16 Jan 2023 at 10:07, Dietmar Eggemann <dietmar.eggemann@....com> wrote:
>>
>> On 13/01/2023 14:40, Vincent Guittot wrote:

[...]

>>> @@ -6132,6 +6135,7 @@ static inline bool cpu_overutilized(int cpu)
>>>       unsigned long rq_util_min = uclamp_rq_get(cpu_rq(cpu), UCLAMP_MIN);
>>>       unsigned long rq_util_max = uclamp_rq_get(cpu_rq(cpu), UCLAMP_MAX);
>>>
>>> +     /* Return true only if the utlization doesn't fit its capacity */
>>
>> s/utlization/utilization
>> s/its/CPU ?
>>
>>>       return !util_fits_cpu(cpu_util_cfs(cpu), rq_util_min, rq_util_max, cpu);
>>>  }
>>
>> cpu_overutilized() is the only place where we now only test for
>> !util_fits_cpu(). The new comment says we only care about utilization
>> not fitting CPU capacity.
>>
>> Does this mean the rq uclamp values are not important here and we could
>> go back to use fits_capacity()?
>>
>> Not sure since util_fits_cpu() is still coded differently:
> 
> uclamp_min is not important but uclamp_max still cap the utilization

OK, makes sense.

I.e. we could pass in `rq_util_min = 0` to avoid fetching it
unnecessary? In case `fits == 1` before the uclamp_min condition in
util_fits_cpu() it doesn't matter if we switch to return `-1` when
called from cpu_overutilized(). Detail though ...

[...]

>>> @@ -6940,12 +6945,28 @@ select_idle_capacity(struct task_struct *p, struct sched_domain *sd, int target)
>>>
>>>               if (!available_idle_cpu(cpu) && !sched_idle_cpu(cpu))
>>>                       continue;
>>> -             if (util_fits_cpu(task_util, util_min, util_max, cpu))
>>> +
>>> +             fits = util_fits_cpu(task_util, util_min, util_max, cpu);
>>> +
>>> +             /* This CPU fits with all capacity and performance requirements */
>>
>> In task_fits_cpu() `utilization and performance (better uclamp)
>> requirements` term was used. I assume it's the same thing here?
>>
>>> +             if (fits > 0)
>>>                       return cpu;
>>> +             /*
>>> +              * Only the min performance (i.e. uclamp_min) doesn't fit. Look
>>> +              * for the CPU with highest performance capacity.
>>                                             ^^^^^^^^^^^^^^^^^^^^
>>
>> Do we use a new CPU capacity value `performance capacity (1)` here?
>>
>> Which I guess is `capacity_orig_of(cpu) - thermal_load_avg(cpu_rq(cpu)`.
>>
>> I'm asking since util_fits_cpu() still uses: `capacity_orig_thermal (2)
>> = capacity_orig - arch_scale_thermal_pressure()` when checking whether
>> to return -1. Shouldn't (1) and (2) be the same?
> 
> I'm all in favor of both being capacity_orig_of(cpu) -
> thermal_load_avg(cpu_rq(cpu) like the capacity inversion detection

I think we need a handy name for this new capacity value, which seems to
be `capacity_orig - capacity reduced by thermal`. And we should either
use `thermal_load_avg` or `thermal pressure` for the latter part. And
then we should use this consistently in all these places:
util_fits_cpu(), feec(), sic().

[...]