[PATCH] cpuset and sched domains: sched_load_balance flag fixes

[PATCH] cpuset and sched domains: sched_load_balance flag fixes

[Paul Jackson]

From: Paul Jackson <pj@sgi.com>

Thanks to Randy Dunlap for the review that caught
some of the following.

Some bug fixes and coding style fixes:
 1) only one statement per line, please.
 2) don't need to guard kfree() calls with a NULL check
 3) use kfifo_free, not kfree, if it came from kfifo_alloc
 4) a pair of curly brackets got lost along the way
 5) missing .read, .write callbacks for sched_load_balance

Without (3), one kfifo buffer memory was leaked each time
one rebuilt scheduler domains

Without (4), the current task was summarily killed each
time one tried to rebuild scheduler domains

Without (5), every read or write system call on a per-cpuset
special file 'sched_load_balance' failed, EINVAL.

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

---

Andrew,

  These fixes go right after the patch they fix:
    [PATCH] cpuset and sched domains: sched_load_balance flag

 kernel/cpuset.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

--- 2.6.23-rc8-mm1.orig/kernel/cpuset.c	2007-10-02 21:29:51.245156939 -0700
+++ 2.6.23-rc8-mm1/kernel/cpuset.c	2007-10-02 21:26:56.000000000 -0700
@@ -579,7 +579,9 @@ static void rebuild_sched_domains(void)
 	int ndoms;		/* number of sched domains in result */
 	int nslot;		/* next empty doms[] cpumask_t slot */
 
-	q = NULL; csa = NULL; doms = NULL;
+	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)) {
@@ -604,9 +606,10 @@ static void rebuild_sched_domains(void)
 		struct cpuset *child;   /* scans child cpusets of cp */
 		if (is_sched_load_balance(cp))
 			csa[csn++] = cp;
-		list_for_each_entry(cont, &cp->css.cgroup->children, sibling)
+		list_for_each_entry(cont, &cp->css.cgroup->children, sibling) {
 			child = cgroup_cs(cont);
 			__kfifo_put(q, (void *)&child, sizeof(cp));
+		}
   	}
 
 	for (i = 0; i < csn; i++)
@@ -668,9 +671,8 @@ rebuild:
 
 done:
 	if (q && !IS_ERR(q))
-		kfree(q);
-	if (csa)
-		kfree(csa);
+		kfifo_free(q);
+	kfree(csa);
 	/* Don't kfree(doms) -- partition_sched_domains() does that. */
 }
 
@@ -1358,6 +1360,8 @@ static struct cftype cft_mem_exclusive =
 
 static struct cftype cft_sched_load_balance = {
 	.name = "sched_load_balance",
+	.read = cpuset_common_file_read,
+	.write = cpuset_common_file_write,
 	.private = FILE_SCHED_LOAD_BALANCE,
 };
 

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

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

[Andrew Morton]

On Wed, 03 Oct 2007 04:44:01 -0700
Paul Jackson <pj@sgi.com> wrote:

> From: Paul Jackson <pj@sgi.com>
> 
> Thanks to Randy Dunlap for the review that caught
> some of the following.
> 
> Some bug fixes and coding style fixes:
>  1) only one statement per line, please.
>  2) don't need to guard kfree() calls with a NULL check
>  3) use kfifo_free, not kfree, if it came from kfifo_alloc
>  4) a pair of curly brackets got lost along the way
>  5) missing .read, .write callbacks for sched_load_balance
> 
> Without (3), one kfifo buffer memory was leaked each time
> one rebuilt scheduler domains
> 
> Without (4), the current task was summarily killed each
> time one tried to rebuild scheduler domains
> 
> Without (5), every read or write system call on a per-cpuset
> special file 'sched_load_balance' failed, EINVAL.
> 
> Signed-off-by: Paul Jackson <pj@sgi.com>
> 
> ---
> 
> Andrew,
> 
>   These fixes go right after the patch they fix:
>     [PATCH] cpuset and sched domains: sched_load_balance flag

I'm getting 100% rejects from this - probably whatever patch it is
patching got lost or unrecognisably mangled.

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

[Paul Jackson]

Andrew wrote:
> I'm getting 100% rejects from this - probably whatever patch it is
> patching got lost or unrecognisably mangled.

Yup - so far as I can tell, you didn't pick up the base patch that
this current patch fixes.  So, no, I wouldn't expect this patch to
make any sense.

As I stated in this current patch in the diffstat section after the
'---' marker, this current patch applies to the base patch of Subject:

  [PATCH] cpuset and sched domains: sched_load_balance flag

You probably didn't pick up that base patch because Nick and I are
still haggling over it.  Well ... we've agreed, but I can't get the
scheduler to work as I thought Nick wanted, and I'm still waiting
to hear from Nick again.

==> So yeah - throw this current patch out.

Nick ... you there ?

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

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

[Ingo Molnar]

* Paul Jackson <pj@sgi.com> wrote:

> Yup - so far as I can tell, you didn't pick up the base patch that 
> this current patch fixes.  So, no, I wouldn't expect this patch to 
> make any sense.
> 
> As I stated in this current patch in the diffstat section after the 
> '---' marker, this current patch applies to the base patch of Subject:
> 
>   [PATCH] cpuset and sched domains: sched_load_balance flag
> 
> You probably didn't pick up that base patch because Nick and I are 
> still haggling over it.  Well ... we've agreed, but I can't get the 
> scheduler to work as I thought Nick wanted, and I'm still waiting to 
> hear from Nick again.

please resend the base patch to Andrew (or better yet, a combo patch 
that Andrew can apply and which just does it all).

We've agreed on the external API and that is what matters for now. The 
internal interfacing between the scheduler and cpusets can/could be 
improved later on - and your original patch is reasonable to begin with. 
So lets just move on and do this.

	Ingo

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

[Paul Jackson]

Ingo wrote:
> please resend the base patch to Andrew (or better yet, a combo patch 
> that Andrew can apply and which just does it all).

I'm reluctant to right now, because I will be off the air
for three days, starting in one more day, and don't like
firing off stuff just before I vanish.

Though I'm aware that time is short for 2.6.24 ... not
sure how best to handle this.

My plan had been to send Andrew something on Wednesday
next week (five days from now), either what we have now,
or with the improved cpuset-to-sched API, if Nick and I
can work through that (well, if Nick can figure out what
I broke, while I'm offline.)

Andrew or Ingo - you got any better plan?

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

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

[Ingo Molnar]

* Paul Jackson <pj@sgi.com> wrote:

> My plan had been to send Andrew something on Wednesday
> next week (five days from now), either what we have now,
> or with the improved cpuset-to-sched API, if Nick and I
> can work through that (well, if Nick can figure out what
> I broke, while I'm offline.)
> 
> Andrew or Ingo - you got any better plan?

i'd suggest we go with what we have now (i've run it for some reasonable 
time on various systems and it caused no breakages) - could you resend 
that? We cannot merge it via sched.git because it has some containers 
(or other?) dependencies, right?

	Ingo

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

[Paul Jackson]

> i'd suggest we go with what we have now

Ok - I'll try sending this against 2.6.23-rc8-mm2
in a couple of hours, if it goes well.

> We cannot merge it via sched.git because it has some containers 
> (or other?) dependencies, right?

This sched_load_balance flag patch collides with the cgroup
(aka container) patches if I'm not careful, yes.

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