CPUFreq addon in HAL

[David Zeuthen]

Hi,

Sorry for the lag, been traveling, busy with work and a bit sick.

On Wed, 2006-07-05 at 16:49 +0200, Holger Macht wrote:
> Hi,
> 
> CPU frequency scaling is the last remaining important power management
> technology that should be controllable from the desktop IMO, but that's
> not possible yet. Or there's at least no common interface/ software
> everybody agrees on.
> 
> Richard already proposed something similar in the past. His idea was to
> control the different frequencies a CPU supports from higher level
> applications on the desktop. His proposal was rejected, though.
> 
> So I'm doing a second apprach and I've got a different way of solving the
> problem. It's implemented as a HAL addon which summarizes all cpufreq
> capable CPUs into six DBus methods on the
> org.freedesktop.Hal.Device.SystemPowerManagement interface.

I think this makes lots of sense.

> Addon-cpufreq supports kernel governors and also implements a userspace
> controlling mechanism. That makes it the "all you need for CPUFreq"
> application. Furthermore, to not make things unnecessarily complicated for
> desktop applications, it is supposed to abstract all the different
> settings you can make for the different governors. One thing which isn't
> available yet, is the possibility to control the performance of
> dynamically scaled CPUs. 

Yea, probably we want a D-BUS interface on each device object
representing an actual CPU which exports

 void SetIgnoreGovernor(bool ignoreGovernor)
 bool GetIgnoreGovernor()
 void SetCPUFrequency(int FreqInMHz)

or something? Maybe use hal properties for cpu frequency too? Or maybe
not.

> There's a method to set the performance (from 1
> to 100) which can be easily used from other applications with showing a
> nice progress bar or the like. That makes it unique in contrast to former
> solutions.

So the abstraction is a slider "how much performance do you want"? I'm
assuming this is dependent on the governor you choose.

Perhaps you could elaborate what the difference is between setting the
slider to 1 versus 25 versus 50 versus 75 versus 100 for different
governors? What's the user experience when changing it? 

(I could read the code I guess... but...)

Some people claim that _the_ litmus test when introducing a setting in
the UI is that if the users can't figure out how the system will react
when changing the setting... it probably shouldn't be there.. 

I'm not saying we shouldn't have such a setting (in fact I believe we
probably should, hence my long reply below) in any UI.. I'm just curious
what the semantics of this 1-100 variable integer is.

> The biggest advantage is that you get CPUFreq out of the box on every
> system supporting HAL without the need to install another daemon. Another
> one is that desktop applications like gnome-power-manager or kpowersave
> can use one common interface after all.

So how does this interact in the event there is an existing daemon on
the system such as e.g. cpuspeed or powernowd? That will be the case on
many systems when they upgrade to a HAL version that includes this? I'm
just curious about possible interferences and/or resonance and whether
we should advise packagers to say e.g.

 Conflicts: cpuspeed
 Conflicts: powernowd

when updating their HAL spec file.

(my hope here is that they can coexist at least until someone pokes your
methods in the addon)

> It claims the following DBus interfaces on the
> /org/freedesktop/Hal/devices/computer device:
> 
> 
> Set a specific CPU frequency governor
> -------------------------------------
>   Interface: org.freedesktop.Hal.Device.SystemPowerManagement
>   Member   : "SetCPUFreqGovernor"
>   Type     : Method call with return
> 
>   Param:
>     string : The governor to be set. This can be any arbitary string. Also
> 	     governors which are not known can be set
> 
>   Return type:
>     int    : 0 on success, 1 on error

