LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] sched/fair: Fix per-CPU kthread and wakee stacking for asym CPU capacity
@ 2021-11-24 14:14 Vincent Donnefort
  2021-11-24 17:11 ` Valentin Schneider
  2021-11-25  9:10 ` Vincent Guittot
  0 siblings, 2 replies; 5+ messages in thread
From: Vincent Donnefort @ 2021-11-24 14:14 UTC (permalink / raw)
  To: peterz, mingo, vincent.guittot
  Cc: linux-kernel, mgorman, dietmar.eggemann, valentin.schneider,
	Vincent Donnefort

A shortcut has been introduced in select_idle_sibling() to return prev_cpu
if the wakee is woken up by a per-CPU kthread. This is an issue for
asymmetric CPU capacity systems where the wakee might not fit prev_cpu
anymore. Evaluate asym_fits_capacity() for prev_cpu before using that
shortcut.

Fixes: 52262ee567ad ("sched/fair: Allow a per-CPU kthread waking a task to stack on the same CPU, to fix XFS performance regression")
Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 6291876a9d32..b90dc6fd86ca 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6410,7 +6410,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
 	 */
 	if (is_per_cpu_kthread(current) &&
 	    prev == smp_processor_id() &&
-	    this_rq()->nr_running <= 1) {
+	    this_rq()->nr_running <= 1 &&
+	    asym_fits_capacity(task_util, prev)) {
 		return prev;
 	}
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sched/fair: Fix per-CPU kthread and wakee stacking for asym CPU capacity
  2021-11-24 14:14 [PATCH] sched/fair: Fix per-CPU kthread and wakee stacking for asym CPU capacity Vincent Donnefort
@ 2021-11-24 17:11 ` Valentin Schneider
  2021-11-24 17:58   ` Vincent Donnefort
  2021-11-25  9:10 ` Vincent Guittot
  1 sibling, 1 reply; 5+ messages in thread
From: Valentin Schneider @ 2021-11-24 17:11 UTC (permalink / raw)
  To: Vincent Donnefort, peterz, mingo, vincent.guittot
  Cc: linux-kernel, mgorman, dietmar.eggemann, Vincent Donnefort

On 24/11/21 14:14, Vincent Donnefort wrote:
> A shortcut has been introduced in select_idle_sibling() to return prev_cpu
> if the wakee is woken up by a per-CPU kthread. This is an issue for
> asymmetric CPU capacity systems where the wakee might not fit prev_cpu
> anymore. Evaluate asym_fits_capacity() for prev_cpu before using that
> shortcut.
>
> Fixes: 52262ee567ad ("sched/fair: Allow a per-CPU kthread waking a task to stack on the same CPU, to fix XFS performance regression")

Shouldn't that rather be

  b4c9c9f15649 ("sched/fair: Prefer prev cpu in asymmetric wakeup path")

? This is an ulterior commit to the one you point to, and before then
asymmetric CPU systems wouldn't use any of the sis() heuristics.

I reportedly reviewed said commit back then, and don't recall anything
specific about that conditional... The cover-letter for v2 states:

  https://lore.kernel.org/lkml/20201028174412.680-1-vincent.guittot@linaro.org/
  """
  don't check capacity for the per-cpu kthread UC because the assumption is
  that the wakee queued work for the per-cpu kthread that is now complete and
  the task was already on this cpu.
  """

So the assumption here is that current is gonna sleep right after waking up
p, so current's utilization doesn't matter, and p was already on prev, so
it should fit there...

I'm thinking things should actually be OK with your other patch that
excludes 'current == swapper' from this condition.

> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 6291876a9d32..b90dc6fd86ca 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6410,7 +6410,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>        */
>       if (is_per_cpu_kthread(current) &&
>           prev == smp_processor_id() &&
> -	    this_rq()->nr_running <= 1) {
> +	    this_rq()->nr_running <= 1 &&
> +	    asym_fits_capacity(task_util, prev)) {
>               return prev;
>       }
>
> --
> 2.25.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sched/fair: Fix per-CPU kthread and wakee stacking for asym CPU capacity
  2021-11-24 17:11 ` Valentin Schneider
