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

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 03/30/2012 05:15 PM, Daniel Lezcano wrote:

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


Well, that depends on when/where this function gets called.
This patch introduces the function. Where is the caller?

As of now, if you are calling this in boot-up code, its not racy.
However, there have been attempts to speed up boot times by trying
to online cpus in parallel with the rest of the kernel initialization[1].
In that case, unless your call is an early init call, it can race
with CPU hotplug.

[1]. https://lkml.org/lkml/2012/1/30/647

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


What about servers with a lot more CPUs, like say 128 or even more? :-)

Moreover I don't see any downsides to the optimization. So should be good
to add it in any case...
 
Regards,
Srivatsa S. Bhat

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