* [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task
@ 2021-12-01 14:34 Vincent Donnefort
2021-12-03 8:34 ` Vincent Guittot
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Vincent Donnefort @ 2021-12-01 14:34 UTC (permalink / raw)
To: peterz, mingo, vincent.guittot
Cc: linux-kernel, mgorman, dietmar.eggemann, valentin.schneider,
Vincent Donnefort
select_idle_sibling() has a special case for tasks woken up by a per-CPU
kthread, where the selected CPU is the previous one. However, the current
condition for this exit path is incomplete. A task can wake up from an
interrupt context (e.g. hrtimer), while a per-CPU kthread is running. A
such scenario would spuriously trigger the special case described above.
Also, a recent change made the idle task like a regular per-CPU kthread,
hence making that situation more likely to happen
(is_per_cpu_kthread(swapper) being true now).
Checking for task context makes sure select_idle_sibling() will not
interpret a wake up from any other context as a wake up by a per-CPU
kthread.
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>
---
v1 -> v2:
* is_idle_thread() -> in_task() to also include spurious detection when
current != swapper. (Vincent Guittot)
---
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 945d987246c5..56db4ae85995 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6399,6 +6399,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
* pattern is IO completions.
*/
if (is_per_cpu_kthread(current) &&
+ in_task() &&
prev == smp_processor_id() &&
this_rq()->nr_running <= 1) {
return prev;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task
2021-12-01 14:34 [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task Vincent Donnefort
@ 2021-12-03 8:34 ` Vincent Guittot
2021-12-03 12:35 ` Valentin Schneider
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Vincent Guittot @ 2021-12-03 8:34 UTC (permalink / raw)
To: Vincent Donnefort
Cc: peterz, mingo, linux-kernel, mgorman, dietmar.eggemann,
Valentin.Schneider
On Wed, 1 Dec 2021 at 15:35, Vincent Donnefort
<vincent.donnefort@arm.com> wrote:
>
> select_idle_sibling() has a special case for tasks woken up by a per-CPU
> kthread, where the selected CPU is the previous one. However, the current
> condition for this exit path is incomplete. A task can wake up from an
> interrupt context (e.g. hrtimer), while a per-CPU kthread is running. A
> such scenario would spuriously trigger the special case described above.
> Also, a recent change made the idle task like a regular per-CPU kthread,
> hence making that situation more likely to happen
> (is_per_cpu_kthread(swapper) being true now).
>
> Checking for task context makes sure select_idle_sibling() will not
> interpret a wake up from any other context as a wake up by a per-CPU
> kthread.
>
> 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>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
>
> ---
> v1 -> v2:
> * is_idle_thread() -> in_task() to also include spurious detection when
> current != swapper. (Vincent Guittot)
> ---
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 945d987246c5..56db4ae85995 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6399,6 +6399,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
> * pattern is IO completions.
> */
> if (is_per_cpu_kthread(current) &&
> + in_task() &&
> prev == smp_processor_id() &&
> this_rq()->nr_running <= 1) {
> return prev;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task
2021-12-01 14:34 [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task Vincent Donnefort
2021-12-03 8:34 ` Vincent Guittot
@ 2021-12-03 12:35 ` Valentin Schneider
2021-12-04 9:53 ` Peter Zijlstra
2021-12-06 15:22 ` [tip: sched/core] " tip-bot2 for Vincent Donnefort
3 siblings, 0 replies; 6+ messages in thread
From: Valentin Schneider @ 2021-12-03 12:35 UTC (permalink / raw)
To: Vincent Donnefort, peterz, mingo, vincent.guittot
Cc: linux-kernel, mgorman, dietmar.eggemann, Vincent Donnefort
On 01/12/21 14:34, Vincent Donnefort wrote:
> select_idle_sibling() has a special case for tasks woken up by a per-CPU
> kthread, where the selected CPU is the previous one. However, the current
> condition for this exit path is incomplete. A task can wake up from an
> interrupt context (e.g. hrtimer), while a per-CPU kthread is running. A
> such scenario would spuriously trigger the special case described above.
> Also, a recent change made the idle task like a regular per-CPU kthread,
> hence making that situation more likely to happen
> (is_per_cpu_kthread(swapper) being true now).
>
> Checking for task context makes sure select_idle_sibling() will not
> interpret a wake up from any other context as a wake up by a per-CPU
> kthread.
>
> 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>
>
Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
> ---
> v1 -> v2:
> * is_idle_thread() -> in_task() to also include spurious detection when
> current != swapper. (Vincent Guittot)
> ---
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 945d987246c5..56db4ae85995 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6399,6 +6399,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
> * pattern is IO completions.
> */
> if (is_per_cpu_kthread(current) &&
> + in_task() &&
> prev == smp_processor_id() &&
> this_rq()->nr_running <= 1) {
> return prev;
> --
> 2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task
2021-12-01 14:34 [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task Vincent Donnefort
2021-12-03 8:34 ` Vincent Guittot
2021-12-03 12:35 ` Valentin Schneider
@ 2021-12-04 9:53 ` Peter Zijlstra
2021-12-06 9:57 ` Vincent Donnefort
2021-12-06 15:22 ` [tip: sched/core] " tip-bot2 for Vincent Donnefort
3 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2021-12-04 9:53 UTC (permalink / raw)
To: Vincent Donnefort
Cc: mingo, vincent.guittot, linux-kernel, mgorman, dietmar.eggemann,
valentin.schneider
On Wed, Dec 01, 2021 at 02:34:50PM +0000, Vincent Donnefort wrote:
> select_idle_sibling() has a special case for tasks woken up by a per-CPU
> kthread, where the selected CPU is the previous one. However, the current
> condition for this exit path is incomplete. A task can wake up from an
> interrupt context (e.g. hrtimer), while a per-CPU kthread is running. A
> such scenario would spuriously trigger the special case described above.
> Also, a recent change made the idle task like a regular per-CPU kthread,
> hence making that situation more likely to happen
> (is_per_cpu_kthread(swapper) being true now).
>
> Checking for task context makes sure select_idle_sibling() will not
> interpret a wake up from any other context as a wake up by a per-CPU
> kthread.
>
> 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 945d987246c5..56db4ae85995 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6399,6 +6399,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
> * pattern is IO completions.
> */
> if (is_per_cpu_kthread(current) &&
> + in_task() &&
> prev == smp_processor_id() &&
> this_rq()->nr_running <= 1) {
> return prev;
Hurmph, so now I have two 'trivial' patches from you that touch this
same function and they's conflicting. I've fixed it up, but perhaps it
would've been nice to have them combined in a series or somesuch :-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task
2021-12-04 9:53 ` Peter Zijlstra
@ 2021-12-06 9:57 ` Vincent Donnefort
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Donnefort @ 2021-12-06 9:57 UTC (permalink / raw)
To: Peter Zijlstra
Cc: mingo, vincent.guittot, linux-kernel, mgorman, dietmar.eggemann,
valentin.schneider
On Sat, Dec 04, 2021 at 10:53:16AM +0100, Peter Zijlstra wrote:
> On Wed, Dec 01, 2021 at 02:34:50PM +0000, Vincent Donnefort wrote:
> > select_idle_sibling() has a special case for tasks woken up by a per-CPU
> > kthread, where the selected CPU is the previous one. However, the current
> > condition for this exit path is incomplete. A task can wake up from an
> > interrupt context (e.g. hrtimer), while a per-CPU kthread is running. A
> > such scenario would spuriously trigger the special case described above.
> > Also, a recent change made the idle task like a regular per-CPU kthread,
> > hence making that situation more likely to happen
> > (is_per_cpu_kthread(swapper) being true now).
> >
> > Checking for task context makes sure select_idle_sibling() will not
> > interpret a wake up from any other context as a wake up by a per-CPU
> > kthread.
> >
> > 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 945d987246c5..56db4ae85995 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -6399,6 +6399,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
> > * pattern is IO completions.
> > */
> > if (is_per_cpu_kthread(current) &&
> > + in_task() &&
> > prev == smp_processor_id() &&
> > this_rq()->nr_running <= 1) {
> > return prev;
>
> Hurmph, so now I have two 'trivial' patches from you that touch this
> same function and they's conflicting. I've fixed it up, but perhaps it
> would've been nice to have them combined in a series or somesuch :-)
>
I definitely should have created a single patchset. Apologies for the
extra work and thanks for taking those two patches!
On another subject, in case you missed them, I also have two tiny fixes,
reviewed by Vincent:
[PATCH v2 1/2] sched/fair: Fix asym_fits_capacity() task_util type
[PATCH v2 2/2] sched/fair: Fix task_fits_capacity() capacity type
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: sched/core] sched/fair: Fix detection of per-CPU kthreads waking a task
2021-12-01 14:34 [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task Vincent Donnefort
` (2 preceding siblings ...)
2021-12-04 9:53 ` Peter Zijlstra
@ 2021-12-06 15:22 ` tip-bot2 for Vincent Donnefort
3 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Vincent Donnefort @ 2021-12-06 15:22 UTC (permalink / raw)
To: linux-tip-commits
Cc: Vincent Donnefort, Peter Zijlstra (Intel),
Vincent Guittot, Valentin Schneider, x86, linux-kernel
The following commit has been merged into the sched/core branch of tip:
Commit-ID: 8b4e74ccb582797f6f0b0a50372ebd9fd2372a27
Gitweb: https://git.kernel.org/tip/8b4e74ccb582797f6f0b0a50372ebd9fd2372a27
Author: Vincent Donnefort <vincent.donnefort@arm.com>
AuthorDate: Wed, 01 Dec 2021 14:34:50
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Sat, 04 Dec 2021 10:56:20 +01:00
sched/fair: Fix detection of per-CPU kthreads waking a task
select_idle_sibling() has a special case for tasks woken up by a per-CPU
kthread, where the selected CPU is the previous one. However, the current
condition for this exit path is incomplete. A task can wake up from an
interrupt context (e.g. hrtimer), while a per-CPU kthread is running. A
such scenario would spuriously trigger the special case described above.
Also, a recent change made the idle task like a regular per-CPU kthread,
hence making that situation more likely to happen
(is_per_cpu_kthread(swapper) being true now).
Checking for task context makes sure select_idle_sibling() will not
interpret a wake up from any other context as a wake up by a per-CPU
kthread.
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>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Link: https://lore.kernel.org/r/20211201143450.479472-1-vincent.donnefort@arm.com
---
kernel/sched/fair.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 884f29d..5cd2798 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6398,6 +6398,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
* pattern is IO completions.
*/
if (is_per_cpu_kthread(current) &&
+ in_task() &&
prev == smp_processor_id() &&
this_rq()->nr_running <= 1) {
return prev;
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-06 15:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01 14:34 [PATCH v2] sched/fair: Fix detection of per-CPU kthreads waking a task Vincent Donnefort
2021-12-03 8:34 ` Vincent Guittot
2021-12-03 12:35 ` Valentin Schneider
2021-12-04 9:53 ` Peter Zijlstra
2021-12-06 9:57 ` Vincent Donnefort
2021-12-06 15:22 ` [tip: sched/core] " tip-bot2 for Vincent Donnefort
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.