[PATCH] sched: CPU hotplug events must not destroy scheduler domains created by the cpusets

[PATCH] sched: CPU hotplug events must not destroy scheduler domains created by the cpusets

[Max Krasnyansky]

This is an updated/split up version of the patch I sent earlier.

Currently sched domains created by the cpusets are completely destroyed
during CPU hotplug event handling. For each CPU hotplug event scheduler
attaches all CPUs to the NULL domain and then puts them all into a single
domain thereby destroying domains created by the cpusets.

The solution is simple, when cpusets are enabled scheduler should not
create the default domain and instead let cpusets rebuild the domains based
on the current settings. Which is exactly what the patch does.

This is just a bug fix and should go into 2.6.26 and .25-stable.

Signed-off-by: Max Krasnyansky <maxk@qualcomm.com>
---
 kernel/cpuset.c |    6 ++++++
 kernel/sched.c  |    8 ++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index a1b61f4..29c6304 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1789,6 +1789,12 @@ static void common_cpu_mem_hotplug_unplug(void)
 	top_cpuset.mems_allowed = node_states[N_HIGH_MEMORY];
 	scan_for_empty_cpusets(&top_cpuset);
 
+	/*
+	 * Scheduler destroys domains on hotplug events.
+	 * Rebuild them based on the current settings.
+	 */
+	rebuild_sched_domains();
+
 	cgroup_unlock();
 }
 
diff --git a/kernel/sched.c b/kernel/sched.c
index 465f3c8..da7c6c2 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -7091,8 +7091,16 @@ static int update_sched_domains(struct notifier_block *nfb,
 		return NOTIFY_DONE;
 	}
 
+#ifndef CONFIG_CPUSETS
+	/*
+	 * Create default domain partitioning if cpusets are disabled.
+	 * Otherwise we let cpusets rebuild the domains based on the 
+	 * current setup.
+	 */
+
 	/* The hotplug lock is already held by cpu_up/cpu_down */
 	arch_init_sched_domains(&cpu_online_map);
+#endif
 
 	return NOTIFY_OK;
 }
-- 
1.5.4.5

Re: [PATCH] sched: CPU hotplug events must not destroy scheduler domains created by the cpusets

[Max Krasnyansky]

Paul Jackson wrote:

> How about:
>  1) Defining arch_init_sched_domains() for all CONFIGs, CPUSET or not
>     (see for example how free_sched_groups() is defined, NUMA or not)
>  2) Then the ifndef can be removed from here.
>  3) Then the arch_init_sched_domains() can be moved comfortably inside
>     the switch statement.

Paul, I just realized that this won't work. We actually need the default
version of arch_init_sched_domains() which builds the default domain. It's
used during bootup. It's only the hotplug event handling where we do not want
the default.
I could still redo the switch() statement as you suggested but it seems that
it's not going to improve the readability that much at this point.

Max

Re: [PATCH] sched: CPU hotplug events must not destroy scheduler domains created by the cpusets

[Max Krasnyansky]

Paul Jackson wrote:
> I think this patch is ok --- though 

> (1) perhaps it should have been two patches, as it seems to fix two bugs, and 
I could split cpusets fix out I suppose.

> (2) I only managed to get my head about 80% of
> the way around this, so while I confident enough to Ack this one, I'm not
> as certain as I might be.
> 
> Acked-by: Paul Jackson <pj@sgi.com>
Thanx.

> I did see a couple of places where the code could be more clearly presented,
> and a couple of lines ending in space characters in *.c files (the Doc files
> are hopelessly failing this space terminator test, so I gave up on them.)
> [snip]
I'll fix the spaces and redo the scheduler parts based on your suggestions.
As usual, nice and thorough review. Thanx.

Max

Re: [PATCH] sched: CPU hotplug events must not destroy scheduler domains created by the cpusets

[Paul Jackson]

I think this patch is ok --- though (1) perhaps it should have been two patches,
as it seems to fix two bugs, and (2) I only managed to get my head about 80% of
the way around this, so while I confident enough to Ack this one, I'm not
as certain as I might be.

