Re: [PATCH v2] sched/fair: Check a task has a fitting cpu when updating misfit
From: Qais Yousef
Date: Thu Dec 28 2023 - 18:19:19 EST
Hi Pierre
On 12/20/23 17:44, Pierre Gondois wrote:
> Hello Qais,
>
> On 12/12/23 16:40, Qais Yousef wrote:
> > From: Qais Yousef <qais.yousef@xxxxxxx>
> >
> > If a misfit task is affined to a subset of the possible cpus, we need to
> > verify that one of these cpus can fit it. Otherwise the load balancer
> > code will continuously trigger needlessly leading the balance_interval
> > to increase in return and eventually end up with a situation where real
> > imbalances take a long time to address because of this impossible
> > imbalance situation.
> >
> > This can happen in Android world where it's common for background tasks
> > to be restricted to little cores.
> >
> > Similarly if we can't fit the biggest core, triggering misfit is
> > pointless as it is the best we can ever get on this system.
> >
> > To be able to detect that; we use asym_cap_list to iterate through
> > capacities in the system to see if the task is able to run at a higher
> > capacity level based on its p->cpus_ptr.
> >
> > To be able to iterate through capacity levels, export asym_cap_list to
> > allow for fast traversal of all available capacity levels in the system.
> >
> > Test:
> > =====
> >
> > Add
> >
> > trace_printk("balance_interval = %lu\n", interval)
> >
> > in get_sd_balance_interval().
>
> Just in case, to avoid taking into account the 'cpu_busy' factor in
> get_sd_balance_interval(), it might be better to track
> 'sd->balance_interval' directly.
The compound effect is important.
>
> Also (and as you said in the commit message), the interest of this patch
> is more to avoid needlessly trying to balance tasks when tasks are pinned
> rather than having a low balance interval. So it might be better to observe
It is about having a low balance interval. Balancing needlessly has a big
consequence on balance interval; which in return on ensuring load balance
happens in timely manner to correct imbalances and handle misfits.
I think phantom domains has been exaggerating these problems.
> either 'sd->nr_balance_failed' or the number of occurrence where
> check_misfit_status() returns 1 and no task is moved.
>
> When testing the patch, with a CPU-bound task pinned to little CPUs, running
> during 1s (and on one iteration):
> - (top sd)->nr_balance_failed was increased:
> without patch:
> 150 times
> with patch:
> 6 times
> - check_misfit_status() returned:
> without patch:
> - ret=0: 591
> - ret=1: 1414
> with patch
> - ret=0: 1755
> - ret=1: 5
> So there should also be improvements in that regard. I could observe
> improvements similar to what you saw regarding the balance_interval.
Thanks for testing!
>
> >
> > run
> > if [ "$MASK" != "0" ]; then
> > adb shell "taskset -a $MASK cat /dev/zero > /dev/null"
> > fi
> > sleep 10
> > // parse ftrace buffer counting the occurrence of each valaue
> >
> > Where MASK is either:
> >
> > * 0: no busy task running
> > * 1: busy task is pinned to 1 cpu; handled today to not cause
> > misfit
> > * f: busy task pinned to little cores, simulates busy background
> > task, demonstrates the problem to be fixed
> >
> > Results:
> > ========
> >
> > Note how occurrence of balance_interval = 128 overshoots for MASK = f.
> >
> > BEFORE
> > ------
> >
> > MASK=0
> >
> > 1 balance_interval = 175
> > 120 balance_interval = 128
> > 846 balance_interval = 64
> > 55 balance_interval = 63
> > 215 balance_interval = 32
> > 2 balance_interval = 31
> > 2 balance_interval = 16
> > 4 balance_interval = 8
> > 1870 balance_interval = 4
> > 65 balance_interval = 2
> >
> > MASK=1
> >
> > 27 balance_interval = 175
> > 37 balance_interval = 127
> > 840 balance_interval = 64
> > 167 balance_interval = 63
> > 449 balance_interval = 32
> > 84 balance_interval = 31
> > 304 balance_interval = 16
> > 1156 balance_interval = 8
> > 2781 balance_interval = 4
> > 428 balance_interval = 2
> >
> > MASK=f
> >
> > 1 balance_interval = 175
> > 1328 balance_interval = 128
> > 44 balance_interval = 64
> > 101 balance_interval = 63
> > 25 balance_interval = 32
> > 5 balance_interval = 31
> > 23 balance_interval = 16
> > 23 balance_interval = 8
> > 4306 balance_interval = 4
> > 177 balance_interval = 2
> >
> > AFTER
> > -----
> >
> > Note how the high values almost disappear for all MASK values. The
> > system has background tasks that could trigger the problem without
> > simulate it even with MASK=0.
> >
> > MASK=0
> >
> > 103 balance_interval = 63
> > 19 balance_interval = 31
> > 194 balance_interval = 8
> > 4827 balance_interval = 4
> > 179 balance_interval = 2
> >
> > MASK=1
> >
> > 131 balance_interval = 63
> > 1 balance_interval = 31
> > 87 balance_interval = 8
> > 3600 balance_interval = 4
> > 7 balance_interval = 2
> >
> > MASK=f
> >
> > 8 balance_interval = 127
> > 182 balance_interval = 63
> > 3 balance_interval = 31
> > 9 balance_interval = 16
> > 415 balance_interval = 8
> > 3415 balance_interval = 4
> > 21 balance_interval = 2
> >
> > Signed-off-by: Qais Yousef <qais.yousef@xxxxxxx>
> > Signed-off-by: Qais Yousef (Google) <qyousef@xxxxxxxxxxx>
> > ---
> >
> > Changes since v1:
> >
> > * Use asym_cap_list (thanks Dietmar) to iterate instead of iterating
> > through every cpu which Vincent was concerned about.
> > * Use uclamped util to compare with capacity instead of util_fits_cpu()
> > when iterating through capcities (Dietmar).
> > * Update commit log with test results to better demonstrate the problem
> >
> > v1 discussion: https://lore.kernel.org/lkml/20230820203429.568884-1-qyousef@xxxxxxxxxxx/#t
> >
> > Food for thoughts: should misfit cause balance_interval to double? This patch
> > will still be needed if the answer is yes to avoid unnecessary misfit-lb to
> > trigger repeatedly anyway.
> >
> > kernel/sched/fair.c | 59 ++++++++++++++++++++++++++++++++++++-----
> > kernel/sched/sched.h | 13 +++++++++
> > kernel/sched/topology.c | 13 +--------
> > 3 files changed, 66 insertions(+), 19 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index bcea3d55d95d..94e2f659fef9 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -5065,17 +5065,59 @@ static inline int task_fits_cpu(struct task_struct *p, int cpu)
> > static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> > {
> > + unsigned long uclamp_min, uclamp_max;
> > + struct asym_cap_data *entry, *next;
>
> These variables could be moved aside the 'clamped_util' declaration
> as their scope is limited (same remark for 'has_fitting_cpu').
>
> > + bool has_fitting_cpu = false;
> > + unsigned long util, cpu_cap;
> > + int cpu = cpu_of(rq);
> > +
> > if (!sched_asym_cpucap_active())
> > return;
> > - if (!p || p->nr_cpus_allowed == 1) {
> > - rq->misfit_task_load = 0;
> > - return;
> > - }
> > + if (!p || p->nr_cpus_allowed == 1)
> > + goto out;
> > - if (task_fits_cpu(p, cpu_of(rq))) {
> > - rq->misfit_task_load = 0;
> > - return;
> > + cpu_cap = arch_scale_cpu_capacity(cpu);
> > +
> > + /* If we can't fit the biggest CPU, that's the best we can ever get. */
> > + if (cpu_cap == SCHED_CAPACITY_SCALE)
> > + goto out;
> > +
> > + uclamp_min = uclamp_eff_value(p, UCLAMP_MIN);
> > + uclamp_max = uclamp_eff_value(p, UCLAMP_MAX);
> > + util = task_util_est(p);
> > +
> > + if (util_fits_cpu(util, uclamp_min, uclamp_max, cpu) > 0)
> > + goto out;
> > +
> > + /*
> > + * If the task affinity is not set to default, make sure it is not
> > + * restricted to a subset where no CPU can ever fit it. Triggering
> > + * misfit in this case is pointless as it has no where better to move
> > + * to. And it can lead to balance_interval to grow too high as we'll
> > + * continuously fail to move it anywhere.
> > + */
> > + if (!cpumask_equal(p->cpus_ptr, cpu_possible_mask)) {
>
> Just to avoid one level of indentation, it might be better to invert the condition.
I think better keep this way and move the variables down here as you suggested.
>
> > + unsigned long clamped_util = clamp(util, uclamp_min, uclamp_max);
> > +
> > + list_for_each_entry_safe(entry, next, &asym_cap_list, link) {
>
> I think list_for_each_entry_safe() allows to safely remove an element
> from a list, but I'm not sure it protects against a concurrent
> element removal + free.
>
> I think an rcu would be the right way to protect against that
> in order to safely access asym_cap_list's elements. Otherwise
> asym_cpu_capacity_scan()
> \-list_del(&entry->link);
> \-kfree(entry);
> might free the element.
Hmm I borrowed this usage from there. But yes, you're right there's use after
free here. I was desperate to avoid the rcu conversion I guess :)
>
> Also, if the list is accessed so many times to look for a CPU with the
> highest capacity, maybe it would be good to order the list and
> access its element from the highest capacity to the lowest.
Good idea. But I'd rather this be split into a different patch. I'll consider
it in next version still if it didn't make things unnecessarily complex.
>
> > + if (entry->capacity > cpu_cap) {
> > + cpumask_t *cpumask;
> > +
> > + if (clamped_util > entry->capacity)
> > + continue;
> > +
> > + cpumask = cpu_capacity_span(entry);
> > + if (!cpumask_intersects(p->cpus_ptr, cpumask))
> > + continue;
> > +
> > + has_fitting_cpu = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!has_fitting_cpu)
> > + goto out;
> > }
> > /*
> > @@ -5083,6 +5125,9 @@ static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> > * task_h_load() returns 0.
> > */
> > rq->misfit_task_load = max_t(unsigned long, task_h_load(p), 1);
> > + return;
> > +out:
> > + rq->misfit_task_load = 0;
> > }
>
> Along with this function, check_misfit_status() is checking that:
> rq->cpu_capacity_orig < rq->rd->max_cpu_capacity
> With this patch, maybe this condition is not required anymore.
Yes! Updated
@@ -9628,9 +9628,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 &&
- (arch_scale_cpu_capacity(rq->cpu) < rq->rd->max_cpu_capacity ||
- check_cpu_capacity(rq, sd));
+ return rq->misfit_task_load && check_cpu_capacity(rq, sd);
}
Thanks!
--
Qais Yousef