Re: [PATCH v3 2/2] powerpc: add support for new hcall H_BEST_ENERGY

[Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>]

* Michael Neuling <mikey@neuling.org> [2010-06-28 16:11:06]:

[snip]

> > These hints are abstract number given by the hypervisor based on
> > the extended knowledge the hypervisor has regarding the current system
> > topology and resource mappings.
> > 
> > The activate and the deactivate part is for the two distinct
> > operations that we could do for energy savings.  When we have more
> > capacity than required, we could deactivate few core to save energy.
> > The choice of the core to deactivate will be based on
> > /sys/devices/system/cpu/deactivate_hint_list.  The comma separated
> > list of cpus (cores) will be the preferred choice.
> > 
> > Once the system has few deactivated cores, based on workload demand we
> > may have to activate them to meet the demand.  In that case the
> > /sys/devices/system/cpu/activate_hint_list will be used to prefer the
> > core in-order among the deactivated cores.
> > 
> > In simple terms, activate_hint_list will be null until we deactivate
> > few cores.  Then we could look at the corresponding list for
> > activation or deactivation.
> 
> Can you put these details in the code and in the check-in comments.

Hi Mikey,

I have added these in the -v4 post.

> > Regarding your second point, there is a reason for both a list and
> > per-cpu interface.  The list gives us a system wide list of cores in
> > one shot for userspace to base their decision.  This will be the
> > preferred interface for most cases.  On the other hand, per-cpu file
> > /sys/device/system/cpu/cpuN/pseries_(de)activate_hint provide more
> > information since it exports the hint value as such.
> > 
> > The idea is that the list interface will be used to get a suggested
> > list of cores to manage, while the per-cpu value can be used to
> > further get fine grain information on a per-core bases from the
> > hypervisor.  This allows Linux to have access to all information that
> > the hypervisor has offered through this hcall interface.
> 
> OK, I didn't realise that they contained different info.  Just more
> reasons that this interface needs better documentation :-)
> 
> Overall, I'm mostly happy with the interface.  It's pretty light weight.

these too.

> > > Other comments below.
> > > 

[snip]

> > > > diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platfo
> rms/
> > > pseries/Kconfig
> > > > index c667f0f..b3dd108 100644
> > > > --- a/arch/powerpc/platforms/pseries/Kconfig
> > > > +++ b/arch/powerpc/platforms/pseries/Kconfig
> > > > @@ -33,6 +33,16 @@ config PSERIES_MSI
> > > >         depends on PCI_MSI && EEH
> > > >         default y
> > > > 
> > > > +config PSERIES_ENERGY
> > > 
> > > Probably need a less generic name.  PSERIES_ENERGY_MANAGEMENT?
> > > PSERIES_ENERGY_HOTPLUG_HINTS?
> >
> > PSERIES_ENERGY_MANAGEMENT may be good but too long for a config
> > option.
> > 
> > The idea is to collect all energy management functions in this module
> > as and when new features are introduced in the pseries platform.  This
> > hcall interface is the first to be included, but going forward in
> > future I do not propose to have different modules for other energy
> > management related features.
> > 
> > The name is specific enough for IBM pseries platform and energy
> > management functions and enablements.  Having less generic name below
> > this level will make it difficult to add all varieties of energy
> > management functions in future.
> 
> OK, I thought this might be the case but you never said.  Please say
> something like "This adds CONFIG_PSERIES_ENERGY which will be used for
> future power saving code" or some such.

I already had this comment in the patch description.  Did not want add
a comment in the Kconfig file as the CONFIG_ prefix is assumed in each
of the 

> > > > +
> > > > +/* Helper Routines to convert between drc_index to cpu numbers */
> > > > +
> > > > +static u32 cpu_to_drc_index(int cpu)
> > > > +{
> > > > +	struct device_node *dn = NULL;
> > > > +	const int *indexes;
> > > > +	int i;
> > > > +	dn = of_find_node_by_path("/cpus");
> > > > +	if (dn == NULL)
> > > > +		goto err;
> > > 
> > > Humm, I not sure this is really needed.  If you don't have /cpus you are
> > > probably not going to boot.
> > 
> > Good suggestion.  I could add all these checks in module_init.   I was
> > think if any of the functions being called is allocating memory and in
> > case they fail, we need to abort.
> > 
> > I just reviewed and look like of_find_node_by_path() will not sleep or
> > allocate any memory.  So if it succeeds once in module_init(), then it
> > will never fail! 
> > 
> > 
> > > > +	indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> > > > +	if (indexes == NULL)
> > > > +		goto err;
> > > 
> > > These checks should probably be moved to module init rather than /sfs
> > > read time.  If they fail, don't load the module and print a warning.  
> > > 
> > > These HCALLS and device-tree entire aren't going to be dynamic.
> > 
> > Agreed.  Only cause of runtime failure is OOM.  If none of these
> > allocate memory, moving these checks once at module_init() will be
> > a good optimization.
> 
> Cool, thanks.  

Hey, I did not yet remove the failure checks in this iteration.  I did
not have these checks earlier and Ben has suggested to check at each
call.  I will discuss with him about moving all checks to
module_init() time and then remove these.

--Vaidy