[PATCH] sched/eas: Don't update misfit status if the task is pinned

[PATCH] sched/eas: Don't update misfit status if the task is pinned

[Qais Yousef]

If the task is pinned to a cpu, setting the misfit status means that
we'll unnecessarily continuously attempt to migrate the task but fail.

This continuous failure will cause the balance_interval to increase to
a high value, and eventually cause unnecessary significant delays in
balancing the system when real imbalance happens.

Caught while testing uclamp where rt-app calibration loop was pinned to
cpu 0, shortly after which we spawn another task with high util_clamp
value. The task was failing to migrate after over 40ms of runtime due to
balance_interval unnecessary expanded to a very high value from the
calibration loop.

Not done here, but it could be useful to extend the check for pinning to
verify that the affinity of the task has a cpu that fits. We could end
up in a similar situation otherwise.

Fixes: 3b1baa6496e6 ("sched/fair: Add 'group_misfit_task' load-balance type")
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 197a51473e0c..9379a481dd8c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4060,7 +4060,7 @@ static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
 	if (!static_branch_unlikely(&sched_asym_cpucapacity))
 		return;
 
-	if (!p) {
+	if (!p || p->nr_cpus_allowed == 1) {
 		rq->misfit_task_load = 0;
 		return;
 	}
-- 
2.25.1

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Valentin Schneider]

Nit: Topic should probably be sched/fair - not many other than us
loonies talk about EAS, and misfit stuff doesn't require an energy
model, only CPU capacity asymmetry.

On 19/01/21 12:07, Qais Yousef wrote:
> If the task is pinned to a cpu, setting the misfit status means that
> we'll unnecessarily continuously attempt to migrate the task but fail.
>
> This continuous failure will cause the balance_interval to increase to
> a high value, and eventually cause unnecessary significant delays in
> balancing the system when real imbalance happens.
>
> Caught while testing uclamp where rt-app calibration loop was pinned to
> cpu 0, shortly after which we spawn another task with high util_clamp
> value. The task was failing to migrate after over 40ms of runtime due to
> balance_interval unnecessary expanded to a very high value from the
> calibration loop.
>
> Not done here, but it could be useful to extend the check for pinning to
> verify that the affinity of the task has a cpu that fits. We could end
> up in a similar situation otherwise.
>
> Fixes: 3b1baa6496e6 ("sched/fair: Add 'group_misfit_task' load-balance type")
> Signed-off-by: Qais Yousef <qais.yousef@arm.com>

Acked-by: Valentin Schneider <valentin.schneider@arm.com>

Thanks!

> ---
>  kernel/sched/fair.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 197a51473e0c..9379a481dd8c 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4060,7 +4060,7 @@ static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
>       if (!static_branch_unlikely(&sched_asym_cpucapacity))
>               return;
>
> -	if (!p) {
> +	if (!p || p->nr_cpus_allowed == 1) {
>               rq->misfit_task_load = 0;
>               return;
>       }
> --
> 2.25.1

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Vincent Guittot]

On Tue, 19 Jan 2021 at 13:08, Qais Yousef <qais.yousef@arm.com> wrote:
>
> If the task is pinned to a cpu, setting the misfit status means that
> we'll unnecessarily continuously attempt to migrate the task but fail.
>
> This continuous failure will cause the balance_interval to increase to
> a high value, and eventually cause unnecessary significant delays in
> balancing the system when real imbalance happens.
>
> Caught while testing uclamp where rt-app calibration loop was pinned to
> cpu 0, shortly after which we spawn another task with high util_clamp
> value. The task was failing to migrate after over 40ms of runtime due to
> balance_interval unnecessary expanded to a very high value from the
> calibration loop.
>
> Not done here, but it could be useful to extend the check for pinning to
> verify that the affinity of the task has a cpu that fits. We could end
> up in a similar situation otherwise.
>
> Fixes: 3b1baa6496e6 ("sched/fair: Add 'group_misfit_task' load-balance type")
> Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> ---
>  kernel/sched/fair.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 197a51473e0c..9379a481dd8c 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4060,7 +4060,7 @@ static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
>         if (!static_branch_unlikely(&sched_asym_cpucapacity))
>                 return;
>
> -       if (!p) {
> +       if (!p || p->nr_cpus_allowed == 1) {

Side question: What happens if there is 2 misfit tasks and the current
one is pinned but not the other waiting one

>                 rq->misfit_task_load = 0;
>                 return;
>         }
> --
> 2.25.1
>

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Valentin Schneider]

