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

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

On 19/01/2023 10:08, Vincent Guittot wrote:
> On Wed, 18 Jan 2023 at 17:48, Qais Yousef <qyousef@layalina.io> wrote:
>>
>> On 01/18/23 09:15, Vincent Guittot wrote:
>>> On Mon, 16 Jan 2023 at 15:56, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>>>>
>>>> On 16/01/2023 12:23, Vincent Guittot wrote:
>>>>> On Mon, 16 Jan 2023 at 10:07, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>>>>>>
>>>>>> On 13/01/2023 14:40, Vincent Guittot wrote:

[...]

>>>>>> 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().
>>>
>>> Ok, let me change this everywhere
>>
>> I'm not keen on this :-/
>>
>> Changing this everywhere could have implications beyond our simple capabilities
>> of testing now :(

It's actually not everywhere. I'm aware of 2 occurrences now in which we
use 'cap_orig - th_pressure': in feec()/compute_energy() (commit
489f16459e00 "sched/fair: Take thermal pressure into account while
estimating energy") and now also in util_fits_cpu().

>> Current choice (in util_fits_cpu()) was based on a direct feedback from Xuewen.

I went through these ~40 emails in the '[PATCH] sched: Take thermal
pressure into account when determine rt fits capacity' thread (1):

https://lkml.kernel.org/r/20220407051932.4071-1-xuewen.yan@unisoc.com

and the '[PATCH 1/7] sched/uclamp: Fix relationship between uclamp and
migration margin' (2):

https://lkml.kernel.org/r/20220629194632.1117723-2-qais.yousef@arm.com

There is this email from Xuewen in (1):

https://lkml.kernel.org/r/CAB8ipk--Y8HxetcmUhBmtWq6Mmd726QmDbcbibGLERJw_PUqkQ@mail.gmail.com

in which he mentioned that he prefers th_pressure but this was a CapInv
prototype in update_cpu_capacity() (the whole discussion was about
th_pressure in rt_task_fits_capacity()) rather than util_fits_cpu().

Maybe I missed something more directly related to util_fits_cpu()?

>> I think we should discuss how we can improve the situation instead rather than
>> worry about consistency. I don't think we can be consistent without doing some
>> improvements on thermal pressure response time.

I'm fine with discussing this next Wednesday.

We just have to watch out for v4 of this patch which uses `cap_orig -
thermal_load_avg()` in sic().

>> A separate proposal patch to invoke some testing and discussion is fine by me.
>>
>> Better keep it a separate work item please?
> 
> Ok, I'm going to keep the current use of arch_scale_thermal_pressure
> and thermal_load_avg for this patch

OK.