sched/debug: initialize sd_sysctl_cpus if !CONFIG_CPUMASK_OFFSTACK

PATCH In review Masayoshi Mizuma 4 participants 6 messages Jan 29, 2019 – Feb 4, 2019 Scheduler

lore

Share & tools

lore

Masayoshi Mizuma

author

2019-01-29 15:13 UTC #0 lore raw

PATCH 1/1 sched/debug: initialize sd_sysctl_cpus if !CONFIG_CPUMASK_OFFSTACK

From: Hidetoshi Seto <[email protected]>

register_sched_domain_sysctl() copies the cpu_possible_mask into sd_sysctl_cpus, but only if sd_sysctl_cpus hasn't already been allocated (ie, CONFIG_CPUMASK_OFFSTACK is set). However, when CONFIG_CPUMASK_OFFSTACK is not set, sd_sysctl_cpus is left uninitialized (all zeroes) and the kernel may fail to initialize sched_domain sysctl entries for all possible cpus.

This is visible to the user if the kernel is booted with maxcpus=n, or if ACPI tables have been modified to leave cpus offline, and then checking for missing /proc/sys/kernel/sched_domain/cpu* entries.

Fix this by separating the allocataion and initialization, and adding a flag to initialize the possible cpu entries while system booting only.

Signed-off-by Hidetoshi Seto <[email protected]>

Reviewed-by Masayoshi Mizuma <[email protected]>

Tested-by Syuuichirou Ishii <[email protected]>

Tested-by Tarumizu, Kohei <[email protected]>

---
 kernel/sched/debug.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sched/debug.c +4 -0

index de3de997e245..9c6637f3e21d 100644
@@ -310,6 +310,7 @@ static struct ctl_table *sd_alloc_ctl_cpu_table(int cpu)
 
 static cpumask_var_t		sd_sysctl_cpus;
 static struct ctl_table_header	*sd_sysctl_header;
