Skip to contentSkip to conversation

sched domain debug sysctl fixes

PATCH In review Milton Miller 2 participants 10 messages Oct 12, 2007 – Oct 16, 2007

lore
Share & tools
lore

Milton Miller

author
#0 lore raw

PATCH 1/1 sched domain debug sysctl fixes

The following 5 patches provide fixes for the sysctl code added under
CONFIG_DEBUG_SCHED. After debugging a boot failure dereferencing
not per-cpu data for an impossible cpu, I found the other issues by inspection.

Following these fixes, /proc/sysctl/kernel/sched_debug/ should work with exclusive cpusets, cpu hotplug, offline and not present cpus, and will show all 11 files.

milton

author
#1 lore raw

PATCH 1/1 sched domain sysctl: free and rebuild when changing domains

Unregister and free the sysctl table before destroying domains, then rebuild and register after creating the new domains. This prevents the sysctl table from pointing to freed memory for root to write.

Signed-off-by Milton Miller <[email protected]>
kernel/sched.c +30 -4
Index: kernel/kernel/sched.c
===================================================================
@@ -5291,6 +5291,18 @@ static struct ctl_table *sd_alloc_ctl_en
 	return entry;
 }
 
+static void sd_free_ctl_entry(struct ctl_table **tablep)
+{
+	struct ctl_table *entry = *tablep;
+
+	for (entry = *tablep; entry->procname; entry++)
+		if (entry->child)
+			sd_free_ctl_entry(&entry->child);
+
+	kfree(*tablep);
+	*tablep = NULL;
+}
+
 static void
 set_table_entry(struct ctl_table *entry,
 		const char *procname, void *data, int maxlen,

@@ -5359,7 +5371,7 @@ static ctl_table *sd_alloc_ctl_cpu_table
 }
 
 static struct ctl_table_header *sd_sysctl_header;
-static void init_sched_domain_sysctl(void)
+static void register_sched_domain_sysctl(void)
 {
 	int i, cpu_num = num_online_cpus();
 	struct ctl_table *entry = sd_alloc_ctl_entry(cpu_num + 1);

@@ -5376,8 +5388,18 @@ static void init_sched_domain_sysctl(voi
 	}
 	sd_sysctl_header = register_sysctl_table(sd_ctl_root);
 }
+
+static void unregister_sched_domain_sysctl(void)
+{
+	unregister_sysctl_table(sd_sysctl_header);
+	sd_sysctl_header = NULL;
+	sd_free_ctl_entry(&sd_ctl_dir[0].child);
+}
 #else
-static void init_sched_domain_sysctl(void)
+static void register_sched_domain_sysctl(void)
+{
+}
+static void unregister_sched_domain_sysctl(void)
 {
 }
 #endif

@@ -6311,6 +6333,8 @@ static int arch_init_sched_domains(const
 
 	err = build_sched_domains(&cpu_default_map);
 
+	register_sched_domain_sysctl();
+
 	return err;
 }
 

