[PATCH 1/3] CPU frequency scaling addon

[Holger Macht]

On Mon 14. Aug - 14:03:52, Dave Jones wrote:
> On Mon, Aug 14, 2006 at 05:58:55PM +0200, Holger Macht wrote:
> 
>  > > Thanks, this is better. Will change at corresponding places.
>  > 
>  > Done. New patch attached.
> 
> +/** @brief set a speed with traversing all intermediary speeds */
> +static int set_speed(struct userspace_interface *iface, int target_speed)
> +{
> +	int delta;
> +	int current_speed = iface->current_speed;
> +
> +	if (current_speed == target_speed)
> +		return -1;
> +
> +	if (current_speed > target_speed)
> +		delta = -1;
> +	else
> +		delta = 1;
> +
> +	do {
> +		current_speed += delta;
> +		write_speed(g_a_i(iface->speeds_kHz, current_speed), iface->base_cpu);
> +	} while (current_speed != target_speed);
> +
> +	return current_speed;
> +}
> 
> This looks seriously nasty.  If we're running at 2GHz and have a
> target of 800MHz, that's 1200 writes to sysfs we're going to do,
> which is just insane.

That's not the case.

> 
> The kernel exports the available steps for a reason, so that you
> don't have to guess at speeds like this.

The array iface->speeds_kHz only contains the steps exported by the
kernel. The current_speed variable is only used as the index of the array.

> 
> 
> +/** writes the new_governor string into the sysfs interface */ 
> +gboolean write_governor(char *new_governor, int cpu_id)
> +{
> +	gboolean	ret		= TRUE;
> +	char		*governor_file  = NULL;
> +	char		governor[MAX_LINE_SIZE + 1];
> +
> +	if (!cpu_online(cpu_id))
> +		goto Out;
> +
> +	governor_file = g_strdup_printf(SYSFS_GOVERNOR_FILE, cpu_id); 
> +	dbg("Trying ot write governor %s", new_governor);
> +
> +	if (!write_line(governor_file, "%s", new_governor)) {
> +		ret = FALSE;
> +		goto Out;
> +	}
> +	
> +	/* check if governor has been set */
> +	usleep(1000);
> +	read_line(governor_file, governor, MAX_LINE_SIZE);
> +	if (strstr(governor, new_governor))
> +		ret = TRUE;
> +	else
> +		ret = FALSE;
> +Out:
> +	g_free(governor_file);
> +	return ret;
> +}
> 
> The usleep should be completely unnecessary.
> A write to sysfs is atomic, we won't return to userspace
> until the governor has been set.

Right. That's in for a reason which is so long ago that I don't remember
it anymore and just overlooked it while doing the cleanup.

> 
> Oh, and you're still including the /proc/cpuinfo definition, even
> though nothing uses it now that you've switched over to using _SC_NR_PROCESSORS

Right, going to change that.

> 
> 		Dave
> 

Regards,
	Holger