[Cbe-oss-dev] Subject: cell: add spu aware cpufreq governor

[Christian Krafft]

Hi Arnd,

first of all, sorry for the late answer.
Here are my comments:

On Sat, 19 Jan 2008 22:38:44 +0100
Arnd Bergmann <arnd at arndb.de> wrote:

> On Friday 18 January 2008, Christian Krafft wrote:
> > +#include <linux/cpufreq.h>
> > +#include <linux/sched.h>
> > +#include <linux/timer.h>
> > +#include <asm/atomic.h>
> > +#include <asm/machdep.h>
> > +#include <asm/spu.h>
> > +
> > +#include "asm/cell-regs.h"
> > +
> > +#define DEBUG		1
> 
> You should have the '#define DEBUG' _before_ the #includes, otherwise pr_debug
> and friends don't do what you think they do.

sure, DEBUG should have been removed anyway.

> > +static void spu_gov_init_work(struct spu_gov_info_struct *info);
> 
> Try to avoid forward declarations for static functions, instead reorder
> the functions in call order.

Forward declaration was not a good idea at all, cause multiple calls to the
init_work function seems a bit odd. Fixed with next version.

> > +static void spu_gov_timer(struct work_struct *work)
> 
> 'timer' is a bad name for a work function, because it suggests
> you're working on a timer_list.

this was a relic from a timer based version, renamed.

> > +static void spu_gov_init_work(struct spu_gov_info_struct *info)
> > +{
> > +	int delay = usecs_to_jiffies(info->poll_int * 1000);
> > +	delay -= jiffies % delay;
> > +	INIT_DELAYED_WORK_DEFERRABLE(&info->work, spu_gov_timer);
> > +	queue_delayed_work_on(info->policy->cpu, kspugov_wq, &info->work,
> > delay); +}
> 
> I think you shouldn't call INIT_DELAYED_WORK_DEFERRABLE() more than once.
> While I'm not sure if it's actually broken like you do it, it's certainly
> not how this interface was meant to be used.

fixed with the removal of that odd forward declaration

> What's the point of the computation, can't you just always queue the
> work to be run in info->poll_int miliseconds from the time you call it?

This code is from the ondemand governor. It should cause the timer to be
triggered at the same jiffy. As our polling intervall is much higher I guess
this makes no sense here -> removed.

It was also a stupid idea to use usecs_to_jiffies instead of msecs ;-)

> > +static void spu_gov_cancel_work(struct spu_gov_info_struct *info)
> > +{
> > +	cancel_delayed_work(&info->work);
> > +}
> 
> Doesn't that need to be cancel_delayed_work_sync() for rearming work queues?

Jep.

> > +static int __init spu_gov_init(void)
> > +{
> > +	if (!machine_is(cell))
> > +		return -ENODEV;
> 
> This seems wrong, why would you only allow that on cell platforms,
> but e.g. not celleb or even ps3 (assuming they had a cpufreq
> backend)? I think the only dependency is on spufs being loaded and
> that should work using module dependencies.

Sure.
I added the dependency to CONFIG_SPUFS.
I also removed the dependency to CBE_CPUFREQ and replaced it by a select
CBE_CPUFREQ.

> > +	kspugov_wq = create_workqueue("kspugov");
> > +	if (!kspugov_wq) {
> > +		printk(KERN_ERR "creation of kspugov failed\n");
> > +		return -EFAULT;
> > +	}
> > +
> > +	return cpufreq_register_governor(&spu_governor);
> > +}
> > +
> > +static void __exit spu_gov_exit(void)
> > +{
> > +	cpufreq_unregister_governor(&spu_governor);
> > +	destroy_workqueue(kspugov_wq);
> > +}
> 
> What happens when the work functions is called after unregistering?

This can not happen, as module can only be unloaded if the spu_gov_govern got
CPUFREQ_GOV_STOP for all cpus it has been started for. When getting
CPUFREQ_GOV_STOP the cancel_delayed_work_SYNC function gets called.
After spu_gov_govern processed the last CPUFREQ_GOV_STOP, no work function gets
called. I added a BUG_ON with a comment to make that clear.


Thanks for the review,
Hope next version is clean ...


-- 
Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/cbe-oss-dev/attachments/20080128/bd3e6a81/attachment.pgp>