Re: [PATCH] sched/fair: handle case of task_h_load() returning 0

[Valentin Schneider <valentin.schneider@arm.com>]

On 07/07/20 14:30, Vincent Guittot wrote:
> On Thu, 2 Jul 2020 at 18:28, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>>
>> On Thu, 2 Jul 2020 at 18:11, Valentin Schneider
>> <valentin.schneider@arm.com> wrote:
>> >
>> >
>> > On 02/07/20 15:42, Vincent Guittot wrote:
>> > > task_h_load() can return 0 in some situations like running stress-ng
>> > > mmapfork, which forks thousands of threads, in a sched group on a 224 cores
>> > > system. The load balance doesn't handle this correctly because
>> > > env->imbalance never decreases and it will stop pulling tasks only after
>> > > reaching loop_max, which can be equal to the number of running tasks of
>> > > the cfs. Make sure that imbalance will be decreased by at least 1.
>> > >
>> > > misfit task is the other feature that doesn't handle correctly such
>> > > situation although it's probably more difficult to face the problem
>> > > because of the smaller number of CPUs and running tasks on heterogenous
>> > > system.
>> > >
>> > > We can't simply ensure that task_h_load() returns at least one because it
>> > > would imply to handle underrun in other places.
>> >
>> > Nasty one, that...
>> >
>> > Random thought: isn't that the kind of thing we have scale_load() and
>> > scale_load_down() for? There's more uses of task_h_load() than I would like
>> > for this, but if we upscale its output (or introduce an upscaled variant),
>> > we could do something like:
>> >
>> > ---
>> > detach_tasks()
>> > {
>> >         long imbalance = env->imbalance;
>> >
>> >         if (env->migration_type == migrate_load)
>> >                 imbalance = scale_load(imbalance);
>> >
>> >         while (!list_empty(tasks)) {
>> >                 /* ... */
>> >                 switch (env->migration_type) {
>> >                 case migrate_load:
>> >                         load = task_h_load_upscaled(p);
>> >                         /* ... usual bits here ...*/
>> >                         lsub_positive(&env->imbalance, load);
>> >                         break;
>> >                         /* ... */
>> >                 }
>> >
>> >                 if (!scale_load_down(env->imbalance))
>> >                         break;
>> >         }
>> > }
>> > ---
>> >
>> > It's not perfect, and there's still the misfit situation to sort out -
>> > still, do you think this is something we could go towards?
>>
>> This will not work for 32bits system.
>>
>> For 64bits, I have to think a bit more if the upscale would fix all
>> cases and support propagation across a hierarchy. And in this case we
>> could also consider to make scale_load/scale_load_down a nop all the
>> time
>
> In addition that problem remains on 32bits, the problem can still
> happen after extending the scale so this current patch still makes
> sense.
>

Right, I think we'd want to have that at the very least for 32bit anyway. I
haven't done the math, but doesn't it require an obscene amount of tasks
for that to still happen on 64bit with the increased resolution?

> Then if we want to reduce the cases where task_h_load returns 0, we
> should better make scale_load_down a nop otherwise we will have to
> maintain 2 values h_load and scale_h_load across the hierarchy
>

I don't fully grasp yet how much surgery that would require, but it does
sound like something we've been meaning to do, see e.g. se_weight:

 * XXX we want to get rid of these helpers and use the full load resolution.