Re: [PATCH] cpuset and sched domains: sched_load_balance flag

[Randy Dunlap <randy.dunlap@oracle.com>]

On Sun, 30 Sep 2007 03:44:03 -0700 Paul Jackson wrote:

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

Are there some attributions missing, else S-O-B ?

> ---
> 
> Andrew - this patch goes right after your *-mm patch:
>   task-containers-enable-containers-by-default-in-some-configs.patch
> and before "add-containerstats-v3.patch"
> 
>  Documentation/cpusets.txt |  141 +++++++++++++++++++++++++
>  include/linux/sched.h     |    2 
>  kernel/cpuset.c           |  254 +++++++++++++++++++++++++++++++++++++++++++++-
>  kernel/sched.c            |   72 ++++++++++---
>  4 files changed, 450 insertions(+), 19 deletions(-)

> --- 2.6.23-rc8-mm1.orig/kernel/cpuset.c	2007-09-29 23:56:40.987962675 -0700
> +++ 2.6.23-rc8-mm1/kernel/cpuset.c	2007-09-29 23:57:51.148979999 -0700

>  /*
> + * Helper routine for rebuild_sched_domains().
> + * Do cpusets a, b have overlapping cpus_allowed masks?
> + */
> +
> +static int cpusets_overlap(struct cpuset *a, struct cpuset *b)

inline ?

> +{
> +	return cpus_intersects(a->cpus_allowed, b->cpus_allowed);
> +}
> +
...
> +
> +static void rebuild_sched_domains(void)
> +{
> +	struct kfifo *q;	/* queue of cpusets to be scanned */
> +	struct cpuset *cp;	/* scans q */
> +	struct cpuset **csa;	/* array of all cpuset ptrs */
> +	int csn;		/* how many cpuset ptrs in csa so far */
> +	int i, j, k;		/* indices for partition finding loops */
> +	cpumask_t *doms;	/* resulting partition; i.e. sched domains */
> +	int ndoms;		/* number of sched domains in result */
> +	int nslot;		/* next empty doms[] cpumask_t slot */
> +
> +	q = NULL; csa = NULL; doms = NULL;

That's not kernel style.  Use either (Andrew would say the second one):

	q = csa = doms = NULL;

or
	q = NULL;
	csa = NULL;
	doms = NULL;

> +
> +	/* Special case for the 99% of systems with one, full, sched domain */
> +	if (is_sched_load_balance(&top_cpuset)) {
> +		ndoms = 1;
> +		doms = kmalloc(sizeof(cpumask_t), GFP_KERNEL);
> +		*doms = top_cpuset.cpus_allowed;
> +		goto rebuild;
> +	}
> +
...

> +
> +rebuild:
> +	/* Have scheduler rebuild sched domains */
> +	lock_cpu_hotplug();
> +	partition_sched_domains(ndoms, doms);
> +	unlock_cpu_hotplug();
> +
> +done:
> +	if (q && !IS_ERR(q))
> +		kfree(q);
> +	if (csa)

Don't need the conditional: kfree(NULL) is OK.

> +		kfree(csa);
> +	/* Don't kfree(doms) -- partition_sched_domains() does that. */
> +}
> +
> +/*
>   * Call with manage_mutex held.  May take callback_mutex during call.
>   */

---
~Randy