sched: Cleanup idle task double initialization
Subsystems (2)
cover sched: Cleanup idle task double initialization
Idle tasks are initialized via __sched_fork() twice, which can be trivially avoided as pointed out here:
https://lore.kernel.org/all/8734p4ymqj.ffs@tglx
As this got ignored, the tasteless hack in init_scx_entity() to work around it stayed and found it's way into mainline.
Mop it up after the fact to not further proliferate technical debt.
Thanks,
tglx
--- core.c | 13 ++++--------- ext.c | 7 +------ 2 files changed, 5 insertions(+), 15 deletions(-)
PATCH 1/2 sched: Initialize idle tasks only once
Idle tasks are initialized via __sched_fork() twice:
fork_idle()
copy_process()
sched_fork()
__sched_fork()
init_idle()
__sched_fork()Instead of cleaning this up, sched_ext hacked around it. Even when analyis and solution were provided in a discussion, nobody cared to clean this up.
init_idle() is also invoked from sched_init() to initialize the boot CPU's idle task, which requires the __sched_fork() invocation. But this can be trivially solved by invoking __sched_fork() before init_idle() in sched_init() and removing the __sched_fork() invocation from init_idle().
Do so and clean up the comments explaining this historical leftover.
--- kernel/sched/core.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
kernel/sched/core.c +5 -7
@@ -4424,7 +4424,8 @@ int wake_up_state(struct task_struct *p,* Perform scheduler related setup for a newly forked process p. * p is forked by current. * -* __sched_fork() is basic setup used by init_idle() too: +* __sched_fork() is basic setup which is also used by sched_init() to +* initialize the boot CPU's idle task. */ static void __sched_fork (unsigned long clone_flags,struct task_struct *p) {
@@ -7680,8 +7681,6 @@ void __init init_idle(struct task_structstruct rq *rq =cpu_rq (cpu);unsigned long flags; -__sched_fork (0 , idle); -raw_spin_lock_irqsave (&idle->pi_lock, flags);raw_spin_rq_lock (rq);
@@ -7696,10 +7695,8 @@ void __init init_idle(struct task_struct#ifdef CONFIG_SMP /* -* It's possible that init_idle() gets called multiple times on a task, -* in that case do_set_cpus_allowed() will not do the right thing. -* -* And since this is boot we can forgo the serialization. +* No validation and serialization required at boot time and for +* setting up the idle tasks of not yet online CPUs. */ set_cpus_allowed_common (idle, &ac);#endif
@@ -8543,6 +8540,7 @@ void __init sched_init(void)* but because we are the idle thread, we just pick up running again * when this runqueue becomes "idle". */ +__sched_fork (0 , current);init_idle (current,smp_processor_id ()); calc_load_update = jiffies +LOAD_FREQ ;
[tip: sched/core] sched: Initialize idle tasks only once
The following commit has been merged into the sched/core branch of tip:
The following commit has been merged into the sched/core branch of tip:
Commit-ID: b23decf8ac9102fc52c4de5196f4dc0a5f3eb80b Gitweb: https://git.kernel.org/tip/b23decf8ac9102fc52c4de5196f4dc0a5f3eb80b Author: Thomas Gleixner <[email protected]> AuthorDate: Mon, 28 Oct 2024 11:43:42 +01:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Tue, 05 Nov 2024 12:55:37 +01:00
sched: Initialize idle tasks only once
Idle tasks are initialized via __sched_fork() twice:
fork_idle()
copy_process()
sched_fork()
__sched_fork()
init_idle()
__sched_fork()Instead of cleaning this up, sched_ext hacked around it. Even when analyis and solution were provided in a discussion, nobody cared to clean this up.
init_idle() is also invoked from sched_init() to initialize the boot CPU's idle task, which requires the __sched_fork() invocation. But this can be trivially solved by invoking __sched_fork() before init_idle() in sched_init() and removing the __sched_fork() invocation from init_idle().
Do so and clean up the comments explaining this historical leftover.
--- kernel/sched/core.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
kernel/sched/core.c +5 -7
@@ -4423,7 +4423,8 @@ int wake_up_state(struct task_struct *p, unsigned int state)* Perform scheduler related setup for a newly forked process p. * p is forked by current. * -* __sched_fork() is basic setup used by init_idle() too: +* __sched_fork() is basic setup which is also used by sched_init() to +* initialize the boot CPU's idle task. */ static void __sched_fork (unsigned long clone_flags,struct task_struct *p) {
@@ -7697,8 +7698,6 @@ void __init init_idle(struct task_struct *idle, int cpu)struct rq *rq =cpu_rq (cpu);unsigned long flags; -__sched_fork (0 , idle); -raw_spin_lock_irqsave (&idle->pi_lock, flags);raw_spin_rq_lock (rq);
@@ -7713,10 +7712,8 @@ void __init init_idle(struct task_struct *idle, int cpu)#ifdef CONFIG_SMP /* -* It's possible that init_idle() gets called multiple times on a task, -* in that case do_set_cpus_allowed() will not do the right thing. -* -* And since this is boot we can forgo the serialization. +* No validation and serialization required at boot time and for +* setting up the idle tasks of not yet online CPUs. */ set_cpus_allowed_common (idle, &ac);#endif
@@ -8561,6 +8558,7 @@ void __init sched_init(void)* but because we are the idle thread, we just pick up running again * when this runqueue becomes "idle". */ +__sched_fork (0 , current);init_idle (current,smp_processor_id ()); calc_load_update = jiffies +LOAD_FREQ ;
PATCH 2/2 sched/ext: Remove sched_fork() hack
In reply to Thomas GleixnerInstead of solving the underlying problem of the double invocation of __sched_fork() for idle tasks, sched-ext decided to hack around the issue by partially clearing out the entity struct to preserve the already enqueued node. A provided analysis and solution has been ignored for four months.
Now that someone else has taken care of cleaning it up, remove the disgusting hack and clear out the full structure.
--- kernel/sched/ext.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
kernel/sched/ext.c +1 -6
@@ -3548,12 +3548,7 @@ static void scx_ops_exit_task(struct tasvoid init_scx_entity (struct sched_ext_entity *scx) { -/* -* init_idle() calls this function again after fork sequence is -* complete. Don't touch ->tasks_node as it's already linked. -*/ -memset (scx,0 , offsetof(struct sched_ext_entity , tasks_node)); - +memset (scx,0 ,sizeof (*scx));INIT_LIST_HEAD (&scx->dsq_list.node);RB_CLEAR_NODE (&scx->dsq_priq); scx->sticky_cpu =-1 ;
Re: [patch 2/2] sched/ext: Remove sched_fork() hack
Quoted 32 lines · Thomas Gleixner
Instead of solving the underlying problem of the double invocation of
__sched_fork()for idle tasks, sched-ext decided to hack around the issue by partially clearing out the entity struct to preserve the already enqueued node. A provided analysis and solution has been ignored for four months.Now that someone else has taken care of cleaning it up, remove the disgusting hack and clear out the full structure.
Fixes f0e1a0643a59 ("sched_ext: Implement BPF extensible scheduler class")Signed-off-by Thomas Gleixner <[email protected]>Cc Tejun Heo <[email protected]>Cc David Vernet <[email protected]>Cc Ingo Molnar <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Juri Lelli <[email protected]>Cc Vincent Guittot <[email protected]>Cc Dietmar Eggemann <[email protected]>Cc Steven Rostedt <[email protected]>Cc Ben Segall <[email protected]>Cc Mel Gorman <[email protected]>Cc Valentin Schneider <[email protected]>--- kernel/sched/ext.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)kernel/sched/ext.c +0
-0@@ -3548,12 +3548,7 @@ static void scx_ops_exit_task(struct tasvoid init_scx_entity (struct sched_ext_entity *scx) {
kernel/sched/ext.c static void scx_ops_exit_task(struct tas · line 3554
Quoted 10 lines
kernel/sched/ext.c +1
-6-/* -* init_idle() calls this function again after fork sequence is -* complete. Don't touch ->tasks_node as it's already linked. -*/ -memset (scx,0 , offsetof(struct sched_ext_entity , tasks_node)); - +memset (scx,0 ,sizeof (*scx));INIT_LIST_HEAD (&scx->dsq_list.node);RB_CLEAR_NODE (&scx->dsq_priq); scx->sticky_cpu =-1 ;
Should the "must be the last" comment in include/linux/sched/ext.h also be removed?
Rasmus
Re: [patch 2/2] sched/ext: Remove sched_fork() hack
Quoted 7 lines · Rasmus Villemoes
kernel/sched/ext.c static void scx_ops_exit_task(struct tas · line 3554
Quoted 4 lines
kernel/sched/ext.c +1
-0+memset (scx,0 ,sizeof (*scx));INIT_LIST_HEAD (&scx->dsq_list.node);RB_CLEAR_NODE (&scx->dsq_priq); scx->sticky_cpu =-1 ;Should the "must be the last" comment in
include/linux/sched/ext.halso be removed?
Oh. Indeed. I missed that one.
Thanks for pointing it out.
tglx
PATCH 2/2 sched/ext: Remove sched_fork() hack
In reply to Thomas GleixnerInstead of solving the underlying problem of the double invocation of __sched_fork() for idle tasks, sched-ext decided to hack around the issue by partially clearing out the entity struct to preserve the already enqueued node. A provided analysis and solution has been ignored for four months.
Now that someone else has taken care of cleaning it up, remove the disgusting hack and clear out the full structure. Remove the comment in the structure declaration as well, as there is no requirement for @node being the last element anymore.
---
V2: Remove the comment in ext.h (Rasmus)
--- include/linux/sched/ext.h | 1 - kernel/sched/ext.c | 7 +------ 2 files changed, 1 insertion(+), 7 deletions(-)
include/linux/sched/ext.h +0 -1
@@ -199,7 +199,6 @@ struct sched_ext_entity {#ifdef CONFIG_EXT_GROUP_SCHED struct cgroup *cgrp_moving_from;#endif -/* must be the last field, see init_scx_entity() */ struct list_head tasks_node; };
kernel/sched/ext.c +1 -6
@@ -3548,12 +3548,7 @@ static void scx_ops_exit_task(struct tasvoid init_scx_entity (struct sched_ext_entity *scx) { -/* -* init_idle() calls this function again after fork sequence is -* complete. Don't touch ->tasks_node as it's already linked. -*/ -memset (scx,0 , offsetof(struct sched_ext_entity , tasks_node)); - +memset (scx,0 ,sizeof (*scx));INIT_LIST_HEAD (&scx->dsq_list.node);RB_CLEAR_NODE (&scx->dsq_priq); scx->sticky_cpu =-1 ;
Re: [patch v1A 2/2] sched/ext: Remove sched_fork() hack
Quoted 11 lines · Thomas Gleixner
Instead of solving the underlying problem of the double invocation of
__sched_fork()for idle tasks, sched-ext decided to hack around the issue by partially clearing out the entity struct to preserve the already enqueued node. A provided analysis and solution has been ignored for four months.Now that someone else has taken care of cleaning it up, remove the disgusting hack and clear out the full structure. Remove the comment in the structure declaration as well, as there is no requirement for @node being the last element anymore.
Quoted 13 lines
Fixes f0e1a0643a59 ("sched_ext: Implement BPF extensible scheduler class")Signed-off-by Thomas Gleixner <[email protected]>Cc Tejun Heo <[email protected]>Cc David Vernet <[email protected]>Cc Ingo Molnar <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Juri Lelli <[email protected]>Cc Vincent Guittot <[email protected]>Cc Dietmar Eggemann <[email protected]>Cc Steven Rostedt <[email protected]>Cc Ben Segall <[email protected]>Cc Mel Gorman <[email protected]>Cc Valentin Schneider <[email protected]>
Thanks.
Signature · 1 line
tejun
[tip: sched/core] sched/ext: Remove sched_fork() hack
In reply to Thomas GleixnerThe following commit has been merged into the sched/core branch of tip:
The following commit has been merged into the sched/core branch of tip:
Commit-ID: 0f0d1b8e5010bfe1feeb4d78d137e41946a5370d Gitweb: https://git.kernel.org/tip/0f0d1b8e5010bfe1feeb4d78d137e41946a5370d Author: Thomas Gleixner <[email protected]> AuthorDate: Mon, 28 Oct 2024 14:20:35 +01:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Tue, 05 Nov 2024 12:55:37 +01:00
sched/ext: Remove sched_fork() hack
Instead of solving the underlying problem of the double invocation of __sched_fork() for idle tasks, sched-ext decided to hack around the issue by partially clearing out the entity struct to preserve the already enqueued node. A provided analysis and solution has been ignored for four months.
Now that someone else has taken care of cleaning it up, remove the disgusting hack and clear out the full structure. Remove the comment in the structure declaration as well, as there is no requirement for @node being the last element anymore.
--- include/linux/sched/ext.h | 1 - kernel/sched/ext.c | 7 +------ 2 files changed, 1 insertion(+), 7 deletions(-)
include/linux/sched/ext.h +0 -1
@@ -199,7 +199,6 @@ struct sched_ext_entity {#ifdef CONFIG_EXT_GROUP_SCHED struct cgroup *cgrp_moving_from;#endif -/* must be the last field, see init_scx_entity() */ struct list_head tasks_node; };
kernel/sched/ext.c +1 -6
@@ -3548,12 +3548,7 @@ static void scx_ops_exit_task(struct task_struct *p)void init_scx_entity (struct sched_ext_entity *scx) { -/* -* init_idle() calls this function again after fork sequence is -* complete. Don't touch ->tasks_node as it's already linked. -*/ -memset (scx,0 , offsetof(struct sched_ext_entity , tasks_node)); - +memset (scx,0 ,sizeof (*scx));INIT_LIST_HEAD (&scx->dsq_list.node);RB_CLEAR_NODE (&scx->dsq_priq); scx->sticky_cpu =-1 ;