Messages in this thread | | | From | Vincent Guittot <> | Date | Thu, 19 Jan 2023 11:08:06 +0100 | Subject | Re: [PATCH v3] sched/fair: unlink misfit task from cpu overutilized |
| |
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: > > > > > > [...] > > > > > > >>> @@ -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 ... > > > > One comment from Qais was to minimize knowledge outside > > util_fits_cpu() that's why I pass both uclamp_min and uclamp_max. > > > > > > > > [...] > > > > > > >>> @@ -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(). > > > > 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 :( > > Current choice (in util_fits_cpu()) was based on a direct feedback from Xuewen. > 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. > > 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
Thanks
> > > Cheers > > -- > Qais Yousef
| |