Acked-by: Paul Jackson <pj@sgi.com>


I did see a couple of places where the code could be more clearly presented,
and a couple of lines ending in space characters in *.c files (the Doc files
are hopelessly failing this space terminator test, so I gave up on them.)

Your patch includes the following.  Note the embedded "line ends in space"'s.

> @@ -7058,6 +7071,7 @@ static int update_sched_domains(struct notifier_block *nfb,
>  	case CPU_DOWN_PREPARE:
>  	case CPU_DOWN_PREPARE_FROZEN:
>  		detach_destroy_domains(&cpu_online_map);
> +		free_sched_domains();
>  		return NOTIFY_OK;
>  
>  	case CPU_UP_CANCELED:
> @@ -7076,8 +7090,16 @@ static int update_sched_domains(struct notifier_block *nfb,
>  		return NOTIFY_DONE;
>  	}
>  
> +#ifndef CONFIG_CPUSETS
> +	/* 

Above line ends in space.

> +	 * Create default domain partitioning if cpusets are disabled.
> +	 * Otherwise we let cpusets rebuild the domains based on the 

Above line ends in space.

> +	 * current setup.
> +	 */
> +
>  	/* The hotplug lock is already held by cpu_up/cpu_down */
>  	arch_init_sched_domains(&cpu_online_map);
> +#endif
>  
>  	return NOTIFY_OK;
>  }


The above patch, on top of the code already there, ends up with the
following code for update_sched_domains():

============================== begin ==============================
static int update_sched_domains(struct notifier_block *nfb,
                                unsigned long action, void *hcpu)
{
        switch (action) {
        case CPU_UP_PREPARE:
        case CPU_UP_PREPARE_FROZEN:
        case CPU_DOWN_PREPARE:
        case CPU_DOWN_PREPARE_FROZEN:
                detach_destroy_domains(&cpu_online_map);
                free_sched_domains();
                return NOTIFY_OK;

        case CPU_UP_CANCELED:
        case CPU_UP_CANCELED_FROZEN:
        case CPU_DOWN_FAILED:
        case CPU_DOWN_FAILED_FROZEN:
        case CPU_ONLINE:
        case CPU_ONLINE_FROZEN:
        case CPU_DEAD:
        case CPU_DEAD_FROZEN:
                /*
                 * Fall through and re-initialise the domains.
                 */
                break;
        default:
                return NOTIFY_DONE;
        }

#ifndef CONFIG_CPUSETS
        /*
         * Create default domain partitioning if cpusets are disabled.
         * Otherwise we let cpusets rebuild the domains based on the
         * current setup.
         */

        /* The hotplug lock is already held by cpu_up/cpu_down */
        arch_init_sched_domains(&cpu_online_map);
#endif

        return NOTIFY_OK;
}
=============================== end ===============================

The "Fall through", break, ifndef and out of the switch call seem to be
a more convoluted way of writing that routine than is necessary.

How about:
 1) Defining arch_init_sched_domains() for all CONFIGs, CPUSET or not
    (see for example how free_sched_groups() is defined, NUMA or not)
 2) Then the ifndef can be removed from here.
 3) Then the arch_init_sched_domains() can be moved comfortably inside
    the switch statement.

The end result would be a update_sched_domains() routine that looked
something like the following:

============================== begin ==============================
static int update_sched_domains(struct notifier_block *nfb,
                                unsigned long action, void *hcpu)
{
        switch (action) {
        case CPU_UP_PREPARE:
        case CPU_UP_PREPARE_FROZEN:
        case CPU_DOWN_PREPARE:
        case CPU_DOWN_PREPARE_FROZEN:
                detach_destroy_domains(&cpu_online_map);
                free_sched_domains();
                return NOTIFY_OK;
        case CPU_UP_CANCELED:
        case CPU_UP_CANCELED_FROZEN:
        case CPU_DOWN_FAILED:
        case CPU_DOWN_FAILED_FROZEN:
        case CPU_ONLINE:
        case CPU_ONLINE_FROZEN:
        case CPU_DEAD:
        case CPU_DEAD_FROZEN:
                arch_init_sched_domains(&cpu_online_map);
                return NOTIFY_OK;
        default:
                return NOTIFY_DONE;
        }
}
=============================== end ===============================


