Re: [RFC PATCH] Dynamic sched domains (v0.5)

[Matthew Dobson <colpatch@us.ibm.com>]

Dinakar Guniguntala wrote:
> Ok, Here is the minimal patchset that I had promised after the
> last discussion.
> 
> What it does have
> o  The current patch enhances the meaning of exclusive cpusets by
>    attaching them (exclusive cpusets) to sched domains
> o  It does _not_ require any additional cpumask_t variable. It
>    just parses the cpus_allowed of the parent/sibling/children
>    cpusets for manipulating sched domains
> o  All existing operations on non-/exclusive cpusets are preserved as-is.
> o  The sched code has been modified to bring it upto 2.6.12-rc2-mm3
> 
> Usage
> o  On setting the cpu_exclusive flag of a cpuset X, it creates two
>    sched domains
>    a. One, All cpus from X's parent cpuset that dont belong to any
>       exclusive sibling cpuset of X
>    b. Two, All cpus in X's cpus_allowed
> o  On adding/deleting cpus to/from a exclusive cpuset X that has exclusive
>    children, it creates two sched domains
>    a. One, All cpus from X's parent cpuset that dont belong to any
>       exclusive sibling cpuset of X
>    b. Two, All cpus in X's cpus_allowed, after taking away any cpus that
>       belong to exclusive child cpusets of X
> o  On unsetting the cpu_exclusive flag of cpuset X or rmdir X, it creates a
>    single sched domain, containing all cpus from X' parent cpuset that
>    dont belong to any exclusive sibling of X and the cpus of X
> o  It does _not_ modify the cpus_allowed variable of the parent as in the
>    previous version. It relies on user space to move tasks to proper
>    cpusets for complete isolation/exclusion
> o  See function update_cpu_domains for more details
> 
> What it does not have
> o  It is still short on documentation
> o  Does not have hotplug support as yet
> o  Supports only x86 as of now
> o  No thoughts on "memory domains" (Though I am not sure, who
>    would use such a thing and what exactly are the requirements)

An interesting feature.  I tried a while ago to get cpusets and
sched_domains to play nice (nicer?) and didn't have much luck.  It seems
you're taking a better approach, with smaller patches.  Good luck!


> diff -Naurp linux-2.6.12-rc2.orig/include/linux/init.h linux-2.6.12-rc2/include/linux/init.h
> --- linux-2.6.12-rc2.orig/include/linux/init.h	2005-04-04 22:07:52.000000000 +0530
> +++ linux-2.6.12-rc2/include/linux/init.h	2005-05-01 22:07:56.000000000 +0530
> @@ -217,7 +217,7 @@ void __init parse_early_param(void);
>  #define __initdata_or_module __initdata
>  #endif /*CONFIG_MODULES*/
>  
> -#ifdef CONFIG_HOTPLUG
> +#if defined(CONFIG_HOTPLUG) || defined(CONFIG_CPUSETS)
>  #define __devinit
>  #define __devinitdata
>  #define __devexit

This looks just plain wrong.  Why do you need this?  It doesn't seem that
arch_init_sched_domains() and/or update_sched_domains() are called from
anywhere that is cpuset related, so why the #ifdef CONFIG_CPUSETS?


> diff -Naurp linux-2.6.12-rc2.orig/kernel/sched.c linux-2.6.12-rc2/kernel/sched.c
> --- linux-2.6.12-rc2.orig/kernel/sched.c	2005-04-28 18:24:11.000000000 +0530
> +++ linux-2.6.12-rc2/kernel/sched.c	2005-05-01 22:06:55.000000000 +0530
> @@ -4526,7 +4526,7 @@ int __init migration_init(void)
>  #endif
>  
>  #ifdef CONFIG_SMP
> -#define SCHED_DOMAIN_DEBUG
> +#undef SCHED_DOMAIN_DEBUG
>  #ifdef SCHED_DOMAIN_DEBUG
>  static void sched_domain_debug(struct sched_domain *sd, int cpu)
>  {

Is this just to quiet boot for your testing?  Is there are better reason
you're turning this off?  It seems unrelated to the rest of your patch.


> ------------------------------------------------------------------------
> 
> diff -Naurp linux-2.6.12-rc2.orig/kernel/cpuset.c linux-2.6.12-rc2/kernel/cpuset.c
> --- linux-2.6.12-rc2.orig/kernel/cpuset.c	2005-04-28 18:24:11.000000000 +0530
> +++ linux-2.6.12-rc2/kernel/cpuset.c	2005-05-01 22:15:06.000000000 +0530
> @@ -602,12 +602,48 @@ static int validate_change(const struct 
>  	return 0;
>  }
>  
> +static void update_cpu_domains(struct cpuset *cur)
> +{
> +	struct cpuset *c, *par = cur->parent;
> +	cpumask_t span1, span2;
> +
> +	if (par == NULL || cpus_empty(cur->cpus_allowed))
> +		return;
> +
> +	/* Get all non-exclusive cpus from parent domain */
> +	span1 = par->cpus_allowed;
> +	list_for_each_entry(c, &par->children, sibling) {
> +		if (is_cpu_exclusive(c))
> +			cpus_andnot(span1, span1, c->cpus_allowed);
> +	}
> +	if (is_removed(cur) || !is_cpu_exclusive(cur)) {
> +		cpus_or(span1, span1, cur->cpus_allowed);
> +		if (cpus_equal(span1, cur->cpus_allowed))
> +			return;
> +		span2 = CPU_MASK_NONE;
> +	}
> +	else {
> +		if (cpus_empty(span1))
> +			return;
> +		span2 = cur->cpus_allowed;
> +		/* If current cpuset has exclusive children, exclude from domain */
> +		list_for_each_entry(c, &cur->children, sibling) {
> +			if (is_cpu_exclusive(c))
> +				cpus_andnot(span2, span2, c->cpus_allowed);
> +		}
> +	}
> +
> +	lock_cpu_hotplug();
> +	rebuild_sched_domains(span1, span2);
> +	unlock_cpu_hotplug();
> +}

Nitpicky, but span1 and span2 could do with better names.

Otherwise, the patch looks good to me.

-Matt