Re: [Cbe-oss-dev] [Patch] Resending: cell: add spu aware cpufreq governor

[Arnd Bergmann <arnd@arndb.de>]

Sorry for my late reply, I lost track of this discussion when it happened
during LCA and never got back to it. The patch looks good enough for 2.6.26,
I think, but one thing still really bothers me:

On Monday 28 January 2008, Christian Krafft wrote:
> +/* parts of this function should go into spu scheduler */
> +static int spu_gov_calc_load(struct spu_gov_info_struct *info)
> +{
> +       unsigned long active_tasks; /* fixed-point */
> +       int cpu, load;
> +
> +       cpu = info->policy->cpu;
> +       active_tasks = cbe_spu_info[cpu_to_node(cpu)].nr_active * FIXED_1;
> +
> +       /* this is also a bit too trivial
> +        * actually we want the max load of all spu's belonging together */
> +       CALC_LOAD(info->load, EXP_1, active_tasks);
> +
> +       load = (info->load + FIXED_1 / 200) >> FSHIFT;
> +
> +       return load;
> +}

The nr_active variable in cbe_spu_info has a completely different meaning
from nr_active() in the linux scheduler. AFAICS, basing your computation
on this makes no sense whatsoever. Instead of looking at what threads are
running on the SPUs, you look at what is loaded. Because of our lazy loading
scheduler mechanism, a task occupying all SPUs at 10% of the time and
otherwise calling nanosleep or futex_wait will still show up as 100% load
here, so we don't lower the frequency although we should.

What's worse, a compute-intensive task running on half of the SPUs with the
rest of them being idle shows up as 50% load and causes the frequency to be
dropped to 50%, when it should run at full speed.

	Arnd <><