Re: 2.6.12-rc6-mm1

[Con Kolivas <kernel@kolivas.org>]

On Sat, 11 Jun 2005 09:14, J.A. Magallon wrote:
> On 06.10, Con Kolivas wrote:
> > The priority biasing was off by mutliplying the total load by the total
> > priority bias and this ruins the ratio of loads between runqueues. This
> > patch should correct the ratios of loads between runqueues to be
> > proportional to overall load.
>
> 2.6.12-rc6-mm1 + this patch just oopses nicely on boot.
> I did not had a digital camera handy, but the first oops that fit in the
> screen was this call chain:
>
> kernel_thread_helper
> init
> init
> do:base_setup
> usermodehelper_init
> __create_workqueue
> EIP in try_to_wake_up
>
> After this, there was another with some do_div_error calls...
>
> Something looks un-initialized the first time, or the integer arithmetic
> is wrong. I really dont like a*(b/c), I really prefer (a*b)/c. It is more
> common b/c == 0 (because b<c), than the possibility of overflowing (a*b).
>
> So I tried both. With this, it boots again:

Doh Doh DOH DOH!
I need a real swift kick up the bum. The point of the patch was to show what 
was wrong with the math, and I shouldn't have posted it without actually 
trying it.

> -		unsigned long prio_bias = rq->prio_bias / rq->nr_running;

rq->nr_running can often be 0 and rq->prio_bias by definition has to be larger 
than or equal to rq->nr_running.

> Perhaps this:
>
> 	if (idle == NOT_IDLE || rq->nr_running > 1)
>
> should be
>
> 	if (idle == NOT_IDLE && rq->nr_running > 1)

No, testing for rq->nr_running > 1 is only needed if we are balancing in an 
idle balance.

> Hope this helps, thanks.

Yes it does :\

Here is what the patch _should_ have been. (*same warnings with this patch 
about math demonstration and untested as should have been posted with the 
earlier one*)

Con

The priority biasing was off by mutliplying the total load by the total 
priority bias and this ruins the ratio of loads between runqueues. This
patch should correct the ratios of loads between runqueues to be proportional
to overall load. -2nd attempt.

Signed-off-by: Con Kolivas <kernel@kolivas.org>

Index: linux-2.6.12-rc6-mm1/kernel/sched.c
===================================================================
--- linux-2.6.12-rc6-mm1.orig/kernel/sched.c	2005-06-10 23:56:56.000000000 +1000
+++ linux-2.6.12-rc6-mm1/kernel/sched.c	2005-06-11 09:55:56.000000000 +1000
@@ -978,7 +978,8 @@ static inline unsigned long __source_loa
 	else
 		source_load = min(cpu_load, load_now);
 
-	if (idle == NOT_IDLE || rq->nr_running > 1)
+	if (idle == NOT_IDLE || rq->nr_running > 1) {
+		unsigned long prio_bias = 1;
 		/*
 		 * If we are busy rebalancing the load is biased by
 		 * priority to create 'nice' support across cpus. When
@@ -987,7 +988,10 @@ static inline unsigned long __source_loa
 		 * prevent idle rebalance from trying to pull tasks from a
 		 * queue with only one running task.
 		 */
-		source_load *= rq->prio_bias;
+		if (rq->nr_running)
+			prio_bias = rq->prio_bias / rq->nr_running;
+		source_load *= prio_bias;
+	}
 
 	return source_load;
 }
@@ -1011,8 +1015,13 @@ static inline unsigned long __target_loa
 	else
 		target_load = max(cpu_load, load_now);
 
-	if (idle == NOT_IDLE || rq->nr_running > 1)
-		target_load *= rq->prio_bias;
+	if (idle == NOT_IDLE || rq->nr_running > 1) {
+		unsigned long prio_bias = 1;
+
+		if (rq->nr_running)
+			prio_bias = rq->prio_bias / rq->nr_running;
+		target_load *= prio_bias;
+	}
 
 	return target_load;
 }

Attachment: pgpTrSoZt5U8J.pgp
Description: PGP signature