Maybe throw an exception on error? Exceptions are more flexible since
the user won't have to remember what the integer means and you can grow
this as needed (it's somewhat like an enum in C). The only exception I
can think of here is org.freedesktop.Hal.Device.SystemPowerManagement.
UnknownGovernor.

Suggest also to return a boolean with TRUE only on success although we
could get away with a void. 

Also you want to use PolicyKit here and introduce a privilege descriptor
for whether the user is privileged (see tools/hal-storage-mount.c for
details or just ask on the list and/or IRC). 

Specifically you want throw a org.freedesktop.Hal.Device.
SystemPowerManagement.PermissionDenied exception with the name of the
privilege as the first word in the detail. This way applications
catching that error can deal with it and, optionally, use
libpolkit-grant to get the privilege.

> Set the performance independently from the current governor
> -----------------------------------------------------------
>   Interface: org.freedesktop.Hal.Device.SystemPowerManagement
>   Member   : "SetCPUFreqPerformance"
>   Type     : Method call with return
> 
>   Param:
>     int    : Value between 1 and 100. The higher the value the more
> 	     performance you get. 50 is default and should be sufficient
> 	     in most cases
> 
>   Return type:
>     int    : 0 on success, 1 on error or if performance settings are not
> 	     supported for the current governor

Same comments about exceptions vs. return value.

> Set if niced processes should be consider when calculating CPU load
> --------------------------------------------------------------------
>   Interface: org.freedesktop.Hal.Device.SystemPowerManagement
>   Member   : "SetCPUFreqConsiderNice"
>   Type     : Method call with return
> 
>   Param:
>     boolean: True if niced processes should be considered, false otherwise
> 
>   Return type:
>     int    : 0 on success, 1 on error or if this setting is not supported
> 	     with the current governor

Same comments about exceptions vs. return value.

> Get the current active governor
> -------------------------------
>   Interface: org.freedesktop.Hal.Device.SystemPowerManagement
>   Member   : "GetCPUFreqGovernor"
>   Type     : Method call with return
> 
>   Return type:
>     string : The current active governor

Perhaps this should be a hal property instead? Maybe not if it can
change underneath us.

> Get the current performance setting
> -----------------------------------
>   Interface: org.freedesktop.Hal.Device.SystemPowerManagement
>   Member   : "GetCPUFreqPerformance"
>   Type     : Method call with return
> 
>   Return type:
>     int    : The current performance setting from 1 to 100

Perhaps this should be a hal property instead? Maybe not if it can
change underneath us.

> Get the current consider nice setting
> -------------------------------------
>   Interface: org.freedesktop.Hal.Device.SystemPowerManagement
>   Member   : "GetCPUFreqConsiderNice"
>   Type     : Method call with return
> 
>   Return type:
>     boolean: True if niced processes are considered, false otherwise

Perhaps this should be a hal property instead? Maybe not if it can
change underneath us.

How about a function to get a list of available governors?

> The addon basically consists of five parts:
> 
>   1. One interface which contains all methods which are common for all
>      governors (see DBus interfaces above)
> 
>   2. Some helper functions
> 
>   3. The userspace governor implementation with the CPULoad calculation
>      mechanism
> 
>   4. The ondemand governor implementation
> 
>   5. DBus/HAL integration
> 
> Current addons only consist of one source file. However, I don't think
> that makes sense for addon-cpufreq. If this would be a independent
> project, I would have separated these parts into more different files. But
> for now, I've just put the userspace part into two files (header, source)
> and the remaining parts into another two files (header, source). It would
> of course be possible to only have one source file and thus get rid of the
> header files if this is desired.
> 
> I've appended a somewhat patch to accomplish all this. Please comment...
> 
> If you guys don't agree that this could be added to HAL as an addon, maybe
> the above mentioned interfaces can be implemented by installing some
> scripts like hal-system-power-cpufreq-set-governor. Then I'm trying to get
> org.freedesktop.CPUFreq for a separate daemon and can call the DBus
> methods inside them. All other people can then of course do whatever they
> like in those scripts.

Personally I think it makes sense to add. I'm unsure how this is handled
on FreeBSD and Solaris? It looks to me as the interface is sufficiently
abstract.

Also, if you can send a patch for the hal spec detailing all the methods
and the exceptions they throw it would be great. For example, the
PermissionDenied exception with the constraint that the first word of
the detail of the exception is the name of the privilege needed is as a
matter of fact part of the ABI. [1]

[1] : Unfortunately I've failed at convincing the other D-BUS developers
that the D-BUS introspection data format should be amended to list what
exceptions the code may throw.

Comments on the patch


> +int get_cpu_count(void)

Maybe use sysconf(3) here?

> +static void dbus_add_method(LibHalContext *halctx, const char *method,
> +				const char *signature)
> +{
> +	libhal_device_property_strlist_append(halctx,
> +		      "/org/freedesktop/Hal/devices/computer",
> +		      "org.freedesktop.Hal.Device.SystemPowerManagement.method_names",
> +		      method,
> +		      NULL);
> +	libhal_device_property_strlist_append(halctx,
> +		      "/org/freedesktop/Hal/devices/computer",
> +		      "org.freedesktop.Hal.Device.SystemPowerManagement.method_signatures",
> +		      signature,
> +		      NULL);
> +}

is used by

> +		dbus_add_method(halctx, "SetCPUFreqGovernor", "s");
> +		dbus_add_method(halctx, "SetCPUFreqPerformance", "i");
> +		dbus_add_method(halctx, "SetCPUFreqConsiderNice", "b");
> +		dbus_add_method(halctx, "GetCPUFreqGovernor", "");
> +		dbus_add_method(halctx, "GetCPUFreqPerformance", "");
> +		dbus_add_method(halctx, "GetCPUFreqConsiderNice", "");

This shouldn't be necessary.


> +	dbus_bus_add_match(dbus_connection,
> +			   "type='method_call',"
> +			   "interface='" DBUS_INTERFACE "',", NULL);

Pretty sure this isn't needed either.

Cheers,
David