Re: [PATCH v6 6/7] sched: add SD_BALANCE_NEWIDLE at MC and CPUlevel for sched_mc>0

[Ingo Molnar]

* Vaidyanathan Srinivasan <svaidy@xxxxxxxxxxxxxxxxxx> wrote:

> * Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> [2008-12-17 17:42:54]:
> 
> > On Wed, 17 Dec 2008 22:57:38 +0530
> > Vaidyanathan Srinivasan <svaidy@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>,
        linux-kernel@xxxxxxxxxxxxxxx, suresh.b.siddha@xxxxxxxxx,
        venkatesh.pallipadi@xxxxxxxxx, a.p.zijlstra@xxxxxxxxx,
        mingo@xxxxxxx, dipankar@xxxxxxxxxx, balbir@xxxxxxxxxxxxxxxxxx,
        vatsa@xxxxxxxxxxxxxxxxxx, ego@xxxxxxxxxx, andi@xxxxxxxxxxxxxx,
        davecb@xxxxxxx, tconnors@xxxxxxxxxxxxxxxxx, maxk@xxxxxxxxxxxx,
        gregory.haskins@xxxxxxxxx, pavel@xxxxxxx

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/