sched: Cleanup idle task double initialization

[patch 2/2] sched/ext: Remove sched_fork() hack

Posted by Thomas Gleixner 3 weeks, 6 days ago

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 <tglx@linutronix.de>
Cc: Tejun Heo <tj@kernel.org>
Cc: David Vernet <void@manifault.com>
Cc: Ingo Molnar <mingo@kernel.org.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Valentin Schneider <vschneid@redhat.com>
---
 kernel/sched/ext.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

--- a/kernel/sched/ext.c
+++ b/kernel/sched/ext.c
@@ -3548,12 +3548,7 @@ static void scx_ops_exit_task(struct tas
 
 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;

Re: [patch 2/2] sched/ext: Remove sched_fork() hack

Posted by Rasmus Villemoes 3 weeks, 6 days ago

On Mon, Oct 28 2024, Thomas Gleixner <tglx@linutronix.de> wrote:

> 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 <tglx@linutronix.de>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: David Vernet <void@manifault.com>
> Cc: Ingo Molnar <mingo@kernel.org.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ben Segall <bsegall@google.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Valentin Schneider <vschneid@redhat.com>
> ---
>  kernel/sched/ext.c |    7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> --- a/kernel/sched/ext.c
> +++ b/kernel/sched/ext.c
> @@ -3548,12 +3548,7 @@ static void scx_ops_exit_task(struct tas
>  
>  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;

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

Posted by Thomas Gleixner 3 weeks, 6 days ago

On Mon, Oct 28 2024 at 13:30, Rasmus Villemoes wrote:
>> +	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?

Oh. Indeed. I missed that one.

Thanks for pointing it out.

       tglx

[patch v1A 2/2] sched/ext: Remove sched_fork() hack

Posted by Thomas Gleixner 3 weeks, 6 days ago

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.

Fixes: f0e1a0643a59 ("sched_ext: Implement BPF extensible scheduler class")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tejun Heo <tj@kernel.org>
Cc: David Vernet <void@manifault.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Valentin Schneider <vschneid@redhat.com>
---
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(-)

--- a/include/linux/sched/ext.h
+++ b/include/linux/sched/ext.h
@@ -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;
 };
 
--- a/kernel/sched/ext.c
+++ b/kernel/sched/ext.c
@@ -3548,12 +3548,7 @@ static void scx_ops_exit_task(struct tas
 
 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;

Re: [patch v1A 2/2] sched/ext: Remove sched_fork() hack

Posted by Tejun Heo 3 weeks, 6 days ago

On Mon, Oct 28, 2024 at 02:20:35PM +0100, Thomas Gleixner wrote:
> 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.
> 
> Fixes: f0e1a0643a59 ("sched_ext: Implement BPF extensible scheduler class")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: David Vernet <void@manifault.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ben Segall <bsegall@google.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Valentin Schneider <vschneid@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[tip: sched/core] sched/ext: Remove sched_fork() hack

Posted by tip-bot2 for Thomas Gleixner 2 weeks, 4 days ago

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 <tglx@linutronix.de>
AuthorDate:    Mon, 28 Oct 2024 14:20:35 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
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.

Fixes: f0e1a0643a59 ("sched_ext: Implement BPF extensible scheduler class")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Tejun Heo <tj@kernel.org>
Link: https://lore.kernel.org/r/87ldy82wkc.ffs@tglx
---
 include/linux/sched/ext.h | 1 -
 kernel/sched/ext.c        | 7 +------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/linux/sched/ext.h b/include/linux/sched/ext.h
index 1ddbde6..2799e72 100644
--- a/include/linux/sched/ext.h
+++ b/include/linux/sched/ext.h
@@ -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;
 };
 
diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
index 5900b06..f6e9a14 100644
--- a/kernel/sched/ext.c
+++ b/kernel/sched/ext.c
@@ -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;