Subject: cell: add spu aware cpufreq governor

Subject: cell: add spu aware cpufreq governor

[Christian Krafft]

[-- Attachment #1.1: Type: text/plain, Size: 7593 bytes --]

From: Christian Krafft <krafft@de.ibm.com>

This patch adds a cpufreq governor that takes the spu load into account.
It's very similar to the ondemand governor, but not as complex.
Instead of hacking spu load into the ondemand governor I'd like to see
cpufreq accepting multiple governors per cpu in future. Don't know if this is
the right way, but it would keep the governors simple.

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

Signed-off-by: Christian Krafft <krafft@de.ibm.com>

Index: linux.git/arch/powerpc/platforms/cell/Kconfig
===================================================================
--- linux.git.orig/arch/powerpc/platforms/cell/Kconfig
+++ linux.git/arch/powerpc/platforms/cell/Kconfig
@@ -87,4 +87,13 @@ config CBE_CPUFREQ_PMI
 	  processor will not only be able to run at lower speed,
 	  but also at lower core voltage.
 
+config CBE_CPUFREQ_SPU_GOVERNOR
+	tristate "CBE frequency scaling based on SPU usage"
+	depends on CBE_CPUFREQ
+	default m
+	help
+	  This governor checks for spu usage to adjust the cpu frequency.
+	  If no spu is running on a given cpu, that cpu will be throttled to
+	  the minimal possible frequency.
+
 endmenu
Index: linux.git/arch/powerpc/platforms/cell/Makefile
===================================================================
--- linux.git.orig/arch/powerpc/platforms/cell/Makefile
+++ linux.git/arch/powerpc/platforms/cell/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_CBE_THERM)			+= cbe_thermal
 obj-$(CONFIG_CBE_CPUFREQ_PMI)		+= cbe_cpufreq_pmi.o
 obj-$(CONFIG_CBE_CPUFREQ)		+= cbe-cpufreq.o
 cbe-cpufreq-y				+= cbe_cpufreq_pervasive.o cbe_cpufreq.o
+obj-$(CONFIG_CBE_CPUFREQ_SPU_GOVERNOR)	+= cbe_spu_governor.o
 
 ifeq ($(CONFIG_SMP),y)
 obj-$(CONFIG_PPC_CELL_NATIVE)		+= smp.o
