[PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Paul Jackson]

As reported by Paul Mackerras <paulus@samba.org>, the previous
patch "cpu_exclusive sched domains fix" broke the ppc64 build,
yielding error messages:

kernel/cpuset.c: In function 'update_cpu_domains':
kernel/cpuset.c:648: error: invalid lvalue in unary '&'
kernel/cpuset.c:648: error: invalid lvalue in unary '&'

On some arch's, the node_to_cpumask() is a function, returning
a cpumask_t.  But the for_each_cpu_mask() requires an lvalue mask.

The following patch fixes this build failure by making a copy
of the cpumask_t on the stack.

I have _not_ yet tried to build this for ppc64 - just for ia64.
I will try that now.  But the fix seems obvious enough that it
is worth sending out now.

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

Index: linux-2.6.13-cpuset-mempolicy-migrate/kernel/cpuset.c
===================================================================
--- linux-2.6.13-cpuset-mempolicy-migrate.orig/kernel/cpuset.c
+++ linux-2.6.13-cpuset-mempolicy-migrate/kernel/cpuset.c
@@ -645,7 +645,9 @@ static void update_cpu_domains(struct cp
 		int i, j;
 
 		for_each_cpu_mask(i, cur->cpus_allowed) {
-			for_each_cpu_mask(j, node_to_cpumask(cpu_to_node(i))) {
+			cpumask_t mask = node_to_cpumask(cpu_to_node(i));
+
+			for_each_cpu_mask(j, mask) {
 				if (!cpu_isset(j, cur->cpus_allowed))
 					return;
 			}

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

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Dinakar Guniguntala]

Paul,

Can we hold on to this patch for a while, as I reported yesterday,
this hangs up my ppc64 box on doing rmdir on a exclusive cpuset.
Still debugging the problem, hope to have a fix soon, Thanks

	-Dinakar


On Wed, Aug 24, 2005 at 04:15:10AM -0700, Paul Jackson wrote:
> As reported by Paul Mackerras <paulus@samba.org>, the previous
> patch "cpu_exclusive sched domains fix" broke the ppc64 build,
> yielding error messages:
> 
> kernel/cpuset.c: In function 'update_cpu_domains':
> kernel/cpuset.c:648: error: invalid lvalue in unary '&'
> kernel/cpuset.c:648: error: invalid lvalue in unary '&'
> 
> On some arch's, the node_to_cpumask() is a function, returning
> a cpumask_t.  But the for_each_cpu_mask() requires an lvalue mask.
> 
> The following patch fixes this build failure by making a copy
> of the cpumask_t on the stack.
> 
> I have _not_ yet tried to build this for ppc64 - just for ia64.
> I will try that now.  But the fix seems obvious enough that it
> is worth sending out now.
> 
> Signed-off-by: Paul Jackson <pj@sgi.com>
> 
> Index: linux-2.6.13-cpuset-mempolicy-migrate/kernel/cpuset.c
> ===================================================================
> --- linux-2.6.13-cpuset-mempolicy-migrate.orig/kernel/cpuset.c
> +++ linux-2.6.13-cpuset-mempolicy-migrate/kernel/cpuset.c
> @@ -645,7 +645,9 @@ static void update_cpu_domains(struct cp
>  		int i, j;
>  
>  		for_each_cpu_mask(i, cur->cpus_allowed) {
> -			for_each_cpu_mask(j, node_to_cpumask(cpu_to_node(i))) {
> +			cpumask_t mask = node_to_cpumask(cpu_to_node(i));
> +
> +			for_each_cpu_mask(j, mask) {
>  				if (!cpu_isset(j, cur->cpus_allowed))
>  					return;
>  			}
> 
> -- 
>                           I won't rest till it's the best ...
>                           Programmer, Linux Scalability
>                           Paul Jackson <pj@sgi.com> 1.650.933.1373
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Paul Jackson]

Dinakar wrote:
> Can we hold on to this patch for a while, as I reported yesterday,

Sure - though I guess it's Linus or Andrew who will have to do
the holding.

I sent it off contingent on the approval of yourself, Hawkes and Nick.

It looks like Linus is living dangerously and put it in already without
your approval. Hawkes reports of the oops if we didn't do something was
definitely motivating us to try something.

Good luck with your fix.  Let us know when you understand it.

I'm not quite sure how close Linus is to releasing 2.6.13, or
what would best serve his needs here.  The best I can do is push
on, be transparent, and let Linus holler if he has a better idea.

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

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Nick Piggin]

Paul Jackson wrote:
> Dinakar wrote:
> 
>>Can we hold on to this patch for a while, as I reported yesterday,
> 
> 
> Sure - though I guess it's Linus or Andrew who will have to do
> the holding.
> 
> I sent it off contingent on the approval of yourself, Hawkes and Nick.
> 

I get the feeling that the problem would be present in more
than just misaligned nodes.

On the POWER5 for example, Dinakar probably has SMT enabled,
which may cause something similar to pop up.

I get the feeling that exclusive cpusets should just be
completely disabled for 2.6.13, and John & Ingo's complete
fix put in 2.6.14-early

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Paul Jackson]

Nick wrote:
> I get the feeling that exclusive cpusets should just be
> completely disabled for 2.6.13

No no - not disable exclusive cpusets - disable using them to try
to define sched domains.

That is, I hope you mean that Dinakar's patch that uses cpu_exclusive
cpusets to define sched domains should be disabled for 2.6.13.  For
what they were worth (not a whole lot, granted), cpu_exclusive cpusets
have been in the kernel since 2.6.12.  They just didn't have any affect
in the placement of sched domains.  So long as they continue to do not
a whole lot, I see more risk than gain in disabling them for 2.6.13.

So long as the cpuset code stops making any calls to partition_sched_domains()
whatsoever, then we should be back where we were in 2.6.12, so far as the
scheduler is concerned - right?

I hope that the following (untested, unbuilt) patch, that I suggested
in my "Mon, 22 Aug 2005 13:38:23 -0700" message best meets you
suggestion above ... and I quote:

==========

The safest, mind numbingly simple thing to do that would avoid the oops
that Hawkes reported is to simply not have the cpuset code call the
code to setup a dynamic sched domain.  This is choice (2) above, and
could be done at the last hour with relative safety.

Here is an untested patch that does (2):

=====

Index: linux-2.6.13-cpuset-mempolicy-migrate/kernel/cpuset.c
===================================================================
--- linux-2.6.13-cpuset-mempolicy-migrate.orig/kernel/cpuset.c
+++ linux-2.6.13-cpuset-mempolicy-migrate/kernel/cpuset.c
@@ -627,6 +627,15 @@ static int validate_change(const struct 
  * Call with cpuset_sem held.  May nest a call to the
  * lock_cpu_hotplug()/unlock_cpu_hotplug() pair.
  */
+
+/*
+ * Hack to avoid 2.6.13 partial node dynamic sched domain bug.
+ * Disable letting 'cpu_exclusive' cpusets define dynamic sched
+ * domains, until the sched domain can handle partial nodes.
+ * Remove this ifdef hackery when sched domains fixed.
+ */
+#define DISABLE_EXCLUSIVE_CPU_DOMAINS 1
+#ifdef DISABLE_EXCLUSIVE_CPU_DOMAINS
 static void update_cpu_domains(struct cpuset *cur)
 {
 	struct cpuset *c, *par = cur->parent;
@@ -667,6 +676,11 @@ static void update_cpu_domains(struct cp
 	partition_sched_domains(&pspan, &cspan);
 	unlock_cpu_hotplug();
 }
+#else
+static void update_cpu_domains(struct cpuset *cur)
+{
+}
+#endif
 
 static int update_cpumask(struct cpuset *cs, char *buf)
 {


=====


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

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Nick Piggin]

Paul Jackson wrote:
> So long as the cpuset code stops making any calls to partition_sched_domains()
> whatsoever, then we should be back where we were in 2.6.12, so far as the
> scheduler is concerned - right?
> 

That's right - sorry I just meant disabling the dynamic sched
domains behaviour of the cpu_exclusive cpusets.

> I hope that the following (untested, unbuilt) patch, that I suggested
> in my "Mon, 22 Aug 2005 13:38:23 -0700" message best meets you
> suggestion above ... and I quote:
> 

I apologise, I missed that patch you sent. I think it looks OK,
and that it looks like what I was thinking about.

We need to revert to a stable behaviour, however we can't risk
major surgery to get there.

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Paul Jackson]

Nick wrote:
> and that it looks like what I was thinking about.

Ok - I almost have my crosstool installation healthy again.
I will actually see to it that my patch builds this time for
whatever arch's I can test on, and send this simple disabling
of sched domain mangling from cpuset-land as a real patch.

I have a couple other commitments - it will be 6 to 12 hours
before I send it in, unless someone asks for a half-baked
version sooner.

> We need to revert to a stable behaviour, however we can't risk
> major surgery to get there.

Yup.  Agreed.

Thanks for looking into this, Nick.

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

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Dinakar Guniguntala]

On Wed, Aug 24, 2005 at 01:31:07PM -0700, Paul Jackson wrote:
> ==========
> 
> The safest, mind numbingly simple thing to do that would avoid the oops
> that Hawkes reported is to simply not have the cpuset code call the
> code to setup a dynamic sched domain.  This is choice (2) above, and
> could be done at the last hour with relative safety.
> 
> Here is an untested patch that does (2):
> 
> =====
> 
> Index: linux-2.6.13-cpuset-mempolicy-migrate/kernel/cpuset.c
> ===================================================================
> --- linux-2.6.13-cpuset-mempolicy-migrate.orig/kernel/cpuset.c
> +++ linux-2.6.13-cpuset-mempolicy-migrate/kernel/cpuset.c
> @@ -627,6 +627,15 @@ static int validate_change(const struct 
>   * Call with cpuset_sem held.  May nest a call to the
>   * lock_cpu_hotplug()/unlock_cpu_hotplug() pair.
>   */
> +
> +/*
> + * Hack to avoid 2.6.13 partial node dynamic sched domain bug.
> + * Disable letting 'cpu_exclusive' cpusets define dynamic sched
> + * domains, until the sched domain can handle partial nodes.
> + * Remove this ifdef hackery when sched domains fixed.
> + */
> +#define DISABLE_EXCLUSIVE_CPU_DOMAINS 1
> +#ifdef DISABLE_EXCLUSIVE_CPU_DOMAINS
>  static void update_cpu_domains(struct cpuset *cur)
>  {
>  	struct cpuset *c, *par = cur->parent;
> @@ -667,6 +676,11 @@ static void update_cpu_domains(struct cp
>  	partition_sched_domains(&pspan, &cspan);
>  	unlock_cpu_hotplug();
>  }
> +#else
> +static void update_cpu_domains(struct cpuset *cur)
> +{
> +}
> +#endif
>  
>  static int update_cpumask(struct cpuset *cs, char *buf)
>  {
> 
> 
> =====
> 

I'll ack this for now until I fix the problems that I am seeing
on ppc64


	Acked-by: Dinakar Guniguntala <dino@in.ibm.com>

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[Paul Jackson]

Dinakar wrote:
> I'll ack this for now until I fix the problems that I am seeing
> on ppc64

Thanks, Dinakar.

Linus - do *NOT* actually apply the literal patch that Dinakar ack'd.

 1) It's logic is backwards - arrgh.
 2) It doesn't undo the other attempt to partially disable this.
 3) It's not a formally submitted and signed off patch from me, but
    just a (useful) topic of discussion.

I have a pair of patches running through crosstool on several arch's now.

    The first patch undoes the partial disable of the cpuset to sched
    domain connection that is in 2.6.13-rc7 now.

    The second patch does what Nick and Dinakar now agree is right -
    totally disabling the ability for exclusive cpusets to define
    sched domains in 2.6.13.

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

Re: [PATCH 2.6.13-rc6] cpu_exclusive sched domains build fix

[John Hawkes]

From: "Dinakar Guniguntala" <dino@in.ibm.com>
> Can we hold on to this patch for a while, as I reported yesterday,
> this hangs up my ppc64 box on doing rmdir on a exclusive cpuset.
> Still debugging the problem, hope to have a fix soon, Thanks

Paul's patch simply constrains the scope of cpuset configurations that will 
invoke the "dynamic sched domains" functionality, which means that some 
cpu-exclusive (a.k.a. "isolated") cpusets will continue to have the 
2.6.12-and-earlier behavior of being periodically examined by the CPU 
Scheduler in load-balancing activities.  That is, Paul's patch simply reverts 
cpuset/sched domain behavior to pre-2.6.13 status (for some cpusets).

The pre-2.6.13 non-"dynamic sched domains" behavior will in fact produce bad 
load-balancing behavior if a cpu-exclusive cpuset is so heavily loaded with 
executing processes, all pinned to the cpu(s) in the cpuset, that the other 
cpus in the system see this cpu(s)/node as the most-heavily-loaded and just 
focus on it during load-balancing -- which would be futile, of course, since 
the processes pinned to this highest-load cpu (and node) cannot be offloaded. 
Since load-balancing looks only at the most-heavily-loaded cpu as a cpu to 
offload, this means that all system load-balancing would be effectively turned 
off.

John Hawkes