+static int register_sched_domain_sysctl_on_boot = 1;
 
 void register_sched_domain_sysctl(void)
 {

@@ -344,9 +345,12 @@ void register_sched_domain_sysctl(void)
 	if (!cpumask_available(sd_sysctl_cpus)) {
 		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
 			return;
+	}
 
+	if (register_sched_domain_sysctl_on_boot) {
 		/* init to possible to not have holes in @cpu_entries */
 		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
+		register_sched_domain_sysctl_on_boot = 0;
 	}
 
 	for_each_cpu(i, sd_sysctl_cpus) {

Signature · 1 line

2.20.1

Joe Lawrence

Acked-by on v1 1/1

2019-01-29 16:16 UTC #1 lore raw

Quoted 9 lines · Masayoshi Mizuma

From: Hidetoshi Seto <[email protected]>
register_sched_domain_sysctl() copies the cpu_possible_mask into sd_sysctl_cpus, but only if sd_sysctl_cpus hasn't already been allocated (ie, CONFIG_CPUMASK_OFFSTACK is set). However, when CONFIG_CPUMASK_OFFSTACK is not set, sd_sysctl_cpus is left uninitialized (all zeroes) and the kernel may fail to initialize sched_domain sysctl entries for all possible cpus.

Quoted 5 lines

This is visible to the user if the kernel is booted with maxcpus=n, or if ACPI tables have been modified to leave cpus offline, and then checking for missing /proc/sys/kernel/sched_domain/cpu* entries.
Fix this by separating the allocataion and initialization, and adding

nit: s/allocataion/allocation (perhaps maintainer can fixup?)

Quoted 26 lines

a flag to initialize the possible cpu entries while system booting only.
Signed-off-by Hidetoshi Seto <[email protected]>
Reviewed-by Masayoshi Mizuma <[email protected]>
Tested-by Syuuichirou Ishii <[email protected]>
Tested-by Tarumizu, Kohei <[email protected]>
---
  kernel/sched/debug.c | 4 ++++
  1 file changed, 4 insertions(+)
kernel/sched/debug.c +1 -0
index de3de997e245..9c6637f3e21d 100644
@@ -310,6 +310,7 @@ static struct ctl_table *sd_alloc_ctl_cpu_table(int cpu)
 
  static cpumask_var_t		sd_sysctl_cpus;
  static struct ctl_table_header	*sd_sysctl_header;
+static int register_sched_domain_sysctl_on_boot = 1;
 
  void register_sched_domain_sysctl(void)
  {
@@ -344,9 +345,12 @@ void register_sched_domain_sysctl(void)
  	if (!cpumask_available(sd_sysctl_cpus)) {
  		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))

kernel/sched/debug.c void register_sched_domain_sysctl(void) · line 356

Quoted 10 lines

kernel/sched/debug.c +3 -0

  			return;
+	}
 
+	if (register_sched_domain_sysctl_on_boot) {
  		/* init to possible to not have holes in @cpu_entries */
  		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
+		register_sched_domain_sysctl_on_boot = 0;
  	}
 
  	for_each_cpu(i, sd_sysctl_cpus) {

Thanks for posting ... looks good on aarch64 where the issue mentioned in the commit message was reported.

Acked-by Joe Lawrence <[email protected]>

-- Joe

Peter Zijlstra

maintainer

2019-01-30 20:14 UTC #2 lore raw

In reply to Masayoshi Mizuma

Quoted 39 lines · Masayoshi Mizuma

From: Hidetoshi Seto <[email protected]>
register_sched_domain_sysctl() copies the cpu_possible_mask into sd_sysctl_cpus, but only if sd_sysctl_cpus hasn't already been allocated (ie, CONFIG_CPUMASK_OFFSTACK is set). However, when CONFIG_CPUMASK_OFFSTACK is not set, sd_sysctl_cpus is left uninitialized (all zeroes) and the kernel may fail to initialize sched_domain sysctl entries for all possible cpus.
This is visible to the user if the kernel is booted with maxcpus=n, or if ACPI tables have been modified to leave cpus offline, and then checking for missing /proc/sys/kernel/sched_domain/cpu* entries.
Fix this by separating the allocataion and initialization, and adding a flag to initialize the possible cpu entries while system booting only.
Signed-off-by Hidetoshi Seto <[email protected]>
Reviewed-by Masayoshi Mizuma <[email protected]>
Tested-by Syuuichirou Ishii <[email protected]>
Tested-by Tarumizu, Kohei <[email protected]>
---
 kernel/sched/debug.c | 4 ++++
 1 file changed, 4 insertions(+)
kernel/sched/debug.c +1 -0
index de3de997e245..9c6637f3e21d 100644
@@ -310,6 +310,7 @@ static struct ctl_table *sd_alloc_ctl_cpu_table(int cpu)
 
 static cpumask_var_t		sd_sysctl_cpus;
 static struct ctl_table_header	*sd_sysctl_header;
+static int register_sched_domain_sysctl_on_boot = 1;
 
 void register_sched_domain_sysctl(void)
 {
@@ -344,9 +345,12 @@ void register_sched_domain_sysctl(void)
 	if (!cpumask_available(sd_sysctl_cpus)) {
 		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))

kernel/sched/debug.c void register_sched_domain_sysctl(void) · line 356

Quoted 10 lines

kernel/sched/debug.c +3 -0

 			return;
+	}
 
+	if (register_sched_domain_sysctl_on_boot) {
 		/* init to possible to not have holes in @cpu_entries */
 		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
+		register_sched_domain_sysctl_on_boot = 0;
 	}
 
 	for_each_cpu(i, sd_sysctl_cpus) {

I change it like the below. By keeping the initial value 0 it can go into .bss instead of .data.

kernel/sched/debug.c +4 -0

@@ -315,6 +315,7 @@ void register_sched_domain_sysctl(void)
 {
 	static struct ctl_table *cpu_entries;
 	static struct ctl_table **cpu_idx;
+	static bool init_done = false;
 	char buf[32];
 	int i;

@@ -344,7 +345,10 @@ void register_sched_domain_sysctl(void)
 	if (!cpumask_available(sd_sysctl_cpus)) {
 		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
 			return;
+	}
 
+	if (!init_done) {
+		init_done = true;
 		/* init to possible to not have holes in @cpu_entries */
 		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
 	}

Masayoshi Mizuma

author

2019-01-31 13:45 UTC #3 lore raw

Quoted 49 lines · Peter Zijlstra

Quoted 48 lines · Masayoshi Mizuma
From: Hidetoshi Seto <[email protected]>
register_sched_domain_sysctl() copies the cpu_possible_mask into sd_sysctl_cpus, but only if sd_sysctl_cpus hasn't already been allocated (ie, CONFIG_CPUMASK_OFFSTACK is set). However, when CONFIG_CPUMASK_OFFSTACK is not set, sd_sysctl_cpus is left uninitialized (all zeroes) and the kernel may fail to initialize sched_domain sysctl entries for all possible cpus.
This is visible to the user if the kernel is booted with maxcpus=n, or if ACPI tables have been modified to leave cpus offline, and then checking for missing /proc/sys/kernel/sched_domain/cpu* entries.
Fix this by separating the allocataion and initialization, and adding a flag to initialize the possible cpu entries while system booting only.
Signed-off-by Hidetoshi Seto <[email protected]>
Reviewed-by Masayoshi Mizuma <[email protected]>
Tested-by Syuuichirou Ishii <[email protected]>
Tested-by Tarumizu, Kohei <[email protected]>
---
 kernel/sched/debug.c | 4 ++++
 1 file changed, 4 insertions(+)
kernel/sched/debug.c +4 -0
index de3de997e245..9c6637f3e21d 100644
@@ -310,6 +310,7 @@ static struct ctl_table *sd_alloc_ctl_cpu_table(int cpu)
 
 static cpumask_var_t		sd_sysctl_cpus;
 static struct ctl_table_header	*sd_sysctl_header;
+static int register_sched_domain_sysctl_on_boot = 1;
 
 void register_sched_domain_sysctl(void)
 {
@@ -344,9 +345,12 @@ void register_sched_domain_sysctl(void)
 	if (!cpumask_available(sd_sysctl_cpus)) {
 		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
 			return;
+	}
 
+	if (register_sched_domain_sysctl_on_boot) {
 		/* init to possible to not have holes in @cpu_entries */
 		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
+		register_sched_domain_sysctl_on_boot = 0;
 	}
 

Quoted 4 lines

kernel/sched/debug.c void register_sched_domain_sysctl(void) · line 356
Quoted 1 line
kernel/sched/debug.c +0 -0
 	for_each_cpu(i, sd_sysctl_cpus) {
I change it like the below. By keeping the initial value 0 it can go into .bss instead of .data.

Great, thanks!
Should I re-post the patch as v2?

- Masa

Quoted 22 lines

kernel/sched/debug.c +4 -0

@@ -315,6 +315,7 @@ void register_sched_domain_sysctl(void)
 {
 	static struct ctl_table *cpu_entries;
 	static struct ctl_table **cpu_idx;
+	static bool init_done = false;
 	char buf[32];
 	int i;

@@ -344,7 +345,10 @@ void register_sched_domain_sysctl(void)
 	if (!cpumask_available(sd_sysctl_cpus)) {
 		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
 			return;
+	}
 
+	if (!init_done) {
+		init_done = true;
 		/* init to possible to not have holes in @cpu_entries */
 		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
 	}

Joe Lawrence

2019-02-01 22:33 UTC #4 lore raw

Quoted 55 lines · Masayoshi Mizuma

Quoted 49 lines · Peter Zijlstra
Quoted 48 lines · Masayoshi Mizuma
From: Hidetoshi Seto <[email protected]>
register_sched_domain_sysctl() copies the cpu_possible_mask into sd_sysctl_cpus, but only if sd_sysctl_cpus hasn't already been allocated (ie, CONFIG_CPUMASK_OFFSTACK is set). However, when CONFIG_CPUMASK_OFFSTACK is not set, sd_sysctl_cpus is left uninitialized (all zeroes) and the kernel may fail to initialize sched_domain sysctl entries for all possible cpus.
This is visible to the user if the kernel is booted with maxcpus=n, or if ACPI tables have been modified to leave cpus offline, and then checking for missing /proc/sys/kernel/sched_domain/cpu* entries.
Fix this by separating the allocataion and initialization, and adding a flag to initialize the possible cpu entries while system booting only.
Signed-off-by Hidetoshi Seto <[email protected]>
Reviewed-by Masayoshi Mizuma <[email protected]>
Tested-by Syuuichirou Ishii <[email protected]>
Tested-by Tarumizu, Kohei <[email protected]>
---
  kernel/sched/debug.c | 4 ++++
  1 file changed, 4 insertions(+)
kernel/sched/debug.c +4 -0
index de3de997e245..9c6637f3e21d 100644
@@ -310,6 +310,7 @@ static struct ctl_table *sd_alloc_ctl_cpu_table(int cpu)
 
  static cpumask_var_t		sd_sysctl_cpus;
  static struct ctl_table_header	*sd_sysctl_header;
+static int register_sched_domain_sysctl_on_boot = 1;
 
  void register_sched_domain_sysctl(void)
  {
@@ -344,9 +345,12 @@ void register_sched_domain_sysctl(void)
  	if (!cpumask_available(sd_sysctl_cpus)) {
  		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
  			return;
+	}
 
+	if (register_sched_domain_sysctl_on_boot) {
  		/* init to possible to not have holes in @cpu_entries */
  		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
+		register_sched_domain_sysctl_on_boot = 0;
  	}
 
Quoted 4 lines
kernel/sched/debug.c void register_sched_domain_sysctl(void) · line 356
Quoted 1 line
kernel/sched/debug.c +0 -0
  	for_each_cpu(i, sd_sysctl_cpus) {
I change it like the below. By keeping the initial value 0 it can go into .bss instead of .data.

Quoted 5 lines

Great, thanks!
Should I re-post the patch as v2?
- Masa

Peter's revision tests fine on my aarch64 box and would get my ACK, however it ends up getting re-posted :)

Thanks,

-- Joe

Quoted 17 lines

kernel/sched/debug.c +2 -0

@@ -315,6 +315,7 @@ void register_sched_domain_sysctl(void)
  {
  	static struct ctl_table *cpu_entries;
  	static struct ctl_table **cpu_idx;
+	static bool init_done = false;
  	char buf[32];
  	int i;

@@ -344,7 +345,10 @@ void register_sched_domain_sysctl(void)
  	if (!cpumask_available(sd_sysctl_cpus)) {
  		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
  			return;
+	}

Quoted 5 lines

kernel/sched/debug.c void register_sched_domain_sysctl(void) · line 354
Quoted 5 lines
kernel/sched/debug.c +2 -0
+	if (!init_done) {
+		init_done = true;
  		/* init to possible to not have holes in @cpu_entries */
  		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
  	}

-- Joe

tip-bot for Hidetoshi Seto

merge bot

2019-02-04 09:02 UTC #5 lore raw

[tip:sched/core] sched/debug: Initialize sd_sysctl_cpus if !CONFIG_CPUMASK_OFFSTACK

In reply to Masayoshi Mizuma

sched/debug: Initialize sd_sysctl_cpus if !CONFIG_CPUMASK_OFFSTACK

Commit-ID:  1ca4fa3ab604734e38e2a3000c9abf788512ffa7
Gitweb:     https://git.kernel.org/tip/1ca4fa3ab604734e38e2a3000c9abf788512ffa7
Author:     Hidetoshi Seto <[email protected]>
AuthorDate: Tue, 29 Jan 2019 10:12:45 -0500
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 4 Feb 2019 09:13:21 +0100

sched/debug: Initialize sd_sysctl_cpus if !CONFIG_CPUMASK_OFFSTACK

This is visible to the user if the kernel is booted with maxcpus=n, or if ACPI tables have been modified to leave CPUs offline, and then checking for missing /proc/sys/kernel/sched_domain/cpu* entries.

Fix this by separating the allocation and initialization, and adding a flag to initialize the possible CPU entries while system booting only.

Tested-by Syuuichirou Ishii <[email protected]>

Tested-by Tarumizu, Kohei <[email protected]>

Signed-off-by Hidetoshi Seto <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Reviewed-by Masayoshi Mizuma <[email protected]>

Acked-by Joe Lawrence <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Masayoshi Mizuma <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link https://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/debug.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sched/debug.c +4 -0

index de3de997e245..8039d62ae36e 100644
@@ -315,6 +315,7 @@ void register_sched_domain_sysctl(void)
 {
 	static struct ctl_table *cpu_entries;
 	static struct ctl_table **cpu_idx;
+	static bool init_done = false;
 	char buf[32];
 	int i;

@@ -344,7 +345,10 @@ void register_sched_domain_sysctl(void)
 	if (!cpumask_available(sd_sysctl_cpus)) {
 		if (!alloc_cpumask_var(&sd_sysctl_cpus, GFP_KERNEL))
 			return;
+	}
 
+	if (!init_done) {
+		init_done = true;
 		/* init to possible to not have holes in @cpu_entries */
 		cpumask_copy(sd_sysctl_cpus, cpu_possible_mask);
 	}