===

This routine:

static void free_sched_domains(void)
{
        ndoms_cur = 0;
        if (doms_cur != &fallback_doms)
                kfree(doms_cur);
        doms_cur = &fallback_doms;
}

might be easier to read as:

static void free_sched_domains(void)
{
        ndoms_cur = 0;
        if (doms_cur != &fallback_doms) {
                kfree(doms_cur);
	        doms_cur = &fallback_doms;
	}
}


-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

[PATCH] sched: CPU hotplug events must not destroy scheduler domains created by the cpusets

[Max Krasnyansky]

First issue is not related to the cpusets. We're simply leaking doms_cur.
It's allocated in arch_init_sched_domains() which is called for every
hotplug event. So we just keep reallocation doms_cur without freeing it.
I introduced free_sched_domains() function that cleans things up.

Second issue is that sched domains created by the cpusets are
completely destroyed by the CPU hotplug events. For all CPU hotplug
events scheduler attaches all CPUs to the NULL domain and then puts
them all into the single domain thereby destroying domains created
by the cpusets (partition_sched_domains).
The solution is simple, when cpusets are enabled scheduler should not
create default domain and instead let cpusets do that. Which is
exactly what the patch does.

Signed-off-by: Max Krasnyansky <maxk@qualcomm.com>
---
 kernel/cpuset.c |    6 ++++++
 kernel/sched.c  |   22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index a1b61f4..29c6304 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1789,6 +1789,12 @@ static void common_cpu_mem_hotplug_unplug(void)
 	top_cpuset.mems_allowed = node_states[N_HIGH_MEMORY];
 	scan_for_empty_cpusets(&top_cpuset);
 
+	/*
+	 * Scheduler destroys domains on hotplug events.
+	 * Rebuild them based on the current settings.
+	 */
+	rebuild_sched_domains();
+
 	cgroup_unlock();
 }
 
diff --git a/kernel/sched.c b/kernel/sched.c
index 8dcdec6..fc8f46b 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6855,6 +6855,18 @@ void __attribute__((weak)) arch_update_cpu_topology(void)
 }
 
 /*
+ * Free current domain masks.
+ * Called after all cpus are attached to NULL domain.
+ */
+static void free_sched_domains(void)
+{
+	ndoms_cur = 0;
+	if (doms_cur != &fallback_doms)
+		kfree(doms_cur);
+	doms_cur = &fallback_doms;
+}
+
+/*
  * Set up scheduler domains and groups. Callers must hold the hotplug lock.
  * For now this just excludes isolated cpus, but could be used to
  * exclude other special cases in the future.
@@ -6974,6 +6986,7 @@ int arch_reinit_sched_domains(void)
 
 	get_online_cpus();
 	detach_destroy_domains(&cpu_online_map);
+	free_sched_domains();
 	err = arch_init_sched_domains(&cpu_online_map);
 	put_online_cpus();
 
@@ -7058,6 +7071,7 @@ static int update_sched_domains(struct notifier_block *nfb,
 	case CPU_DOWN_PREPARE:
 	case CPU_DOWN_PREPARE_FROZEN:
 		detach_destroy_domains(&cpu_online_map);
+		free_sched_domains();
 		return NOTIFY_OK;
 
 	case CPU_UP_CANCELED:
@@ -7076,8 +7090,16 @@ static int update_sched_domains(struct notifier_block *nfb,
 		return NOTIFY_DONE;
 	}
 
+#ifndef CONFIG_CPUSETS
+	/* 
+	 * Create default domain partitioning if cpusets are disabled.
+	 * Otherwise we let cpusets rebuild the domains based on the 
+	 * current setup.
+	 */
+
 	/* The hotplug lock is already held by cpu_up/cpu_down */
 	arch_init_sched_domains(&cpu_online_map);
+#endif
 
 	return NOTIFY_OK;
 }
-- 
1.5.4.5