On 19/01/21 14:34, Vincent Guittot wrote:
> On Tue, 19 Jan 2021 at 13:08, Qais Yousef <qais.yousef@arm.com> wrote:
>>
>> If the task is pinned to a cpu, setting the misfit status means that
>> we'll unnecessarily continuously attempt to migrate the task but fail.
>>
>> This continuous failure will cause the balance_interval to increase to
>> a high value, and eventually cause unnecessary significant delays in
>> balancing the system when real imbalance happens.
>>
>> Caught while testing uclamp where rt-app calibration loop was pinned to
>> cpu 0, shortly after which we spawn another task with high util_clamp
>> value. The task was failing to migrate after over 40ms of runtime due to
>> balance_interval unnecessary expanded to a very high value from the
>> calibration loop.
>>
>> Not done here, but it could be useful to extend the check for pinning to
>> verify that the affinity of the task has a cpu that fits. We could end
>> up in a similar situation otherwise.
>>
>> Fixes: 3b1baa6496e6 ("sched/fair: Add 'group_misfit_task' load-balance type")
>> Signed-off-by: Qais Yousef <qais.yousef@arm.com>
>> ---
>>  kernel/sched/fair.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 197a51473e0c..9379a481dd8c 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -4060,7 +4060,7 @@ static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
>>         if (!static_branch_unlikely(&sched_asym_cpucapacity))
>>                 return;
>>
>> -       if (!p) {
>> +       if (!p || p->nr_cpus_allowed == 1) {
>
> Side question: What happens if there is 2 misfit tasks and the current
> one is pinned but not the other waiting one
>

update_misfit_status() is called either on the current task (at tick) or
on the task picked by pick_next_task_fair() - i.e. CFS current or
about-to-be-current.

So if you have 2 CPU hogs enqueued on a single LITTLE, and one of them
is pinned, the other one will be moved away either via regular load
balance, or via misfit balance sometime after it's picked as the next
task to run.

Admittedly that second case suffers from unfortunate timing mostly
related to the load balance interval. There was an old patch in the
Android stack that would reduce the balance interval upon detecting a
misfit task to "accelerate" its upmigration; this might need to be
revisited...

>>                 rq->misfit_task_load = 0;
>>                 return;
>>         }
>> --
>> 2.25.1
>>

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Vincent Guittot]

On Tue, 19 Jan 2021 at 14:54, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
> On 19/01/21 14:34, Vincent Guittot wrote:
> > On Tue, 19 Jan 2021 at 13:08, Qais Yousef <qais.yousef@arm.com> wrote:
> >>
> >> If the task is pinned to a cpu, setting the misfit status means that
> >> we'll unnecessarily continuously attempt to migrate the task but fail.
> >>
> >> This continuous failure will cause the balance_interval to increase to
> >> a high value, and eventually cause unnecessary significant delays in
> >> balancing the system when real imbalance happens.
> >>
> >> Caught while testing uclamp where rt-app calibration loop was pinned to
> >> cpu 0, shortly after which we spawn another task with high util_clamp
> >> value. The task was failing to migrate after over 40ms of runtime due to
> >> balance_interval unnecessary expanded to a very high value from the
> >> calibration loop.
> >>
> >> Not done here, but it could be useful to extend the check for pinning to
> >> verify that the affinity of the task has a cpu that fits. We could end
> >> up in a similar situation otherwise.
> >>
> >> Fixes: 3b1baa6496e6 ("sched/fair: Add 'group_misfit_task' load-balance type")
> >> Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> >> ---
> >>  kernel/sched/fair.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 197a51473e0c..9379a481dd8c 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -4060,7 +4060,7 @@ static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> >>         if (!static_branch_unlikely(&sched_asym_cpucapacity))
> >>                 return;
> >>
> >> -       if (!p) {
> >> +       if (!p || p->nr_cpus_allowed == 1) {
> >
> > Side question: What happens if there is 2 misfit tasks and the current
> > one is pinned but not the other waiting one
> >
>
> update_misfit_status() is called either on the current task (at tick) or
> on the task picked by pick_next_task_fair() - i.e. CFS current or
> about-to-be-current.
>
> So if you have 2 CPU hogs enqueued on a single LITTLE, and one of them
> is pinned, the other one will be moved away either via regular load

This doesn't seem reliable because it uses load or nr_running

> balance, or via misfit balance sometime after it's picked as the next
> task to run.
>
> Admittedly that second case suffers from unfortunate timing mostly
> related to the load balance interval. There was an old patch in the
> Android stack that would reduce the balance interval upon detecting a

Shouldn't we keep track of enqueue misfit tasks instead ?

> misfit task to "accelerate" its upmigration; this might need to be
> revisited...
>
> >>                 rq->misfit_task_load = 0;
> >>                 return;
> >>         }
> >> --
> >> 2.25.1
> >>

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Valentin Schneider]

