Messages in this thread | | | From | Vincent Guittot <> | Date | Mon, 8 Feb 2021 16:29:11 +0100 | Subject | Re: [PATCH 3/8] sched/fair: Tweak misfit-related capacity checks |
| |
On Fri, 5 Feb 2021 at 21:07, Valentin Schneider <valentin.schneider@arm.com> wrote: > > On 05/02/21 18:17, Vincent Guittot wrote: > > On Fri, 5 Feb 2021 at 18:00, Valentin Schneider > >> >> @@ -8253,7 +8260,7 @@ check_cpu_capacity(struct rq *rq, struct sched_domain *sd) > >> >> static inline int check_misfit_status(struct rq *rq, struct sched_domain *sd) > >> >> { > >> >> return rq->misfit_task_load && > >> >> - (rq->cpu_capacity_orig < rq->rd->max_cpu_capacity || > >> >> + (capacity_greater(rq->rd->max_cpu_capacity, rq->cpu_capacity_orig) || > >> > > >> > Why do you add a margin here whereas there was no margin before ? > >> > > >> > >> Comparing capacities without any sort of filter can lead to ping-ponging > >> tasks around (capacity values very easily fluctuate by +/- 1, if not more). > > > > max_cpu_capacity reflects the max of the cpu_capacity_orig values > > don't aim to change and can be considered as static values. > > It would be better to fix this rounding problem (if any) in > > topology_get_cpu_scale instead of computing a margin every time it's > > used > > > > That's embarrassing, I was convinced we had something updating > rd->max_cpu_capacity with actual rq->capacity values... But as you point > out that's absolutely not the case, it's all based on rq->capacity_orig, > which completely invalidates patch 5/8. > > Welp. > > Perhaps I can still keep 5/8 with something like > > if (!rq->misfit_task_load) > return false; > > do { > if (capacity_greater(group->sgc->max_capacity, rq->cpu_capacity)) > return true; > > group = group->next; > } while (group != sd->groups);
I don't catch what you want to achieve with this while loop compared to the original condition which is : trigger a load_balance : - if there is CPU with higher original capacity - or if the capacity of this cpu has significantly reduced because of pressure and there is maybe others with more capacity even if it's one with highest original capacity
> > return false; > > This works somewhat well for big.LITTLE, but for DynamIQ systems under a > single L3 this ends up iterating over all the CPUs :/
| |