Messages in this thread | | | From | Nikhil Rao <> | Date | Wed, 18 May 2011 12:39:55 -0700 | Subject | Re: [PATCH v2 3/3] sched: increase SCHED_LOAD_SCALE resolution |
| |
On Wed, May 18, 2011 at 11:25 AM, Ingo Molnar <mingo@elte.hu> wrote: > > * Nikhil Rao <ncrao@google.com> wrote: > >> -#define SCHED_LOAD_SHIFT 10 >> +#if BITS_PER_LONG > 32 >> +#define SCHED_LOAD_RESOLUTION 10 >> +#define scale_load(w) (w << SCHED_LOAD_RESOLUTION) >> +#define scale_load_down(w) (w >> SCHED_LOAD_RESOLUTION) >> +#else >> +#define SCHED_LOAD_RESOLUTION 0 >> +#define scale_load(w) w >> +#define scale_load_down(w) w >> +#endif > > Please use the: > > #if X > # define Y A > #else > # define Y B > #endif > > type of nested CPP branching style. > > Also, the 'w' probably wants to be '(w)', just in case.
Thanks for the review. Will fix the macros.
> >> if (!lw->inv_weight) { >> - if (BITS_PER_LONG > 32 && unlikely(lw->weight >= WMULT_CONST)) >> + unsigned long w = scale_load_down(lw->weight); >> + if (BITS_PER_LONG > 32 && unlikely(w >= WMULT_CONST)) >> lw->inv_weight = 1; > > Please separate local variable declarations and the first statement following > it by an extra empty line. >
Yes, will do.
> Could you also put all the performance measurement description into the > changelog of this third commit - so that people can see (and enjoy) the > extensive testing you have done on this topic? >
Yes, will add the performance changes to the commit description.
>> /* >> - * Increase resolution of nice-level calculations: >> + * Increase resolution of nice-level calculations for 64-bit architectures. >> */ >> -#define SCHED_LOAD_SHIFT 10 >> +#if BITS_PER_LONG > 32 >> +#define SCHED_LOAD_RESOLUTION 10 >> +#define scale_load(w) (w << SCHED_LOAD_RESOLUTION) >> +#define scale_load_down(w) (w >> SCHED_LOAD_RESOLUTION) >> +#else >> +#define SCHED_LOAD_RESOLUTION 0 >> +#define scale_load(w) w >> +#define scale_load_down(w) w >> +#endif > > Please also be a bit more verbose in the comment above why we treat 64-bit > architectures differently - it's not obvious.
Added a more descriptive comment.
Thanks for the review. I'll refresh the patch and send it with the fixes.
-Thanks, Nikhil -- 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/
| |