On 19/01/21 15:19, Vincent Guittot wrote:
> On Tue, 19 Jan 2021 at 14:54, Valentin Schneider
> <valentin.schneider@arm.com> wrote:
>> On 19/01/21 14:34, Vincent Guittot wrote:
>> >> -       if (!p) {
>> >> +       if (!p || p->nr_cpus_allowed == 1) {
>> >
>> > Side question: What happens if there is 2 misfit tasks and the current
>> > one is pinned but not the other waiting one
>> >
>>
>> update_misfit_status() is called either on the current task (at tick) or
>> on the task picked by pick_next_task_fair() - i.e. CFS current or
>> about-to-be-current.
>>
>> So if you have 2 CPU hogs enqueued on a single LITTLE, and one of them
>> is pinned, the other one will be moved away either via regular load
>
> This doesn't seem reliable because it uses load or nr_running
>

Right

>> balance, or via misfit balance sometime after it's picked as the next
>> task to run.
>>
>> Admittedly that second case suffers from unfortunate timing mostly
>> related to the load balance interval. There was an old patch in the
>> Android stack that would reduce the balance interval upon detecting a
>
> Shouldn't we keep track of enqueue misfit tasks instead ?
>

That might help. This being CFS however, the maintenance of it might
prove prohibitive. I faintly recall having concerns about expanding the
misfit logic to non-current tasks, but nothing comes to mind right
now...

Historically (before PELT time scaling) I think it wasn't possible to
have a steady state with more than one misfit task on a rq, as two
similar CPU hogs would end up with a utilization value of at most half
the CPU's capacity. If those were at e.g. opposite ends of the NICE
spectrum, if one would be flagged as misfit then the other wouldn't
(can't have two slices greater than 80%!)

I *think* that's still true with timescaling, but then we did add the
uclamp stuff to make tasks look bigger than they are...

>> misfit task to "accelerate" its upmigration; this might need to be
>> revisited...
>>
>> >>                 rq->misfit_task_load = 0;
>> >>                 return;
>> >>         }
>> >> --
>> >> 2.25.1
>> >>

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Vincent Guittot]

On Tue, 19 Jan 2021 at 17:55, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
> On 19/01/21 15:19, Vincent Guittot wrote:
> > On Tue, 19 Jan 2021 at 14:54, Valentin Schneider
> > <valentin.schneider@arm.com> wrote:
> >> On 19/01/21 14:34, Vincent Guittot wrote:
> >> >> -       if (!p) {
> >> >> +       if (!p || p->nr_cpus_allowed == 1) {
> >> >
> >> > Side question: What happens if there is 2 misfit tasks and the current
> >> > one is pinned but not the other waiting one
> >> >
> >>
> >> update_misfit_status() is called either on the current task (at tick) or
> >> on the task picked by pick_next_task_fair() - i.e. CFS current or
> >> about-to-be-current.
> >>
> >> So if you have 2 CPU hogs enqueued on a single LITTLE, and one of them
> >> is pinned, the other one will be moved away either via regular load
> >
> > This doesn't seem reliable because it uses load or nr_running
> >
>
> Right
>
> >> balance, or via misfit balance sometime after it's picked as the next
> >> task to run.
> >>
> >> Admittedly that second case suffers from unfortunate timing mostly
> >> related to the load balance interval. There was an old patch in the
> >> Android stack that would reduce the balance interval upon detecting a
> >
> > Shouldn't we keep track of enqueue misfit tasks instead ?
> >
>
> That might help. This being CFS however, the maintenance of it might
> prove prohibitive. I faintly recall having concerns about expanding the
> misfit logic to non-current tasks, but nothing comes to mind right
> now...
>
> Historically (before PELT time scaling) I think it wasn't possible to
> have a steady state with more than one misfit task on a rq, as two
> similar CPU hogs would end up with a utilization value of at most half
> the CPU's capacity. If those were at e.g. opposite ends of the NICE
> spectrum, if one would be flagged as misfit then the other wouldn't
> (can't have two slices greater than 80%!)

I remember this

>
> I *think* that's still true with timescaling, but then we did add the
> uclamp stuff to make tasks look bigger than they are...

Yeah, uclamp makes it possible now

>
> >> misfit task to "accelerate" its upmigration; this might need to be
> >> revisited...
> >>
> >> >>                 rq->misfit_task_load = 0;
> >> >>                 return;
> >> >>         }
> >> >> --
> >> >> 2.25.1
> >> >>

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Quentin Perret]

On Tuesday 19 Jan 2021 at 12:07:55 (+0000), Qais Yousef wrote:
> If the task is pinned to a cpu, setting the misfit status means that
> we'll unnecessarily continuously attempt to migrate the task but fail.
> 
> This continuous failure will cause the balance_interval to increase to
> a high value, and eventually cause unnecessary significant delays in
> balancing the system when real imbalance happens.
> 
> Caught while testing uclamp where rt-app calibration loop was pinned to
> cpu 0, shortly after which we spawn another task with high util_clamp
> value. The task was failing to migrate after over 40ms of runtime due to
> balance_interval unnecessary expanded to a very high value from the
> calibration loop.
> 
> Not done here, but it could be useful to extend the check for pinning to
> verify that the affinity of the task has a cpu that fits. We could end
> up in a similar situation otherwise.

Do you mean failing the sched_setaffinity syscall if e.g. the task
has a min clamp that is higher than the capacity of the CPUs to which it
will be pinned? If so, I'm not sure if we really want that.

But this patch makes sense on its own for sure, so:

Reviewed-by: Quentin Perret <qperret@google.com>

Thanks,
Quentin

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Qais Yousef]

On 01/19/21 15:35, Quentin Perret wrote:
> On Tuesday 19 Jan 2021 at 12:07:55 (+0000), Qais Yousef wrote:
> > If the task is pinned to a cpu, setting the misfit status means that
> > we'll unnecessarily continuously attempt to migrate the task but fail.
> > 
> > This continuous failure will cause the balance_interval to increase to
> > a high value, and eventually cause unnecessary significant delays in
> > balancing the system when real imbalance happens.
> > 
> > Caught while testing uclamp where rt-app calibration loop was pinned to
> > cpu 0, shortly after which we spawn another task with high util_clamp
> > value. The task was failing to migrate after over 40ms of runtime due to
> > balance_interval unnecessary expanded to a very high value from the
> > calibration loop.
> > 
> > Not done here, but it could be useful to extend the check for pinning to
> > verify that the affinity of the task has a cpu that fits. We could end
> > up in a similar situation otherwise.
> 
> Do you mean failing the sched_setaffinity syscall if e.g. the task
> has a min clamp that is higher than the capacity of the CPUs to which it
> will be pinned? If so, I'm not sure if we really want that.

No. In Android for instance, I'm worried a background task affined to little
cores that has a utilization > capacity_of(little) will trigger the same
problem. It'll be affined to more than just 1 cpu, but none of the little cpus
will actually fit.

Makes sense?

> But this patch makes sense on its own for sure, so:
> 
> Reviewed-by: Quentin Perret <qperret@google.com>

Thanks

--
Qais Yousef

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Quentin Perret]

