* Michael Neuling <mi...@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