@ 2021-11-24 17:58   ` Vincent Donnefort
  2021-11-24 18:04     ` Valentin Schneider
  0 siblings, 1 reply; 5+ messages in thread
From: Vincent Donnefort @ 2021-11-24 17:58 UTC (permalink / raw)
  To: Valentin Schneider
  Cc: peterz, mingo, vincent.guittot, linux-kernel, mgorman, dietmar.eggemann

On Wed, Nov 24, 2021 at 05:11:32PM +0000, Valentin Schneider wrote:
> On 24/11/21 14:14, Vincent Donnefort wrote:
> > A shortcut has been introduced in select_idle_sibling() to return prev_cpu
> > if the wakee is woken up by a per-CPU kthread. This is an issue for
> > asymmetric CPU capacity systems where the wakee might not fit prev_cpu
> > anymore. Evaluate asym_fits_capacity() for prev_cpu before using that
> > shortcut.
> >
> > Fixes: 52262ee567ad ("sched/fair: Allow a per-CPU kthread waking a task to stack on the same CPU, to fix XFS performance regression")
> 
> Shouldn't that rather be
> 
>   b4c9c9f15649 ("sched/fair: Prefer prev cpu in asymmetric wakeup path")

Yes definitely, my bad!

> 
> ? This is an ulterior commit to the one you point to, and before then
> asymmetric CPU systems wouldn't use any of the sis() heuristics.
> 
> I reportedly reviewed said commit back then, and don't recall anything
> specific about that conditional... The cover-letter for v2 states:
> 
>   https://lore.kernel.org/lkml/20201028174412.680-1-vincent.guittot@linaro.org/
>   """
>   don't check capacity for the per-cpu kthread UC because the assumption is
>   that the wakee queued work for the per-cpu kthread that is now complete and
>   the task was already on this cpu.
>   """
> 
> So the assumption here is that current is gonna sleep right after waking up
> p, so current's utilization doesn't matter, and p was already on prev, so
> it should fit there...

I don't think the assumption that "p was already on prev should fit" is
correct if we take into account uclamp min. That value can change from one
activation to the other and make that task artificially too big for prev_cpu...

> 
> I'm thinking things should actually be OK with your other patch that
> excludes 'current == swapper' from this condition.

...But indeed if we add [1] to the equation, this patch here would only
protect against that specific corner case.

(And probably also against the fact that this same task could have a value
that doesn't fit this CPU anymore but didn't trigger misfit during its previous
activation?)

[1] https://lore.kernel.org/lkml/20211124154239.3191366-1-vincent.donnefort@arm.com/

> 
> > Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 6291876a9d32..b90dc6fd86ca 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -6410,7 +6410,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
> >        */
> >       if (is_per_cpu_kthread(current) &&
> >           prev == smp_processor_id() &&
> > -	    this_rq()->nr_running <= 1) {
> > +	    this_rq()->nr_running <= 1 &&
> > +	    asym_fits_capacity(task_util, prev)) {
> >               return prev;
> >       }
> >
> > --
> > 2.25.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sched/fair: Fix per-CPU kthread and wakee stacking for asym CPU capacity
  2021-11-24 17:58   ` Vincent Donnefort
@ 2021-11-24 18:04     ` Valentin Schneider
  0 siblings, 0 replies; 5+ messages in thread
From: Valentin Schneider @ 2021-11-24 18:04 UTC (permalink / raw)
  To: Vincent Donnefort
  Cc: peterz, mingo, vincent.guittot, linux-kernel, mgorman, dietmar.eggemann

