On Tuesday 25 of September 2012 00:43:54 Daniel Lezcano wrote:
> With the tegra3 and the big.LITTLE [1] new architectures, several cpus
> with different characteristics (latencies and states) can co-exists on the
> system.
>
> The cpuidle framework has the limitation of handling only identical cpus.
>
> This patch removes this limitation by introducing the multiple driver support
> for cpuidle.
>
> This option is configurable at compile time and should be enabled for the
> architectures mentioned above. So there is no impact for the other platforms
> if the option is disabled. The option defaults to 'n'. Note the multiple
> drivers
> support is also compatible with the existing drivers, even if just one driver
> is
> needed, all the cpu will be tied to this driver using an extra small chunk of
> processor memory.
>
> The multiple driver support use a per-cpu driver pointer instead of a global
> variable and the accessor to this variable are done from a cpu context.
>
> In order to keep the compatibility with the existing drivers, the function
> 'cpuidle_register_driver' and 'cpuidle_unregister_driver' will register
> the specified driver for all the cpus.
>
> The sysfs output for the 'current_driver' is changed when this option is
> set by giving the drivers per cpu.
>
> eg.
> cpu0: acpi_idle
> cpu1: acpi_idle
>
> but if the option is disabled, the output will remain the same.
>
> [1] http://lwn.net/Articles/481055/
>
>
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
> ---
> drivers/cpuidle/Kconfig | 9 +++
> drivers/cpuidle/cpuidle.c | 24 ++++++--
> drivers/cpuidle/driver.c | 136 +++++++++++++++++++++++++++++++++++++++++---
> drivers/cpuidle/sysfs.c | 50 +++++++++++++----
> include/linux/cpuidle.h | 8 ++-
> 5 files changed, 197 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index a76b689..234ae65 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -9,6 +9,15 @@ config CPU_IDLE
>
> If you're using an ACPI-enabled platform, you should say Y here.
>
> +config CPU_IDLE_MULTIPLE_DRIVERS
> + bool "Support multiple cpuidle drivers"
> + depends on CPU_IDLE
> + default n
> + help
> + Allows the cpuidle framework to use different drivers for each CPU.
> + This is useful if you have a system with different CPU latencies and
> + states. If unsure say N.
> +
> config CPU_IDLE_GOV_LADDER
> bool
> depends on CPU_IDLE
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index e28f6ea..c581b99 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -68,7 +68,7 @@ static cpuidle_enter_t cpuidle_enter_ops;
> int cpuidle_play_dead(void)
> {
> struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
> - struct cpuidle_driver *drv = cpuidle_get_driver();
> + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev->cpu);
It would be better to change that to be called as cpuidle_get_cpu_driver(dev),
since dev is a cpuidle_device already.
> int i, dead_state = -1;
> int power_usage = -1;
>
> @@ -128,7 +128,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev,
> struct cpuidle_driver *drv,
> int cpuidle_idle_call(void)
> {
> struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
> - struct cpuidle_driver *drv = cpuidle_get_driver();
> + struct cpuidle_driver *drv;
> int next_state, entered_state;
>
> if (off)
> @@ -141,6 +141,8 @@ int cpuidle_idle_call(void)
> if (!dev || !dev->enabled)
> return -EBUSY;
>
> + drv = cpuidle_get_cpu_driver(dev->cpu);
> +
> /* ask the governor for the next state */
> next_state = cpuidle_curr_governor->select(drv, dev);
> if (need_resched()) {
> @@ -308,15 +310,19 @@ static void poll_idle_init(struct cpuidle_driver *drv)
> {}
> int cpuidle_enable_device(struct cpuidle_device *dev)
> {
> int ret, i;
> - struct cpuidle_driver *drv = cpuidle_get_driver();
> + struct cpuidle_driver *drv;
>
> if (!dev)
> return -EINVAL;
>
> if (dev->enabled)
> return 0;
> +
> + drv = cpuidle_get_cpu_driver(dev->cpu);
> +
> if (!drv || !cpuidle_curr_governor)
> return -EIO;
> +
> if (!dev->state_count)
> dev->state_count = drv->state_count;
>
> @@ -368,15 +374,18 @@ EXPORT_SYMBOL_GPL(cpuidle_enable_device);
> */
> void cpuidle_disable_device(struct cpuidle_device *dev)
> {
> + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev->cpu);
> +
> if (!dev->enabled)
> return;
> - if (!cpuidle_get_driver() || !cpuidle_curr_governor)
> +
> + if (!drv || !cpuidle_curr_governor)
> return;
>
> dev->enabled = 0;
>
> if (cpuidle_curr_governor->disable)
> - cpuidle_curr_governor->disable(cpuidle_get_driver(), dev);
> + cpuidle_curr_governor->disable(drv, dev);
>
> cpuidle_remove_state_sysfs(dev);
> enabled_devices--;
> @@ -395,7 +404,8 @@ static int __cpuidle_register_device(struct
> cpuidle_device *dev)
> {
> int ret;
> struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
> + struct cpuidle_driver *cpuidle_driver =
> + cpuidle_get_cpu_driver(dev->cpu);
>
> if (!try_module_get(cpuidle_driver->owner))
> return -EINVAL;
> @@ -461,7 +471,7 @@ EXPORT_SYMBOL_GPL(cpuidle_register_device);
> void cpuidle_unregister_device(struct cpuidle_device *dev)
> {
> struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
> + struct cpuidle_driver *cpuidle_driver =
> cpuidle_get_cpu_driver(dev->cpu);
>
> if (dev->registered == 0)
> return;
> diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c
> index 391f80f..6529b91 100644
> --- a/drivers/cpuidle/driver.c
> +++ b/drivers/cpuidle/driver.c
> @@ -14,7 +14,11 @@
>
> #include "cpuidle.h"
>
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> +DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
> +#else
> static struct cpuidle_driver *cpuidle_curr_driver;
> +#endif
> DEFINE_SPINLOCK(cpuidle_driver_lock);
>
> static void set_power_states(struct cpuidle_driver *drv)
> @@ -47,12 +51,25 @@ static void __cpuidle_driver_init(struct cpuidle_driver
> *drv)
> set_power_states(drv);
> }
>
> -static void cpuidle_set_driver(struct cpuidle_driver *drv)
> +static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu)
> {
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> + per_cpu(cpuidle_drivers, cpu) = drv;
> +#else
> cpuidle_curr_driver = drv;
> +#endif
I'm not a big fan of #ifdef blocks inside of functions. In my opinion it's
better to put entire functions under #ifdef blocks. So, for example, in this
particular case I would do"
#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu)
{
per_cpu(cpuidle_drivers, cpu) = drv;
}
#else
static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu)
{
cpuidle_curr_driver = drv;
}
#endif
> }
>
> -static int __cpuidle_register_driver(struct cpuidle_driver *drv)
> +static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
> +{
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> + return per_cpu(cpuidle_drivers, cpu);
> +#else
> + return cpuidle_curr_driver;
> +#endif
> +}
And here analogously.
> +
> +static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu)
> {
> if (!drv || !drv->state_count)
> return -EINVAL;
> @@ -60,26 +77,102 @@ static int __cpuidle_register_driver(struct
> cpuidle_driver *drv)
> if (cpuidle_disabled())
> return -ENODEV;
>
> - if (cpuidle_get_driver())
> + if (__cpuidle_get_cpu_driver(cpu))
> return -EBUSY;
>
> __cpuidle_driver_init(drv);
>
> - cpuidle_set_driver(drv);
> + __cpuidle_set_cpu_driver(drv, cpu);
>
> return 0;
> }
>
> -static void __cpuidle_unregister_driver(struct cpuidle_driver *drv)
> +static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu)
> {
> - if (drv != cpuidle_get_driver()) {
> + if (drv != __cpuidle_get_cpu_driver(cpu)) {
> WARN(1, "invalid cpuidle_unregister_driver(%s)\n",
> drv->name);
> return;
> }
>
> if (!WARN_ON(drv->refcnt > 0))
> - cpuidle_set_driver(NULL);
> + __cpuidle_set_cpu_driver(NULL, cpu);
> +}
> +
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> +static void __cpuidle_unregister_all_cpu_driver(struct cpuidle_driver *drv)
> +{
> + int cpu;
> + for_each_present_cpu(cpu)
> + __cpuidle_unregister_driver(drv, cpu);
> +}
> +
> +static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv)
> +{
> + int ret = 0;
> + int i, cpu;
> +
> + for_each_present_cpu(cpu) {
> + ret = __cpuidle_register_driver(drv, cpu);
> + if (!ret)
> + continue;
> + for (i = cpu - 1; i >= 0; i--)
I wonder if this is going to work in all cases. For example, is there any
guarantee that the CPU numbers start from 0 and that there are no gaps?
> + __cpuidle_unregister_driver(drv, i);
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int __cpuidle_for_each_driver(int (*cb)(int, struct cpuidle_driver *,
> + void *), void *data)
> +{
> + struct cpuidle_driver *drv;
> + int cpu, ret = 0;
> +
> + for_each_present_cpu(cpu) {
> + drv = __cpuidle_get_cpu_driver(cpu);
> + ret = cb(cpu, drv, data);
> + if (ret < 0)
> + break;
> + }
> + return ret;
> +}
> +
> +int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu)
> +{
> + int ret;
> +
> + spin_lock(&cpuidle_driver_lock);
> + ret = __cpuidle_register_driver(drv, cpu);
> + spin_unlock(&cpuidle_driver_lock);
> +
> + return ret;
> +}
> +
> +void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu)
> +{
> + spin_lock(&cpuidle_driver_lock);
> + __cpuidle_unregister_driver(drv, cpu);
> + spin_unlock(&cpuidle_driver_lock);
> +}
> +#endif
> +
> +int cpuidle_for_each_driver(int (*cb)(int, struct cpuidle_driver *, void *),
> + void *data)
> +{
> + int ret;
> +
> + spin_lock(&cpuidle_driver_lock);
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> + ret = __cpuidle_for_each_driver(cb, data);
> +#else
> + ret = cb(smp_processor_id(),
> + __cpuidle_get_cpu_driver(smp_processor_id()), data);
> +#endif
Here I'd make the definition of __cpuidle_for_each_driver() depend on
whether or not CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is set.
> + spin_unlock(&cpuidle_driver_lock);
> +
> + return ret;
> }
>
> /**
> @@ -91,7 +184,11 @@ int cpuidle_register_driver(struct cpuidle_driver *drv)
> int ret;
>
> spin_lock(&cpuidle_driver_lock);
> - ret = __cpuidle_register_driver(drv);
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> + ret = __cpuidle_register_all_cpu_driver(drv);
> +#else
> + ret = __cpuidle_register_driver(drv, smp_processor_id());
> +#endif
And analogously here.
> spin_unlock(&cpuidle_driver_lock);
>
> return ret;
> @@ -103,18 +200,37 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver);
> */
> struct cpuidle_driver *cpuidle_get_driver(void)
> {
> - return cpuidle_curr_driver;
> + return __cpuidle_get_cpu_driver(smp_processor_id());
> }
> EXPORT_SYMBOL_GPL(cpuidle_get_driver);
>
> /**
> + * cpuidle_get_cpu_driver - return the driver tied with a cpu
> + */
> +struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu)
> +{
> + struct cpuidle_driver *drv;
> +
> + spin_lock(&cpuidle_driver_lock);
> + drv = __cpuidle_get_cpu_driver(cpu);
> + spin_unlock(&cpuidle_driver_lock);
> +
> + return drv;
> +}
> +EXPORT_SYMBOL_GPL(cpuidle_get_cpu_driver);
> +
> +/**
> * cpuidle_unregister_driver - unregisters a driver
> * @drv: the driver
> */
> void cpuidle_unregister_driver(struct cpuidle_driver *drv)
> {
> spin_lock(&cpuidle_driver_lock);
> - __cpuidle_unregister_driver(drv);
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> + __cpuidle_unregister_all_cpu_driver(drv);
> +#else
> + __cpuidle_unregister_driver(drv, smp_processor_id());
> +#endif
I'm slightly cautious about using smp_processor_id() above.
get_cpu()/put_cpu() is the preferred way of doing this nowadays (although
this particular code is under the spinlock, so it should be OK).
> spin_unlock(&cpuidle_driver_lock);
> }
> EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index 5f809e3..2596422 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -43,21 +43,46 @@ out:
> return i;
> }
>
> +struct cbarg {
> + char *buf;
> + ssize_t count;
> +};
> +
> +static int each_driver_cb(int cpu, struct cpuidle_driver *drv, void *data)
> +{
> + int ret;
> + struct cbarg *cbarg = data;
> +
> + if ((drv && (strlen(drv->name) + cbarg->count) >= PAGE_SIZE) ||
> + (!drv && (strlen("none") + cbarg->count) >= PAGE_SIZE))
> + return -EOVERFLOW;
> +
> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
> + ret = sprintf(cbarg->buf + cbarg->count, "cpu%d: %s\n",
> + cpu, drv ? drv->name : "none" );
> +#else
> + ret = sprintf(cbarg->buf, "%s\n", drv ? drv->name : "none");
> +#endif
> + if (ret < 0)
> + return ret;
> +
> + cbarg->count += ret;
> +
> + return 0;
> +}
> +
> static ssize_t show_current_driver(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - ssize_t ret;
> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
> + struct cbarg cbarg = { .buf = buf };
> + int ret;
>
> - spin_lock(&cpuidle_driver_lock);
> - if (cpuidle_driver)
> - ret = sprintf(buf, "%s\n", cpuidle_driver->name);
> - else
> - ret = sprintf(buf, "none\n");
> - spin_unlock(&cpuidle_driver_lock);
> + ret = cpuidle_for_each_driver(each_driver_cb, &cbarg);
> + if (ret < 0)
> + return ret;
>
> - return ret;
> + return cbarg.count;
> }
>
> static ssize_t show_current_governor(struct device *dev,
> @@ -363,10 +388,10 @@ int cpuidle_add_state_sysfs(struct cpuidle_device
> *device)
> {
> int i, ret = -ENOMEM;
> struct cpuidle_state_kobj *kobj;
> - struct cpuidle_driver *drv = cpuidle_get_driver();
> + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(device->cpu);
>
> /* state statistics */
> - for (i = 0; i < device->state_count; i++) {
> + for (i = 0; i < drv->state_count; i++) {
> kobj = kzalloc(sizeof(struct cpuidle_state_kobj), GFP_KERNEL);
> if (!kobj)
> goto error_state;
> @@ -374,7 +399,8 @@ int cpuidle_add_state_sysfs(struct cpuidle_device *device)
> kobj->state_usage = &device->states_usage[i];
> init_completion(&kobj->kobj_unregister);
>
> - ret = kobject_init_and_add(&kobj->kobj, &ktype_state_cpuidle,
> &device->kobj,
> + ret = kobject_init_and_add(&kobj->kobj,
> + &ktype_state_cpuidle, &device->kobj,
> "state%d", i);
> if (ret) {
> kfree(kobj);
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index a4ff9f8..0e0b0ad 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -164,6 +164,13 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int index));
> extern int cpuidle_play_dead(void);
>
> +extern int cpuidle_for_each_driver(
> + int (*cb)(int, struct cpuidle_driver *, void *), void *data);
> +
> +extern struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu);
> +extern int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu);
> +extern void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int
> cpu);
> +
> #else
> static inline void disable_cpuidle(void) { }
> static inline int cpuidle_idle_call(void) { return -ENODEV; }
> @@ -190,7 +197,6 @@ static inline int cpuidle_wrap_enter(struct
> cpuidle_device *dev,
> struct cpuidle_driver *drv, int index))
> { return -ENODEV; }
> static inline int cpuidle_play_dead(void) {return -ENODEV; }
> -
> #endif
>
> #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
>
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
