[PATCH] sched/smt: Call sched_core_cpu_deactivate() after error is handled

[PATCH] sched/smt: Call sched_core_cpu_deactivate() after error is handled

[Andrea Ciprietti]

In sched_cpu_deactivate(), the error path restores most of the initial
state before returning, but, if CONFIG_SCHED_SMT is defined, it does not
undo the previous call to sched_core_cpu_deactivate().

There is no easy way to invert such function since it overrides some shared
state of the leader CPU. Instead, simply move the call past the error
handling code. This is fine since cpuset_cpu_inactive() performs unrelated
operations.

Note that there's no need for #ifdef since cpuset_core_* have empty
definitions when CONFIG_SCHED_SMT is not set (in which case this patch is a
no-op).

Fixes: 3c474b3239f1 ("sched: Fix Core-wide rq->lock for uninitialized CPUs")
Cc: stable@kernel.org
Suggested-by: Josh Don <joshdon@google.com>
Signed-off-by: Andrea Ciprietti <ciprietti@google.com>
---
 kernel/sched/core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 719e0ed1e976..4d55bc243ae5 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8187,10 +8187,6 @@ int sched_cpu_deactivate(unsigned int cpu)
 	 */
 	sched_smt_present_dec(cpu);
 
-#ifdef CONFIG_SCHED_SMT
-	sched_core_cpu_deactivate(cpu);
-#endif
-
 	if (!sched_smp_initialized)
 		return 0;
 
@@ -8204,6 +8200,7 @@ int sched_cpu_deactivate(unsigned int cpu)
 		sched_update_numa(cpu, true);
 		return ret;
 	}
+	sched_core_cpu_deactivate(cpu);
 	sched_domains_numa_masks_clear(cpu);
 	return 0;
 }
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH] sched/smt: Call sched_core_cpu_deactivate() after error is handled

[Valentin Schneider]

On 08/11/24 12:11, Andrea Ciprietti wrote:
> In sched_cpu_deactivate(), the error path restores most of the initial
> state before returning, but, if CONFIG_SCHED_SMT is defined, it does not
> undo the previous call to sched_core_cpu_deactivate().
>
> There is no easy way to invert such function since it overrides some shared
> state of the leader CPU. Instead, simply move the call past the error
> handling code. This is fine since cpuset_cpu_inactive() performs unrelated
> operations.
>
> Note that there's no need for #ifdef since cpuset_core_* have empty
> definitions when CONFIG_SCHED_SMT is not set (in which case this patch is a
> no-op).
>
> Fixes: 3c474b3239f1 ("sched: Fix Core-wide rq->lock for uninitialized CPUs")
> Cc: stable@kernel.org
> Suggested-by: Josh Don <joshdon@google.com>
> Signed-off-by: Andrea Ciprietti <ciprietti@google.com>
> ---
>  kernel/sched/core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 719e0ed1e976..4d55bc243ae5 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8187,10 +8187,6 @@ int sched_cpu_deactivate(unsigned int cpu)
>  	 */
>  	sched_smt_present_dec(cpu);
>  
> -#ifdef CONFIG_SCHED_SMT
> -	sched_core_cpu_deactivate(cpu);
> -#endif
> -
>  	if (!sched_smp_initialized)
>  		return 0;

                ^^^^^^^^
You're moving the call to the other side of this, however I don't recall
that hot-unplug can happen before sched_init_smp(). Built-in rcutorture
wouldn't start flipping CPUs until device_initcall's, which happens later
in kernel_init_freeable():

  kernel_init_freeable()
  `\
    smp_init() <- boots all secondaries
    sched_init_smp()
    do_basic_setup()
    `\
      do_initcalls()

So this looks okay, but needs a mention in the changelog IMO.

>
> @@ -8204,6 +8200,7 @@ int sched_cpu_deactivate(unsigned int cpu)
>  		sched_update_numa(cpu, true);
>  		return ret;
>  	}
> +	sched_core_cpu_deactivate(cpu);
>  	sched_domains_numa_masks_clear(cpu);
>  	return 0;
>  }
> -- 
> 2.47.0.277.g8800431eea-goog

[PATCH v2] sched/smt: Call sched_core_cpu_deactivate() after error is handled

[Andrea Ciprietti]

In sched_cpu_deactivate(), the error path restores most of the initial
state before returning, but, if CONFIG_SCHED_SMT is defined, it does not
undo the previous call to sched_core_cpu_deactivate().

There is no easy way to invert such function since it overrides some shared
state of the leader CPU. Instead, simply move the call past the error
handling code. This is fine since:

	- sched_cpu_deactivate() is CPU hot-unplug, which doesn't happen until
	  after sched_init_smp() in kernel_init_freeable():

		kernel_init_freeable()
		`\
		  smp_init() <- boots all secondaries
		  sched_init_smp()
		  do_basic_setup()
		  `\
			do_initcalls()

	- cpuset_cpu_inactive() performs unrelated operations.

Note that there's no need for #ifdef since cpuset_core_* have empty
definitions when CONFIG_SCHED_SMT is not set (in which case this patch is a
no-op).

Fixes: 3c474b3239f1 ("sched: Fix Core-wide rq->lock for uninitialized CPUs")
Cc: stable@kernel.org
Suggested-by: Josh Don <joshdon@google.com>
Signed-off-by: Andrea Ciprietti <ciprietti@google.com>
---
Changelog v1->v2:
	- Added details to commit message.
---
 kernel/sched/core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 719e0ed1e976..4d55bc243ae5 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8187,10 +8187,6 @@ int sched_cpu_deactivate(unsigned int cpu)
 	 */
 	sched_smt_present_dec(cpu);
 
-#ifdef CONFIG_SCHED_SMT
-	sched_core_cpu_deactivate(cpu);
-#endif
-
 	if (!sched_smp_initialized)
 		return 0;
 
@@ -8204,6 +8200,7 @@ int sched_cpu_deactivate(unsigned int cpu)
 		sched_update_numa(cpu, true);
 		return ret;
 	}
+	sched_core_cpu_deactivate(cpu);
 	sched_domains_numa_masks_clear(cpu);
 	return 0;
 }
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v2] sched/smt: Call sched_core_cpu_deactivate() after error is handled

[Andrea Ciprietti]

Quick follow-up on this patch: does the new version of the changelog 
reflect the change you suggested? If so, is it ready to be taken?

Thanks!