Index: linux.git/arch/powerpc/platforms/cell/cbe_spu_governor.c
===================================================================
--- /dev/null
+++ linux.git/arch/powerpc/platforms/cell/cbe_spu_governor.c
@@ -0,0 +1,191 @@
+/*
+ * spu aware cpufreq governor for the cell processor
+ *
+ * (C) Copyright IBM Deutschland Entwicklung GmbH 2005-2007
+ *
+ * Author: Christian Krafft <krafft@de.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#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
+#define POLL_TIME	1000	/* in ms */
+
+struct spu_gov_info_struct {
+	unsigned long load;
+	unsigned long last_load;
+	struct cpufreq_policy *policy;
+	struct delayed_work work;
+	unsigned int poll_int;
+};
+static DEFINE_PER_CPU(struct spu_gov_info_struct, spu_gov_info);
+
+static struct workqueue_struct *kspugov_wq;
+
+/* 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;
+}
+
+static void spu_gov_init_work(struct spu_gov_info_struct *info);
+
+static void spu_gov_timer(struct work_struct *work)
+{
+	struct spu_gov_info_struct *info;
+	unsigned int load_int;
+
+	info = container_of(work, struct spu_gov_info_struct, work.work);
+
+	load_int = spu_gov_calc_load(info);
+
+	if ((load_int == 0) && (info->policy->cur != info->policy->min)) {
+		pr_debug("switching low frequency\n");
+		__cpufreq_driver_target(info->policy,
+			info->policy->min,
+			CPUFREQ_RELATION_L);
+	} else if ((load_int > 0) && (info->policy->cur != info->policy->max)) {
+		pr_debug("switching to high frequency\n");
+		__cpufreq_driver_target(info->policy,
+			info->policy->max,
+			CPUFREQ_RELATION_H);
+	}
+
+	spu_gov_init_work(info);
+}
+
+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);
+}
+
+static void spu_gov_cancel_work(struct spu_gov_info_struct *info)
+{
+	cancel_delayed_work(&info->work);
+}
+
+static int spu_gov_govern(struct cpufreq_policy *policy, unsigned int event)
+{
+	unsigned int cpu = policy->cpu;
+	struct spu_gov_info_struct *info, *affected_info;
+	int i;
+	int ret = 0;
+
+	info = &per_cpu(spu_gov_info, cpu);
+
+	switch (event) {
+	case CPUFREQ_GOV_START:
+		if (!cpu_online(cpu)) {
+			printk(KERN_ERR "cpu %d is not online\n", cpu);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (!policy->cur) {
+			printk(KERN_ERR "no cpu specified in policy\n");
+			ret = -EINVAL;
+			break;
+		}
+
+		/* initialize spu_gov_info for all affected cpus */
+		for_each_cpu_mask(i, policy->cpus) {
+			affected_info = &per_cpu(spu_gov_info, i);
+			affected_info->policy = policy;
+		}
+
+		info->poll_int = POLL_TIME;
+
+		/* setup timer */
+		spu_gov_init_work(info);
+
+		break;
+
+	case CPUFREQ_GOV_STOP:
+		/* cancel timer */
+		spu_gov_cancel_work(info);
+
+		/* clean spu_gov_info for all affected cpus */
+		for_each_cpu_mask (i, policy->cpus) {
+			info = &per_cpu(spu_gov_info, i);
+			info->policy = NULL;
+		}
+
+		break;
+	}
+
+	return ret;
+}
+
+static struct cpufreq_governor spu_governor = {
+	.name = "spu_governor",
+	.governor = spu_gov_govern,
+	.owner = THIS_MODULE,
+};
+
+/*
+ * module init and destoy
+ */
+
+static int __init spu_gov_init(void)
+{
+	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);
+}
+
+static void __exit spu_gov_exit(void)
+{
+	cpufreq_unregister_governor(&spu_governor);
+	destroy_workqueue(kspugov_wq);
+}
+
+
+module_init(spu_gov_init);
+module_exit(spu_gov_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Christian Krafft <krafft@de.ibm.com>");
+


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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

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

[Arnd Bergmann]

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.

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

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

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

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

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

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

	Arnd <><

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

[Christian Krafft]

[-- Attachment #1.1: Type: text/plain, Size: 4136 bytes --]

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@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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

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

[Christian Krafft]

[-- Attachment #1.1: Type: text/plain, Size: 7547 bytes --]

From: Christian Krafft <krafft@de.ibm.com>

This patch adds a cpufreq governor that takes the spu load into account.
It's very similar to the ondemand governor, but not as complex.
Instead of hacking spu load into the ondemand governor I'd like to see
cpufreq accepting multiple governors per cpu in future. Don't know if this is
the right way, but it would keep the governors simple.

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

Signed-off-by: Christian Krafft <krafft@de.ibm.com>

Index: linux.git/arch/powerpc/platforms/cell/Kconfig
===================================================================
--- linux.git.orig/arch/powerpc/platforms/cell/Kconfig
+++ linux.git/arch/powerpc/platforms/cell/Kconfig
@@ -87,4 +87,14 @@ config CBE_CPUFREQ_PMI
 	  processor will not only be able to run at lower speed,
 	  but also at lower core voltage.
 
+config CBE_CPUFREQ_SPU_GOVERNOR
+	tristate "CBE frequency scaling based on SPU usage"
+	depends SPU_FS
+	select CBE_CPUFREQ
+	default m
+	help
+	  This governor checks for spu usage to adjust the cpu frequency.
+	  If no spu is running on a given cpu, that cpu will be throttled to
+	  the minimal possible frequency.
+
 endmenu
Index: linux.git/arch/powerpc/platforms/cell/Makefile
===================================================================
--- linux.git.orig/arch/powerpc/platforms/cell/Makefile
+++ linux.git/arch/powerpc/platforms/cell/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_CBE_THERM)			+= cbe_thermal
 obj-$(CONFIG_CBE_CPUFREQ_PMI)		+= cbe_cpufreq_pmi.o
 obj-$(CONFIG_CBE_CPUFREQ)		+= cbe-cpufreq.o
 cbe-cpufreq-y				+= cbe_cpufreq_pervasive.o cbe_cpufreq.o
+obj-$(CONFIG_CBE_CPUFREQ_SPU_GOVERNOR)	+= cbe_spu_governor.o
 
 ifeq ($(CONFIG_SMP),y)
 obj-$(CONFIG_PPC_CELL_NATIVE)		+= smp.o
Index: linux.git/arch/powerpc/platforms/cell/cbe_spu_governor.c
===================================================================
--- /dev/null
+++ linux.git/arch/powerpc/platforms/cell/cbe_spu_governor.c
@@ -0,0 +1,199 @@
+/*
+ * spu aware cpufreq governor for the cell processor
+ *
+ * (C) Copyright IBM Deutschland Entwicklung GmbH 2005-2007
+ *
+ * Author: Christian Krafft <krafft@de.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#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 POLL_TIME	1000	/* in ms */
+
+struct spu_gov_info_struct {
+	unsigned long load;
+	unsigned long last_load;
+	struct cpufreq_policy *policy;
+	struct delayed_work work;
+	unsigned int poll_int;
+};
+static DEFINE_PER_CPU(struct spu_gov_info_struct, spu_gov_info);
+
+static struct workqueue_struct *kspugov_wq;
+
+/* 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;
+}
+
+static void spu_gov_work(struct work_struct *work)
+{
+	struct spu_gov_info_struct *info;
+	unsigned int load_int;
+	int delay;
+
+	info = container_of(work, struct spu_gov_info_struct, work.work);
+
+	/* after cancel_delayed_work_sync we unset info->policy */
+	BUG_ON(info->policy == NULL);
+
+	load_int = spu_gov_calc_load(info);
+
+	if ((load_int == 0) && (info->policy->cur != info->policy->min)) {
+		pr_debug("switching cpu %d to low frequency\n", info->policy->cpu);
+		__cpufreq_driver_target(info->policy,
+			info->policy->min,
+			CPUFREQ_RELATION_L);
+	} else if ((load_int > 0) && (info->policy->cur != info->policy->max)) {
+		pr_debug("switching cpu %d to high frequency\n", info->policy->cpu);
+		__cpufreq_driver_target(info->policy,
+			info->policy->max,
+			CPUFREQ_RELATION_H);
+	}
+
+	delay = msecs_to_jiffies(info->poll_int);
+	queue_delayed_work_on(info->policy->cpu, kspugov_wq, &info->work, delay);
+}
+
+static void spu_gov_init_work(struct spu_gov_info_struct *info)
+{
+	int delay = msecs_to_jiffies(info->poll_int);
+	INIT_DELAYED_WORK_DEFERRABLE(&info->work, spu_gov_work);
+	queue_delayed_work_on(info->policy->cpu, kspugov_wq, &info->work, delay);
+}
+
+static void spu_gov_cancel_work(struct spu_gov_info_struct *info)
+{
+	cancel_delayed_work_sync(&info->work);
+}
+
+static int spu_gov_govern(struct cpufreq_policy *policy, unsigned int event)
+{
+	unsigned int cpu = policy->cpu;
+	struct spu_gov_info_struct *info, *affected_info;
+	int i;
+	int ret = 0;
+
+	info = &per_cpu(spu_gov_info, cpu);
+
+	switch (event) {
+	case CPUFREQ_GOV_START:
+		if (!cpu_online(cpu)) {
+			printk(KERN_ERR "cpu %d is not online\n", cpu);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (!policy->cur) {
+			printk(KERN_ERR "no cpu specified in policy\n");
+			ret = -EINVAL;
+			break;
+		}
+
+		/* initialize spu_gov_info for all affected cpus */
+		for_each_cpu_mask(i, policy->cpus) {
+			affected_info = &per_cpu(spu_gov_info, i);
+			affected_info->policy = policy;
+		}
+
+		info->poll_int = POLL_TIME;
+
+		/* setup timer */
+		spu_gov_init_work(info);
+
+		break;
+
+	case CPUFREQ_GOV_STOP:
+		/* cancel timer */
+		spu_gov_cancel_work(info);
+
+		/* clean spu_gov_info for all affected cpus */
+		for_each_cpu_mask (i, policy->cpus) {
+			info = &per_cpu(spu_gov_info, i);
+			info->policy = NULL;
+		}
+
+		break;
+	}
+
+	return ret;
+}
+
+static struct cpufreq_governor spu_governor = {
+	.name = "spu_governor",
+	.governor = spu_gov_govern,
+	.owner = THIS_MODULE,
+};
+
+/*
+ * module init and destoy
+ */
+
+static int __init spu_gov_init(void)
+{
+	int ret;
+
+	kspugov_wq = create_workqueue("kspugov");
+	if (!kspugov_wq) {
+		printk(KERN_ERR "creation of kspugov failed\n");
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = cpufreq_register_governor(&spu_governor);
+	if (ret) {
+		printk(KERN_ERR "registration of governor failed\n");
+		destroy_workqueue(kspugov_wq);
+		goto out;;
+	}
+out:
+	return ret;
+}
+
+static void __exit spu_gov_exit(void)
+{
+	cpufreq_unregister_governor(&spu_governor);
+	destroy_workqueue(kspugov_wq);
+}
+
+
+module_init(spu_gov_init);
+module_exit(spu_gov_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Christian Krafft <krafft@de.ibm.com>");
+

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

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

[Arnd Bergmann]

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

Re: [Cbe-oss-dev] [Patch] Resending: cell_add_spuaware_cpufreq_governor.diff

[Christian Krafft]

[-- Attachment #1.1: Type: text/plain, Size: 7994 bytes --]

From: Christian Krafft <krafft@de.ibm.com>

This patch adds a cpufreq governor that takes the spu load into account.
It's very similar to the ondemand governor, but not as complex.
Instead of hacking spu load into the ondemand governor I'd like to see
cpufreq accepting multiple governors per cpu in future. Don't know if this is
the right way, but it would keep the governors simple.

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

This resent version fixes also corrects the depends clause in the Kconfig file.

Signed-off-by: Christian Krafft <krafft@de.ibm.com>




---
Index: linux.git/arch/powerpc/platforms/cell/Kconfig
===================================================================
--- linux.git.orig/arch/powerpc/platforms/cell/Kconfig
+++ linux.git/arch/powerpc/platforms/cell/Kconfig
@@ -94,6 +94,16 @@ config CBE_CPUFREQ_PMI
 	  processor will not only be able to run at lower speed,
 	  but also at lower core voltage.
 
+config CBE_CPUFREQ_SPU_GOVERNOR
+	tristate "CBE frequency scaling based on SPU usage"
+	depends on SPU_FS
+	select CBE_CPUFREQ
+	default m
+	help
+	  This governor checks for spu usage to adjust the cpu frequency.
+	  If no spu is running on a given cpu, that cpu will be throttled to
+	  the minimal possible frequency.
+
 endmenu
 
 config OPROFILE_CELL
Index: linux.git/arch/powerpc/platforms/cell/Makefile
===================================================================
--- linux.git.orig/arch/powerpc/platforms/cell/Makefile
+++ linux.git/arch/powerpc/platforms/cell/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_CBE_THERM)			+= cbe_thermal
 obj-$(CONFIG_CBE_CPUFREQ_PMI)		+= cbe_cpufreq_pmi.o
 obj-$(CONFIG_CBE_CPUFREQ)		+= cbe-cpufreq.o
 cbe-cpufreq-y				+= cbe_cpufreq_pervasive.o cbe_cpufreq.o
+obj-$(CONFIG_CBE_CPUFREQ_SPU_GOVERNOR)	+= cbe_spu_governor.o
 
 ifeq ($(CONFIG_SMP),y)
 obj-$(CONFIG_PPC_CELL_NATIVE)		+= smp.o
Index: linux.git/arch/powerpc/platforms/cell/cbe_spu_governor.c
===================================================================
--- /dev/null
+++ linux.git/arch/powerpc/platforms/cell/cbe_spu_governor.c
@@ -0,0 +1,199 @@
+/*
+ * spu aware cpufreq governor for the cell processor
+ *
+ * (C) Copyright IBM Deutschland Entwicklung GmbH 2005-2007
+ *
+ * Author: Christian Krafft <krafft@de.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#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 POLL_TIME	1000	/* in ms */
+
+struct spu_gov_info_struct {
+	unsigned long load;
+	unsigned long last_load;
+	struct cpufreq_policy *policy;
+	struct delayed_work work;
+	unsigned int poll_int;
+};
+static DEFINE_PER_CPU(struct spu_gov_info_struct, spu_gov_info);
+
+static struct workqueue_struct *kspugov_wq;
+
+/* 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;
+}
+
+static void spu_gov_work(struct work_struct *work)
+{
+	struct spu_gov_info_struct *info;
+	unsigned int load_int;
+	int delay;
+
+	info = container_of(work, struct spu_gov_info_struct, work.work);
+
+	/* after cancel_delayed_work_sync we unset info->policy */
+	BUG_ON(info->policy == NULL);
+
+	load_int = spu_gov_calc_load(info);
+
+	if ((load_int == 0) && (info->policy->cur != info->policy->min)) {
+		pr_debug("switching cpu %d to low frequency\n", info->policy->cpu);
+		__cpufreq_driver_target(info->policy,
+			info->policy->min,
+			CPUFREQ_RELATION_L);
+	} else if ((load_int > 0) && (info->policy->cur != info->policy->max)) {
+		pr_debug("switching cpu %d to high frequency\n", info->policy->cpu);
+		__cpufreq_driver_target(info->policy,
+			info->policy->max,
+			CPUFREQ_RELATION_H);
+	}
+
+	delay = msecs_to_jiffies(info->poll_int);
+	queue_delayed_work_on(info->policy->cpu, kspugov_wq, &info->work, delay);
+}
+
+static void spu_gov_init_work(struct spu_gov_info_struct *info)
+{
+	int delay = msecs_to_jiffies(info->poll_int);
+	INIT_DELAYED_WORK_DEFERRABLE(&info->work, spu_gov_work);
+	queue_delayed_work_on(info->policy->cpu, kspugov_wq, &info->work, delay);
+}
+
+static void spu_gov_cancel_work(struct spu_gov_info_struct *info)
+{
+	cancel_delayed_work_sync(&info->work);
+}
+
+static int spu_gov_govern(struct cpufreq_policy *policy, unsigned int event)
+{
+	unsigned int cpu = policy->cpu;
+	struct spu_gov_info_struct *info, *affected_info;
+	int i;
+	int ret = 0;
+
+	info = &per_cpu(spu_gov_info, cpu);
+
+	switch (event) {
+	case CPUFREQ_GOV_START:
+		if (!cpu_online(cpu)) {
+			printk(KERN_ERR "cpu %d is not online\n", cpu);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (!policy->cur) {
+			printk(KERN_ERR "no cpu specified in policy\n");
+			ret = -EINVAL;
+			break;
+		}
+
+		/* initialize spu_gov_info for all affected cpus */
+		for_each_cpu_mask(i, policy->cpus) {
+			affected_info = &per_cpu(spu_gov_info, i);
+			affected_info->policy = policy;
+		}
+
+		info->poll_int = POLL_TIME;
+
+		/* setup timer */
+		spu_gov_init_work(info);
+
+		break;
+
+	case CPUFREQ_GOV_STOP:
+		/* cancel timer */
+		spu_gov_cancel_work(info);
+
+		/* clean spu_gov_info for all affected cpus */
+		for_each_cpu_mask (i, policy->cpus) {
+			info = &per_cpu(spu_gov_info, i);
+			info->policy = NULL;
+		}
+
+		break;
+	}
+
+	return ret;
+}
+
+static struct cpufreq_governor spu_governor = {
+	.name = "spu_governor",
+	.governor = spu_gov_govern,
+	.owner = THIS_MODULE,
+};
+
+/*
+ * module init and destoy
+ */
+
+static int __init spu_gov_init(void)
+{
+	int ret;
+
+	kspugov_wq = create_workqueue("kspugov");
+	if (!kspugov_wq) {
+		printk(KERN_ERR "creation of kspugov failed\n");
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = cpufreq_register_governor(&spu_governor);
+	if (ret) {
+		printk(KERN_ERR "registration of governor failed\n");
+		destroy_workqueue(kspugov_wq);
+		goto out;;
+	}
+out:
+	return ret;
+}
+
+static void __exit spu_gov_exit(void)
+{
+	cpufreq_unregister_governor(&spu_governor);
+	destroy_workqueue(kspugov_wq);
+}
+
+
+module_init(spu_gov_init);
+module_exit(spu_gov_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Christian Krafft <krafft@de.ibm.com>");
+

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Re: [Cbe-oss-dev] [Patch] Resending: cell_add_spuaware_cpufreq_governor.diff

[Arnd Bergmann]

On Wednesday 16 April 2008, Christian Krafft wrote:
> 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 <><

Re: [Cbe-oss-dev] [Patch] Resending: cell_add_spuaware_cpufreq_governor.diff

[Christian Krafft]

[-- Attachment #1.1: Type: text/plain, Size: 1522 bytes --]

On Wed, 16 Apr 2008 14:08:25 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Wednesday 16 April 2008, Christian Krafft wrote:
> > 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 <><

What we have now is good enough to
- switch to full power if there are spus active (waiting or running) and 
- switch to low power if not.

What we have now is not good enough to
- switch to low power if there are spus waiting, but none are running

As we cannot _yet_ differentiate between spus waiting and spus running means
that in _some_ cases we will not save as much power as we could.

The point of submitting the driver is to get it working.
Further improvements will follow of course.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

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

[Akinobu Mita]

> +/*
> + * module init and destoy
> + */
> +
> +static int __init spu_gov_init(void)
> +{
> +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.

> +}

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

[Christian Krafft]

[-- Attachment #1.1: Type: text/plain, Size: 1413 bytes --]

First of all, sorry for the delayed answer.

I had no hardware access (and no mail) last week,
I'll provide an updated patch very soon.

And thanks for your comments ;-)

Cheers,
Christian

On Mon, 21 Jan 2008 12:09:23 +0900 (JST)
"Akinobu Mita" <mita@fixstars.com> wrote:

> > +/*
> > + * module init and destoy
> > + */
> > +
> > +static int __init spu_gov_init(void)
> > +{
> > +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.
> 
> > +}
> 
> 
> 


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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq