Re: [PATCH 4/4] cpuidle - support multiple drivers

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

On 10/11/2012 07:21 PM, Rafael J. Wysocki wrote:
> On Thursday 11 of October 2012 12:04:37 Daniel Lezcano wrote:
>> On 10/07/2012 11:26 PM, Rafael J. Wysocki wrote:
>>> 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.
>> Thanks Rafael for the review.
>>
>> I took into account all your remarks for the V2.
>>
>> [ cut ]
>>
>>>> +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?
>> AFAICS, the cpumask.h is not assuming the cpu numbers start from zero
>> and they are contiguous.
>>
>> I will fix this reverse loop, thanks for spotting this.
>>
>> [ cut ]
>>
>>>>  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).
>> yes, get_cpu does preempt_disable() and smp_processor_id()
>> As spin_lock does also preempt_disable() that should be ok.
>> But I changed the code to use the preferred way.
> Cool, thanks!
>
> I've seen your new version 

Actually, the patchset is just sysfs cleanup for the multiple drivers
support.
The patches are trivial IMO. If you think they are ok and they are
merged, I will send the multiple drivers patchset V2 on top of them.

> and I'm going to look deeper into it in the next
> few days, but I will be travelling from tomorrow through the end of the next
> week, so I may be even slower to respond than usually. Sorry about that.

No problem, I understand.

Thanks
-- Daniel

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


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html