Messages in this thread Patch in this message | | | Date | Sun, 09 Nov 2003 08:48:28 -0800 | From | "Martin J. Bligh" <> | Subject | Re: [PATCH] Fix find busiest queue 2.6.0-test9 |
| |
>> > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |