Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 20 Jan 2023 18:53:00 +0000 | Subject | Re: [PATCH v3] sched/fair: unlink misfit task from cpu overutilized | From | Dietmar Eggemann <> |
| |
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.
| ![\](/images/icornerr.gif) |