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