@@ -6327,6 +6351,8 @@ static void detach_destroy_domains(const
 {
 	int i;
 
+	unregister_sched_domain_sysctl();
+
 	for_each_cpu_mask(i, *cpu_map)
 		cpu_attach_domain(NULL, i);
 	synchronize_sched();

@@ -6357,6 +6383,8 @@ int partition_sched_domains(cpumask_t *p
 	if (!err && !cpus_empty(*partition2))
 		err = build_sched_domains(partition2);
 
+	register_sched_domain_sysctl();
+
 	return err;
 }
 

@@ -6488,8 +6516,6 @@ void __init sched_init_smp(void)
 	/* XXX: Theoretical race here - CPU may be hotplugged now */
 	hotcpu_notifier(update_sched_domains, 0);
 
-	init_sched_domain_sysctl();
-
 	/* Move init over to a non-isolated CPU */
 	if (set_cpus_allowed(current, non_isolated_cpus) < 0)
 		BUG();
author
#2 lore raw

PATCH 1/1 sched.c: use kcalloc

In reply to Milton Miller

kcalloc checks for n * sizeof(element) overflows and it zeros.

Signed-off-by Milton Miller <[email protected]>
kernel/sched.c +2 -3
Index: kernel/kernel/sched.c
===================================================================
@@ -5283,10 +5283,9 @@ static struct ctl_table sd_ctl_root[] = 
 static struct ctl_table *sd_alloc_ctl_entry(int n)
 {
 	struct ctl_table *entry =
-		kmalloc(n * sizeof(struct ctl_table), GFP_KERNEL);
+		kcalloc(n, sizeof(struct ctl_table), GFP_KERNEL);
 
 	BUG_ON(!entry);
-	memset(entry, 0, n * sizeof(struct ctl_table));
 
 	return entry;
 }

@@ -6080,7 +6079,7 @@ static int build_sched_domains(const cpu
 	/*
 	 * Allocate the per-node list of sched groups
 	 */
-	sched_group_nodes = kzalloc(sizeof(struct sched_group *)*MAX_NUMNODES,
+	sched_group_nodes = kcalloc(MAX_NUMNODES, sizeof(struct sched_group *),
 					   GFP_KERNEL);
 	if (!sched_group_nodes) {
 		printk(KERN_WARNING "Can not alloc sched group node list\n");
author
#3 lore raw

PATCH 1/1 sched domain sysctl: register online cpus

In reply to Milton Miller

init_sched_domain_sysctl was walking cpus 0-n and referencing per_cpu variables. If the cpus_possible mask is not contigious this will result in a crash referencing unallocated data. If the online mask is not contigious then we would show offline cpus and miss online ones.

Signed-off-by Milton Miller <[email protected]>
kernel/sched.c +2 -1
Index: kernel/kernel/sched.c
===================================================================
@@ -5367,11 +5367,12 @@ static void init_sched_domain_sysctl(voi
 
 	sd_ctl_dir[0].child = entry;
 
-	for (i = 0; i < cpu_num; i++, entry++) {
+	for_each_online_cpu(i) {
 		snprintf(buf, 32, "cpu%d", i);
 		entry->procname = kstrdup(buf, GFP_KERNEL);
 		entry->mode = 0555;
 		entry->child = sd_alloc_ctl_cpu_table(i);
+		entry++;
 	}
 	sd_sysctl_header = register_sysctl_table(sd_ctl_root);
 }
author
#4 lore raw

PATCH 1/1 sched domain sysctl: sysctl tables can have no holes

In reply to Milton Miller

The register_sysctl_table stops on the first entry without a procname and ctl_name, so we need to fill in consecutive entries.

Signed-off-by Milton Miller <[email protected]>
kernel/sched.c +4 -3
Index: kernel/kernel/sched.c
===================================================================
@@ -5315,7 +5315,7 @@ set_table_entry(struct ctl_table *entry,
 static struct ctl_table *
 sd_alloc_ctl_domain_table(struct sched_domain *sd)
 {
-	struct ctl_table *table = sd_alloc_ctl_entry(14);
+	struct ctl_table *table = sd_alloc_ctl_entry(12);
 
 	if (table == NULL)
 		return NULL;

@@ -5338,11 +5338,12 @@ sd_alloc_ctl_domain_table(struct sched_d
 		sizeof(int), 0644, proc_dointvec_minmax);
 	set_table_entry(&table[8], "imbalance_pct", &sd->imbalance_pct,
 		sizeof(int), 0644, proc_dointvec_minmax);
-	set_table_entry(&table[10], "cache_nice_tries",
+	set_table_entry(&table[9], "cache_nice_tries",
 		&sd->cache_nice_tries,
 		sizeof(int), 0644, proc_dointvec_minmax);
-	set_table_entry(&table[12], "flags", &sd->flags,
+	set_table_entry(&table[10], "flags", &sd->flags,
 		sizeof(int), 0644, proc_dointvec_minmax);
+	/* &table[11] is terminator */
 
 	return table;
 }
author
#5 lore raw

PATCH 1/1 sched domain sysctl: don't bug on alloc failure

In reply to Milton Miller

Now that we are calling this at runtime, a more relaxed error path is suggested. If an allocation fails, we just register the partial table, which will show empty directories.

Signed-off-by Milton Miller <[email protected]>
kernel/sched.c +8 -2
Index: kernel/kernel/sched.c
===================================================================
@@ -5285,8 +5285,6 @@ static struct ctl_table *sd_alloc_ctl_en
 	struct ctl_table *entry =
 		kcalloc(n, sizeof(struct ctl_table), GFP_KERNEL);
 
-	BUG_ON(!entry);
-
 	return entry;
 }
 

@@ -5319,6 +5317,9 @@ sd_alloc_ctl_domain_table(struct sched_d
 {
 	struct ctl_table *table = sd_alloc_ctl_entry(14);
 
+	if (table == NULL)
+		return NULL;
+
 	set_table_entry(&table[0], "min_interval", &sd->min_interval,
 		sizeof(long), 0644, proc_doulongvec_minmax);
 	set_table_entry(&table[1], "max_interval", &sd->max_interval,

@@ -5356,6 +5357,8 @@ static ctl_table *sd_alloc_ctl_cpu_table
 	for_each_domain(cpu, sd)
 		domain_num++;
 	entry = table = sd_alloc_ctl_entry(domain_num + 1);
+	if (table == NULL)
+		return NULL;
 
 	i = 0;
 	for_each_domain(cpu, sd) {

@@ -5376,6 +5379,9 @@ static void register_sched_domain_sysctl
 	struct ctl_table *entry = sd_alloc_ctl_entry(cpu_num + 1);
 	char buf[32];
 
+	if (entry == NULL)
+		return;
+
 	sd_ctl_dir[0].child = entry;
 
 	for_each_online_cpu(i) {
author
#6 lore raw

PATCH 1/1 sched domain debug sysctl fixes

In reply to Milton Miller

I forgot to put the seqence numbers in the patch subjects (I did in the message ids).

Here is the order I diffed them:

sched domain sysctl: register online cpus
sched domain sysctl: free and rebuild when changing domains sched.c: use kcalloc
sched domain sysctl: don't bug on alloc failure sched domain sysctl: sysctl tables can have no holes

milton

#7 lore raw
In reply to Milton Miller
Quoted 8 lines · Milton Miller

The following 5 patches provide fixes for the sysctl code added under CONFIG_DEBUG_SCHED. After debugging a boot failure dereferencing not per-cpu data for an impossible cpu, I found the other issues by inspection.

Following these fixes, /proc/sysctl/kernel/sched_debug/ should work with exclusive cpusets, cpu hotplug, offline and not present cpus, and will show all 11 files.

thanks, great fixes! They should show up in the next sched-devel git tree.

Ingo

author
#8 lore raw

PATCH 1/1 sched domain sysctl: free kstrdup allocations

The procnames for the cpu and domain were allocated via kstrdup and so
should also be freed. The names for the files are static, but we
can differentiate them by the presence of the proc_handler. If a kstrdup (of < 32 characters) fails the sysctl code will not see the procname or remaining table entries, but any child tables and names will be reclaimed upon free.

Signed-off-by Milton Miller <[email protected]>

---

Hi Ingo.

It occurred to me this morning that the procname field was dynamically allocated and needed to be freed. I started to put in break statements when allocation failed but it was approaching 50% error handling code.

I came up with this alternative of looping while entry->mode is set and checking proc_handler instead of ->table. Alternatively, the string version of the domain name and cpu number could be stored the structs.

I verified by compiling CONFIG_DEBUG_SLAB and checking the allocation counts after taking a cpuset exclusive and back.

milton

kernel/sched.c +11 -2
Index: kernel/kernel/sched.c
===================================================================
@@ -5290,11 +5290,20 @@ static struct ctl_table *sd_alloc_ctl_en
 
 static void sd_free_ctl_entry(struct ctl_table **tablep)
 {
-	struct ctl_table *entry = *tablep;
+	struct ctl_table *entry;
 
-	for (entry = *tablep; entry->procname; entry++)
+	/*
+	 * In the intermediate directories, both the child directory and
+	 * procname are dynamically allocated and could fail but the mode
+	 * will always be set.  In the lowest directory the names are
+	 * static strings and all have proc handlers.
+	 */
+	for (entry = *tablep; entry->mode; entry++) {
 		if (entry->child)
 			sd_free_ctl_entry(&entry->child);
+		if (entry->proc_handler == NULL)
+			kfree(entry->procname);
+	}
 
 	kfree(*tablep);
 	*tablep = NULL;
Applied
#9 lore raw

Re: [PATCH] sched domain sysctl: free kstrdup allocations

Quoted 6 lines · Milton Miller

The procnames for the cpu and domain were allocated via kstrdup and so should also be freed. The names for the files are static, but we can differentiate them by the presence of the proc_handler. If a kstrdup (of < 32 characters) fails the sysctl code will not see the procname or remaining table entries, but any child tables and names will be reclaimed upon free.

thanks, applied.

Ingo