* Michael Neuling [off-list ref] [2010-06-28 16:11:06]:

[snip]

quoted

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.

quoted

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.

quoted

quoted

Other comments below.

[snip]

quoted

quoted

quoted

diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platfo

rms/

quoted

quoted

pseries/Kconfig

quoted

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 

quoted

quoted

quoted

+
+/* 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! 

quoted

quoted

+	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