On Tuesday 19 Jan 2021 at 16:40:27 (+0000), Qais Yousef wrote:
> On 01/19/21 15:35, Quentin Perret wrote:
> > Do you mean failing the sched_setaffinity syscall if e.g. the task
> > has a min clamp that is higher than the capacity of the CPUs to which it
> > will be pinned? If so, I'm not sure if we really want that.
> 
> No. In Android for instance, I'm worried a background task affined to little
> cores that has a utilization > capacity_of(little) will trigger the same
> problem. It'll be affined to more than just 1 cpu, but none of the little cpus
> will actually fit.
> 
> Makes sense?

Now yes.

I agree this may be a real problem, but capacity_of() very much is a
per-CPU thing, because of RT pressure and such, and that is not a static
thing by any mean. So, even if the task doesn't fit on any CPU _now_ we
might still want to mark it misfit, just so it can be picked up by a
potential idle balance on another CPU later on. Maybe capacity_orig_of
would be preferable?

Thanks,
Quentin

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Qais Yousef]

On 01/19/21 16:55, Quentin Perret wrote:
> On Tuesday 19 Jan 2021 at 16:40:27 (+0000), Qais Yousef wrote:
> > On 01/19/21 15:35, Quentin Perret wrote:
> > > Do you mean failing the sched_setaffinity syscall if e.g. the task
> > > has a min clamp that is higher than the capacity of the CPUs to which it
> > > will be pinned? If so, I'm not sure if we really want that.
> > 
> > No. In Android for instance, I'm worried a background task affined to little
> > cores that has a utilization > capacity_of(little) will trigger the same
> > problem. It'll be affined to more than just 1 cpu, but none of the little cpus
> > will actually fit.
> > 
> > Makes sense?
> 
> Now yes.
> 
> I agree this may be a real problem, but capacity_of() very much is a
> per-CPU thing, because of RT pressure and such, and that is not a static
> thing by any mean. So, even if the task doesn't fit on any CPU _now_ we
> might still want to mark it misfit, just so it can be picked up by a
> potential idle balance on another CPU later on. Maybe capacity_orig_of
> would be preferable?