On 24/11/21 17:58, Vincent Donnefort wrote:
> On Wed, Nov 24, 2021 at 05:11:32PM +0000, Valentin Schneider wrote:
>> On 24/11/21 14:14, Vincent Donnefort wrote:
>> > A shortcut has been introduced in select_idle_sibling() to return prev_cpu
>> > if the wakee is woken up by a per-CPU kthread. This is an issue for
>> > asymmetric CPU capacity systems where the wakee might not fit prev_cpu
>> > anymore. Evaluate asym_fits_capacity() for prev_cpu before using that
>> > shortcut.
>> >
>> > Fixes: 52262ee567ad ("sched/fair: Allow a per-CPU kthread waking a task to stack on the same CPU, to fix XFS performance regression")
>>
>> Shouldn't that rather be
>>
>>   b4c9c9f15649 ("sched/fair: Prefer prev cpu in asymmetric wakeup path")
>
> Yes definitely, my bad!
>
>>
>> ? This is an ulterior commit to the one you point to, and before then
>> asymmetric CPU systems wouldn't use any of the sis() heuristics.
>>
>> I reportedly reviewed said commit back then, and don't recall anything
>> specific about that conditional... The cover-letter for v2 states:
>>
>>   https://lore.kernel.org/lkml/20201028174412.680-1-vincent.guittot@linaro.org/
>>   """
>>   don't check capacity for the per-cpu kthread UC because the assumption is
>>   that the wakee queued work for the per-cpu kthread that is now complete and
>>   the task was already on this cpu.
>>   """
>>
>> So the assumption here is that current is gonna sleep right after waking up
>> p, so current's utilization doesn't matter, and p was already on prev, so
>> it should fit there...
>
> I don't think the assumption that "p was already on prev should fit" is
> correct if we take into account uclamp min. That value can change from one
> activation to the other and make that task artificially too big for prev_cpu...
>

Humph, good point, hadn't thought of that.

>>
>> I'm thinking things should actually be OK with your other patch that
>> excludes 'current == swapper' from this condition.
>
> ...But indeed if we add [1] to the equation, this patch here would only
> protect against that specific corner case.
>
> (And probably also against the fact that this same task could have a value
> that doesn't fit this CPU anymore but didn't trigger misfit during its previous
> activation?)

That would imply crossing the misfit threshold right at the dequeue signal
update, but that can happen.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sched/fair: Fix per-CPU kthread and wakee stacking for asym CPU capacity
  2021-11-24 14:14 [PATCH] sched/fair: Fix per-CPU kthread and wakee stacking for asym CPU capacity Vincent Donnefort
  2021-11-24 17:11 ` Valentin Schneider
@ 2021-11-25  9:10 ` Vincent Guittot
  1 sibling, 0 replies; 5+ messages in thread
From: Vincent Guittot @ 2021-11-25  9:10 UTC (permalink / raw)
  To: Vincent Donnefort
  Cc: peterz, mingo, linux-kernel, mgorman, dietmar.eggemann,
	Valentin.Schneider

On Wed, 24 Nov 2021 at 15:15, Vincent Donnefort
<vincent.donnefort@arm.com> wrote:
>
> A shortcut has been introduced in select_idle_sibling() to return prev_cpu
> if the wakee is woken up by a per-CPU kthread. This is an issue for
> asymmetric CPU capacity systems where the wakee might not fit prev_cpu
> anymore. Evaluate asym_fits_capacity() for prev_cpu before using that
> shortcut.
>
> Fixes: 52262ee567ad ("sched/fair: Allow a per-CPU kthread waking a task to stack on the same CPU, to fix XFS performance regression")
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 6291876a9d32..b90dc6fd86ca 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6410,7 +6410,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>          */
>         if (is_per_cpu_kthread(current) &&
>             prev == smp_processor_id() &&
> -           this_rq()->nr_running <= 1) {
> +           this_rq()->nr_running <= 1 &&
> +           asym_fits_capacity(task_util, prev)) {

With this patch, i don't think that
20211124154239.3191366-1-vincent.donnefort@arm.com is needed anymore

>                 return prev;
>         }
>
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-11-25  9:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-24 14:14 [PATCH] sched/fair: Fix per-CPU kthread and wakee stacking for asym CPU capacity Vincent Donnefort
2021-11-24 17:11 ` Valentin Schneider
2021-11-24 17:58   ` Vincent Donnefort
2021-11-24 18:04     ` Valentin Schneider
2021-11-25  9:10 ` Vincent Guittot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).