On Sat, 23 Jun 2007 00:20:36 +0200 Ingo Molnar <[EMAIL PROTECTED]> wrote:
> > * S.Çağlar Onur <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/