Re: [RFC PATCHC 1/3] cpuidle: encapsulate power info in a separate structure

[Nicolas Pitre]

On Fri, 28 Mar 2014, Daniel Lezcano wrote:

> Hi Nicolas,
> 
> thanks for reviewing the patchset.
> 
> On 03/28/2014 07:17 PM, Nicolas Pitre wrote:
> > On Fri, 28 Mar 2014, Daniel Lezcano wrote:
> >
> >> The scheduler needs some information from cpuidle to know the timing for a
> >> specific idle state a cpu is.
> >>
> >> This patch creates a separate structure to group the cpuidle power info in
> >> order to share it with the scheduler. It improves the encapsulation of the
> >> code.
> >
> > Having cpuidle_power as a structure name, or worse, 'power' as a struct
> > member, is a really bad choice.
> 
> Yes, I was asking myself if this name was a good choice or not. I
> assumed 'power' could have been a good name because 'target_residency'
> is a time conversion of the power needed to enter this state.

Still, that's something the casual reviewer might not know.

And we ought to be careful when talking about power as well.  By 
definition, power means energy transferred per unit of time.  Sometimes 
we tend to say 'power' when we actually mean 'energy'.  With more "power 
aware" work going into the scheduler, it is better to disambiguate those 
terms.

> > Amongst the fields this struct
> > contains, only 1 out of 3 is about power.  The word "power" is already
> > abused quite significantly to mean too many different things already.
> >
> > I'd suggest something inspired by your own patch log message i.e.
> > 'struct cpuidle_info' instead, and use 'info' as a field name within
> > struct cpuidle_state.  Having 'params" instead of "info" could be a good
> > alternative too, although slightly longer.
> 
> Hmm 'info' or 'param' sound too vague. What about:
> 
> cpuidle_attr
> or
> cpuidle_property

As you wish.  As long as it isn't 'power'.


Nicolas
--
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/