Messages in this thread | | | Date | Mon, 25 Jun 2007 20:02:35 -0700 | From | Andrew Morton <> | Subject | Re: [patch] CFS scheduler, -v18 |
| |
On Sat, 23 Jun 2007 00:20:36 +0200 Ingo Molnar <mingo@elte.hu> wrote:
> > * S.Çağlar Onur <caglar@pardus.org.tr> wrote: > > > > kernel/sched.c:745:28: sched_idletask.c: No such file or directory > > > > Ahh and this happens with [1], grabbing sched_idletask.c from .18 one solves > > the problem... > > oops, indeed - i've fixed up the -git patch: > > http://people.redhat.com/mingo/cfs-scheduler/sched-cfs-v2.6.22-git-v18.patch >
So I locally generated the diff to take -mm up to the above version of CFS.
- sys_sched_yield_to() went away? I guess I missed that.
- Curious. the simplification of task_tick_rt() seems to go only halfway. Could do
if (p->policy != SCHED_RR) return;
if (--p->time_slice) return;
/* stuff goes here */
- dud macro: #define is_rt_policy(p) ((p) == SCHED_FIFO || (p) == SCHED_RR)
It evaluates its arg twice and could and should be coded in C.
There are a bunch of other don't-need-to-be-implemented-as-a-macro macros around there too. Generally, I suggest you review all the patchset for macros-which-don't-need-to-be-macros.
- Extraneous newline:
enum cpu_idle_type {
- Style thing:
struct sched_entity { struct load_weight load; /* for nice- load-balancing purposes */ int on_rq; struct rb_node run_node; unsigned long delta_exec; s64 delta_fair;
u64 wait_start_fair; u64 wait_start; u64 exec_start; u64 sleep_start, sleep_start_fair; u64 block_start; u64 sleep_max; u64 block_max; u64 exec_max; u64 wait_max; u64 last_ran;
s64 wait_runtime; u64 sum_exec_runtime; s64 fair_key; s64 sum_wait_runtime, sum_sleep_runtime; unsigned long wait_runtime_overruns, wait_runtime_underruns; };
I think the one-definition-per-line style is better than the `unsigned long foo,bar,zot,zit;' thing. Easier to read, easier to read subsequent patches and it leaves more room for a comment describing what the field does.
- None of these fields have comments describing what they do ;)
- __exit_signal() does apparently-unlocked 64-bit arith. Is there some implicit locking here or do we not care about the occasional race-induced inaccuracy?
(ditto, lots of places, I expect)
(Gee, there's shitloads of 64-bit stuff in there. Does it all _really_ need to be 64-bit on 32-bit?)
- weight_s64() (what does this do?) looks too big to inline on 32-bit.
- update_stats_enqueue() looks too big to inline even on 64-bit.
- Overall, this change is tremendously huge for something which is supposedly ready-to-merge. Looks like a lot of that is the sched_entity conversion, but afaict there's quite a lot besides.
- Should "4" in
(sysctl_sched_features & 4)
be enumerated?
- Maybe even __check_preempt_curr_fair() is too porky to inline.
- There really is an astonishing amount of 64-bit arith in here...
- Some opportunities for useful comments have been missed ;)
#define NICE_0_LOAD SCHED_LOAD_SCALE #define NICE_0_SHIFT SCHED_LOAD_SHIFT
<wonders what these mean>
- Should any of those new 64-bit arith functions in sched.c be pulled out and made generic?
- update_curr_load() is huge, inlined and has several callsites?
- lots more macros-which-dont-need-to-be-macros in sched.c: load_weight(), PRIO_TO_load_weight(), RTPRIO_TO_load_weight(), maybe others. People are more inclined to comment functions than they are macros, for some reason.
- inc_load(), dec_load(), inc_nr_running(), dec_nr_running(): these will generate plenty of code on 32-bit and they're all inlined with multiple callsites.
- overall, CFS takes sched.o from 41157 of .text up to 48781 on x86_64, which at 18% is rather a large bloat. Hopefully a lot of this is the new debug stuff.
- On i386 sched.o went from 33755 up to 43660 which is 29% growth. Possibly acceptable, but why did it increase a lot more than the x86_64 version? All that 64-bit arith, I assume?
- style (or the lack thereof):
p->se.sum_wait_runtime = p->se.sum_sleep_runtime = 0; p->se.sleep_start = p->se.sleep_start_fair = p->se.block_start = 0; p->se.sleep_max = p->se.block_max = p->se.exec_max = p->se.wait_max = 0; p->se.wait_runtime_overruns = p->se.wait_runtime_underruns = 0;
bit of an eyesore?
- in sched_init() this looks funny:
rq->ls.load_update_last = sched_clock(); rq->ls.load_update_start = sched_clock();
was it intended that these both get the same value?
- 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/
| |