lkml.org 
[lkml]   [2007]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2007-06-26 05:07    [W:0.083 / U:0.260 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site