On 03/30/2012 09:48 PM, Daniel Lezcano wrote:
> On 03/30/2012 01:59 PM, Srivatsa S. Bhat wrote:
>> 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@xxxxxxxxxx>
>>>>> ---
>>>>> 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?
>
> There is no caller for the moment because they are in the different arch
> specific code in the different trees.
>
> But the callers will be in the init calls at boot up.
>
>> As of now, if you are calling this in boot-up code, its not racy.
>
> Most of the caller are in the boot-up code, in device_init or
> module_init. The other ones are doing some specific initialization on
> the cpuidle_device (cpuinit, like acpi) and can't use the
> cpuidle_register function.
>
>> 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
>
> Aha ! Now I understand the race you were talking about. Thanks for the
> pointer. It is very interesting.
>
> I realize if the cpus boot up in parallel, that will break a lot of
> things and, for my concern, that will break most of the cpuidle drivers.
>
Exactly!
> So far the cpu bootup parallelization is not there, so from my POV, my
> patch is correct as we will factor out in a single place some code which
> will be potentially broken by this parallelization in the future. It
> will be easier to fix that in a single place rather in multiple drivers.
>
> Thanks for spotting this potential problem. This is something I will
> keep in mind for the future.
>
Sure, that would be great!
>>>>> +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...
>
> Yes, no problem. I will add it.
>
Thanks!
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
