On Tuesday 25 of September 2012 00:43:52 Daniel Lezcano wrote:

The code checks if the driver is already set without taking the lock, but, right after, it takes the lock to assign the variable.

If it is safe to check the variable without lock, then it is safe to assign it without lock. If it is unsafe to assign without a lock, then it is unsafe to check it without a lock.

I don't find a path in the different drivers where that could happen because the arch specific drivers are written in such way it is not possible to register a driver while it is unregistered, except maybe in a very improbable case when "intel_idle" and "processor_idle" are competing. One could unregister a driver, while the other one is registering.

Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org

The code looks generally OK (a minor nit below), but I'm not really sure what you wanted to say in the changelog. It would suffice to say that the existing code is racy and either we don't need to take the spinlock in cpuidle_unregister_driver() at all, or we should put the test under the spinlock as well as the modification.

---

drivers/cpuidle/driver.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 39ba8e1..4a0c4ab 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -85,17 +85,17 @@ EXPORT_SYMBOL_GPL(cpuidle_get_driver); */ void cpuidle_unregister_driver(struct cpuidle_driver *drv) {

• spin_lock(&cpuidle_driver_lock);
• if (drv != cpuidle_curr_driver) { WARN(1, "invalid cpuidle_unregister_driver(%s)\n", drv->name);

It is not necessary to execute the WARN() under the spinlock.

• return;
  

• goto out;
  

  }

• spin_lock(&cpuidle_driver_lock);
• if (!WARN_ON(drv->refcnt > 0)) cpuidle_curr_driver = NULL;

+out: spin_unlock(&cpuidle_driver_lock); } EXPORT_SYMBOL_GPL(cpuidle_unregister_driver);

Thanks, Rafael

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