Hmm IIUC you want to still tag it as misfit so it'll be balanced within the
little cores in case there's another core with more spare capacity, right?

This needs more thinking. Misfit doesn't seem the right mechanism to handle
this. If there are multiple tasks crammed on the same CPU, then we should try
to distribute yes. If it is the only task I can't see this being useful unless
the pressure is very high. Which could be an indication of another problem in
the system..

Thanks

--
Qais Yousef

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Quentin Perret]

On Tuesday 19 Jan 2021 at 17:42:44 (+0000), Qais Yousef wrote:
> Hmm IIUC you want to still tag it as misfit so it'll be balanced within the
> little cores in case there's another core with more spare capacity, right?

Well yes but that's just a special case. But even you have big CPUs in
the affinity mask, you may find that the task fits on none of the CPUs
because they're currently under pressure. But in this case, you may
still want to mark the task as misfit because being under pressure may
be a relatively transient state.

Re: [PATCH] sched/eas: Don't update misfit status if the task is pinned

[Qais Yousef]

On 01/19/21 17:50, Quentin Perret wrote:
> On Tuesday 19 Jan 2021 at 17:42:44 (+0000), Qais Yousef wrote:
> > Hmm IIUC you want to still tag it as misfit so it'll be balanced within the
> > little cores in case there's another core with more spare capacity, right?
> 
> Well yes but that's just a special case. But even you have big CPUs in
> the affinity mask, you may find that the task fits on none of the CPUs
> because they're currently under pressure. But in this case, you may
> still want to mark the task as misfit because being under pressure may
> be a relatively transient state.

Okay. So your thoughts are that if the utilization is above capacity_orig_of()
then marking it as misfit is meaningless (taking into account the cpus it is
affined to). Which I agree with. But if it is less than capacity_orig_of() but
doesn't fit because of pressure ie:

	util <= capacity_orig_of(cpu) && util > capacity_of(cpu)

then we should mark it as misfit as it currently does. I think this makes sense
too. There's the margin to consider in the mix here too. And util clamp
effects. And the fact this gets called from pick_next_task_fair() which is
a hot path :-)

Unless someone else beats me to it, I'll send a patch eventually :-)

Thanks

--
Qais Yousef