Subject: cell: add spu aware cpufreq governor

+static void spu_gov_init_work(struct spu_gov_info_struct *info);

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +4 -0

+	kspugov_wq = create_workqueue("kspugov");
+	if (!kspugov_wq) {
+		printk(KERN_ERR "creation of kspugov failed\n");
+		return -EFAULT;

arch/powerpc/platforms/cell/cbe_spu_governor.c line 32

Quoted 10 lines · Christian Krafft

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +10 -0

+#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.

Quoted 1 line

+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.

Quoted 1 line

+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.

Quoted 7 lines

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +6 -0

+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.

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?

arch/powerpc/platforms/cell/cbe_spu_governor.c line 101

Quoted 4 lines

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +4 -0

+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?

arch/powerpc/platforms/cell/cbe_spu_governor.c line 168

Quoted 4 lines

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +4 -0

+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.

Quoted 8 lines

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +8 -0

+	kspugov_wq = create_workqueue("kspugov");
+	if (!kspugov_wq) {
+		printk(KERN_ERR "creation of kspugov failed\n");
+		return -EFAULT;
+	}
+
+	return cpufreq_register_governor(&spu_governor);
+}

arch/powerpc/platforms/cell/cbe_spu_governor.c line 183

Quoted 6 lines

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +6 -0

+
+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?

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +7 -0

+/* 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;

This patch is also missing a correct load calculation.
It works pretty well for spu's running at full time or idling, but not so well for mixed load (i.e. each spu running 50 percent of the time we would switch to fullspeed instead of half speed).

Quoted 4 lines · Christian Krafft

This patch is also missing a correct load calculation.
It works pretty well for spu's running at full time or idling, but not so well for mixed load (i.e. each spu running 50 percent of the time we would switch to fullspeed instead of half speed).

So why should that be something to optimize for? If the SPUs are all idle, I would assume we'd be better off throwing the tasks off and putting them into low-power mode, rather than changing the frequency, right?

What's the point of your driver, really?

	Arnd <><

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +6 -0

+/*
+ * module init and destoy
+ */
+
+static int __init spu_gov_init(void)
+{

+}

Quoted 6 lines

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +6 -0

+/*
+ * module init and destoy
+ */
+
+static int __init spu_gov_init(void)
+{

arch/powerpc/platforms/cell/cbe_spu_governor.c line 176

Quoted 10 lines

arch/powerpc/platforms/cell/cbe_spu_governor.c new file +10 -0

+if (!machine_is(cell))
+return -ENODEV;
+
+kspugov_wq = create_workqueue("kspugov");
+if (!kspugov_wq) {
+printk(KERN_ERR "creation of kspugov failed\n");
+return -EFAULT;
+}
+
+return cpufreq_register_governor(&spu_governor);

Missing an error handling. This line should be something like:

    err = cpufreq_register_governor(&spu_governor);
    if (err) {
        destroy_workqueue(kspugov_wq);
        return err;
    }

    return 0;

It seems like this is copied from drivers/cpufreq/cpufreq_governor.c Althought this is not big problem, it would be nice to fix as well.

Quoted 1 line

+}