Re: [RFC PATCH] Dynamic sched domains aka Isolated cpusets (v0.2)

[Paul Jackson <pj@sgi.com>]

A few code details (still working on more substantive reply):

+	/* An isolated cpuset has to be exclusive */
+	if ((is_cpu_isolated(trial) && !is_cpu_exclusive(cur)) 
+	   || (!is_cpu_exclusive(trial) && is_cpu_isolated(cur)))
+		return -EINVAL;

Is the above code equivalant to what the comment states:

	if (is_cpu_isolated(trial) <= is_cpu_exclusive(trial))
		return -EINVAL;

+	t = old_parent = *par;
+	cpus_or(all_map, cs->cpus_allowed, cs->isolated_map);
+
+	/* If cpuset empty or top_cpuset, return */
+        if (cpus_empty(all_map) || par == NULL)
+                return;

If the (par == NULL) check succeeds, then perhaps the earlier (*par)
dereference will have oopsed first?

+	struct cpuset *par = cs->parent, t, old_parent;

Looks like 't' was chosen to be a one-char variable name, to keep some
lines below within 80 columns.  I'd do the same myself.  But this leaves
a non-symmetrical naming pattern for the new and old parent cpuset values.

Perhaps the following would work better?

	struct cpuset *parptr;
	struct cpuset o, n;	/* old and new parent cpuset values */

+static void update_cpu_domains(struct cpuset *cs, cpumask_t old_map)

Could old_map be passed as a (const cpumask_t *)?  The stack space of
this code, just for cpumask_t's (see the old and new above) is getting
large for (really) big systems.

+	/* Make the change */
+	par->cpus_allowed = t.cpus_allowed;
+	par->isolated_map = t.isolated_map;

Why don't you need to propogate upward this change to the parents
cpus_allowed and isolated_map?  If a parents isolated_map grows (or
shrinks), doesn't that affect every ancestor, all the way to the top
cpuset?

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