Re: [PATCH] cpuidle : use percpu cpuidle in the core code

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 03/30/2012 01:25 PM, Srivatsa S. Bhat wrote:
> On 03/30/2012 04:18 PM, Daniel Lezcano wrote:
>
>> The usual cpuidle initialization routines are to register the
>> driver, then register a cpuidle device per cpu.
>>
>> With the device's state count default initialization with the
>> driver's state count, the code initialization remains mostly the
>> same in the different drivers.
>>
>> We can then add a new function 'cpuidle_register' where we register
>> the driver and the devices. These devices can be defined in a global
>> static variable in cpuidle.c. We will be able to factor out and
>> remove a lot of duplicate lines of code.
>>
>> As we still have some drivers, with different initialization routines,
>> we keep 'cpuidle_register_driver' and 'cpuidle_register_device' as low
>> level initialization routines to do some specific operations on the
>> cpuidle devices.
>>
>> Signed-off-by: Daniel Lezcano<daniel.lezcano@linaro.org>
>> ---
>>   drivers/cpuidle/cpuidle.c |   34 ++++++++++++++++++++++++++++++++++
>>   include/linux/cpuidle.h   |    3 +++
>>   2 files changed, 37 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index b8a1faf..2a174e8 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -23,6 +23,7 @@
>>   #include "cpuidle.h"
>>
>>   DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
>> +DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device);
>>
>>   DEFINE_MUTEX(cpuidle_lock);
>>   LIST_HEAD(cpuidle_detected_devices);
>> @@ -391,6 +392,39 @@ int cpuidle_register_device(struct cpuidle_device *dev)
>>
>>   EXPORT_SYMBOL_GPL(cpuidle_register_device);
>>
>> +int cpuidle_register(struct cpuidle_driver *drv)
>> +{
>> +	int ret, cpu;
>> +	struct cpuidle_device *dev;
>> +
>> +	ret = cpuidle_register_driver(drv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for_each_online_cpu(cpu) {
>> +		dev =&per_cpu(cpuidle_device, cpu);
>> +		dev->cpu = cpu;
>> +
>> +		ret = cpuidle_register_device(dev);
>> +		if (ret)
>> +			goto out_unregister;
>> +	}
>> +
>
>
> Isn't this racy with respect to CPU hotplug?

No, I don't think so. Do you see a race ?

>> +out:
>> +	return ret;
>> +
>> +out_unregister:
>> +	for_each_online_cpu(cpu) {
>> +		dev =&per_cpu(cpuidle_device, cpu);
>> +		cpuidle_unregister_device(dev);
>> +	}
>> +
>
>
> This could be improved I guess.. What if the registration fails
> for the first cpu itself? Then looping over entire online cpumask
> would be a waste of time..

Certainly in a critical section that would make sense, but for 4,8 or 16 
cpus in an initialization path at boot time... Anyway, I can add what is 
proposed in https://lkml.org/lkml/2012/3/22/143.

Thanks
   -- Daniel

> Here is a discussion on some very similar code:
> https://lkml.org/lkml/2012/3/22/72
> https://lkml.org/lkml/2012/3/22/143
>
>> +	cpuidle_unregister_driver(drv);
>> +
>> +	goto out;
>> +}
>> +EXPORT_SYMBOL_GPL(cpuidle_register);
>> +
>>   /**
>>    * cpuidle_unregister_device - unregisters a CPU's idle PM feature
>>    * @dev: the cpu
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index f3ebbba..17e3d33 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -133,6 +133,7 @@ struct cpuidle_driver {
>>   #ifdef CONFIG_CPU_IDLE
>>   extern void disable_cpuidle(void);
>>   extern int cpuidle_idle_call(void);
>> +extern int cpuidle_register(struct cpuidle_driver *drv);
>>   extern int cpuidle_register_driver(struct cpuidle_driver *drv);
>>   struct cpuidle_driver *cpuidle_get_driver(void);
>>   extern void cpuidle_unregister_driver(struct cpuidle_driver *drv);
>> @@ -150,6 +151,8 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev,
>>   #else
>>   static inline void disable_cpuidle(void) { }
>>   static inline int cpuidle_idle_call(void) { return -ENODEV; }
>> +static inline int cpuidle_register(struct cpuidle_driver *drv)
>> +{return -ENODEV; }
>>   static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
>>   {return -ENODEV; }
>>   static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
>
>
>
> Regards,
> Srivatsa S. Bhat
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm