lkml.org 
[lkml]   [2008]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 6/7] sched: add SD_BALANCE_NEWIDLE at MC and CPU level for sched_mc>0

* Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> wrote:

> * Andrew Morton <akpm@linux-foundation.org> [2008-12-17 17:42:54]:
>
> > On Wed, 17 Dec 2008 22:57:38 +0530
> > Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> wrote:
> >
> > > --- a/include/linux/sched.h
> > > +++ b/include/linux/sched.h
> > > @@ -782,6 +782,16 @@ enum powersavings_balance_level {
> > > ((sched_mc_power_savings || sched_smt_power_savings) ? \
> > > SD_POWERSAVINGS_BALANCE : 0)
> >
> > What's with all the crappy macros in here?
>
> Hi Andrew,
>
> These macros set the SD_POWERSAVINGS_BALANCE flag based on the
> sysfs tunable.
>
> > > +/*
> > > + * Optimise SD flags for power savings:
> > > + * SD_BALANCE_NEWIDLE helps agressive task consolidation and power savings.
> > > + * Keep default SD flags if sched_{smt,mc}_power_saving=0
> > > + */
> > > +
> > > +#define POWERSAVING_SD_FLAGS \
> > > + ((sched_mc_power_savings || sched_smt_power_savings) ? \
> > > + SD_BALANCE_NEWIDLE : 0)
> >
> > This one purports to be a constant, but it isn't - it's code.
> >
> > It would be cleaner, clearer and more idiomatic to do
> >
> > static inline int powersaving_sd_flags(void)
> > {
> > ...
> > }
>
> Your are suggesting to move these to inline functions. I will write
> a patch and post for review.
>
> > Also, doing (sched_mc_power_savings | sched_smt_power_saving) might
> > save a branch.
> >
> > > #define test_sd_parent(sd, flag) ((sd->parent && \
> > > (sd->parent->flags & flag)) ? 1 : 0)
> >
> > buggy when passed an expression with side-effects. Doesn't need to be
> > implemented as a macro.
>
> Agreed, but these macros are used throughout sched.c and are performance
> sensitive. Inline functions are a close enough replacement for the
> macro let me look for any performance penalty as well and report.

those macros are historic so feel free to convert them to inlines without
re-measuring performance impact.

The patchset looks pretty good in principle otherwise, so if you could
address Andrew's comments and clean up those bits in the next iteration we
could start testing it in the scheduler tree. (Please also add Balbir
Singh's acks to the next iteration.)

and please fix your mailer to not inject stuff like this:

Mail-Followup-To: Andrew Morton <akpm@linux-foundation.org>,
linux-kernel@vger.kernel.org, suresh.b.siddha@intel.com,
venkatesh.pallipadi@intel.com, a.p.zijlstra@chello.nl,
mingo@elte.hu, dipankar@in.ibm.com, balbir@linux.vnet.ibm.com,
vatsa@linux.vnet.ibm.com, ego@in.ibm.com, andi@firstfloor.org,
davecb@sun.com, tconnors@astro.swin.edu.au, maxk@qualcomm.com,
gregory.haskins@gmail.com, pavel@suse.cz

It utterly messed up the addressing mode of my reply here and i had to
edit the raw email headers manually to fix it up ;-)

Ingo


\
 
 \ /
  Last update: 2008-12-18 13:49    [W:0.064 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site