Messages in this thread |  | | From | Valentin Schneider <> | Subject | Re: [PATCH] sched/fair: handle case of task_h_load() returning 0 | Date | Wed, 08 Jul 2020 11:34:10 +0100 |
| |
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.
|  |