Re: [PATCH] Fix find busiest queue 2.6.0-test9

[Martin J. Bligh]

>> > I ran it on the 16-way - no difference in performance. If the code is
>> > correct as was before (and I agree, it seems it was), perhaps it's just
>> > in need of a big fat comment to explain the confusion? ;-)
>> 
>> Ingo already dropped a fat comment ;) This is the relevant part:
>> 
>>  * We fend off statistical fluctuations in runqueue lengths by
>>  * saving the runqueue length during the previous load-balancing
>>  * operation and using the smaller one the current and saved lengths.
> 
> Well that was the comment that led me to make that patch. 
> 
> After discussion with mbligh it seems the confusion coming from me seeing
> ->prev_cpu_load
> as the load for that runqueue the last time we balanced; whereas it's actually 
> the load of the last runqueue checked during the balancing. 

Personally, I think the following makes the code more readable - if
several of us can't easily see what the code is doing, I think it's
a problem. I'm sure it makes me the very personification of Evil to 
suggest such a thing, but I don't care ;-) 

diff -urpN -X /home/fletch/.diff.exclude virgin/kernel/sched.c fbq_readable/kernel/sched.c
--- virgin/kernel/sched.c	Tue Sep  2 09:55:57 2003
+++ fbq_readable/kernel/sched.c	Sun Nov  9 08:44:39 2003
@@ -902,6 +902,14 @@ static inline unsigned int double_lock_b
 	return nr_running;
 }
 
+/* 
+ * macro to make the code more readable - this_rq->prev_cpu_load[i]
+ * is our local cached value of i's prev cpu_load. However, putting
+ * this_rq->prev_cpu_load into the code makes it read like it's the
+ * prev_cpu_load of this_cpu, which makes it confusing to read
+ */
+#define prev_cpu_load_cache(cpu) (this_rq->prev_cpu_load[cpu])
+
 /*
  * find_busiest_queue - find the busiest runqueue among the cpus in cpumask.
  */
@@ -932,10 +940,10 @@ static inline runqueue_t *find_busiest_q
 	 * that case we are less picky about moving a task across CPUs and
 	 * take what can be taken.
 	 */
-	if (idle || (this_rq->nr_running > this_rq->prev_cpu_load[this_cpu]))
+	if (idle || (this_rq->nr_running > prev_cpu_load_cache(this_cpu)))
 		nr_running = this_rq->nr_running;
 	else
-		nr_running = this_rq->prev_cpu_load[this_cpu];
+		nr_running = prev_cpu_load_cache(this_cpu);
 
 	busiest = NULL;
 	max_load = 1;
@@ -944,11 +952,11 @@ static inline runqueue_t *find_busiest_q
 			continue;
 
 		rq_src = cpu_rq(i);
-		if (idle || (rq_src->nr_running < this_rq->prev_cpu_load[i]))
+		if (idle || (rq_src->nr_running < prev_cpu_load_cache(i)))
 			load = rq_src->nr_running;
 		else
-			load = this_rq->prev_cpu_load[i];
-		this_rq->prev_cpu_load[i] = rq_src->nr_running;
+			load = prev_cpu_load_cache(i);
+		prev_cpu_load_cache(i) = rq_src->nr_running;
 
 		if ((load > max_load) && (rq_src != this_rq)) {
 			busiest = rq_src;

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/