RE: CPUFREQ-CPUHOTPLUG: Possible circular locking dependency

Pallipadi, Venkatesh
2006-12-06T18:38:00Z

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org
>[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of
>Gautham R Shenoy
>Sent: Thursday, November 30, 2006 3:44 AM
>To: Ingo Molnar
>Cc: Gautham R Shenoy; akpm@osdl.org;
>linux-kernel@vger.kernel.org; torvalds@osdl.org;
>davej@redhat.com; dipankar@in.ibm.com; vatsa@in.ibm.com
>Subject: Re: CPUFREQ-CPUHOTPLUG: Possible circular locking dependency
>
>On Thu, Nov 30, 2006 at 12:03:15PM +0100, Ingo Molnar wrote:
>>
>> * Gautham R Shenoy <ego@in.ibm.com> wrote:
>>
>> > a) cpufreq maintain's it's own cpumask in the variable
>> > policy->affected_cpus and says : If a frequency change is issued to
>> > any one of the cpu's in the affected_cpus mask, you change
>frequency
>> > on all cpus in the mask. So this needs to be consistent with
>> > cpu_online map and hence cpu hotplug aware. Furthermore,
>we don't want
>> > cpus in this mask to go down when we are trying to change
>frequencies
>> > on them. The function which drives the frequency change in
>> > cpufreq-core is cpufreq_driver_target and it needs cpu-hotplug
>> > protection.
>>
>> couldnt this complexity be radically simplified by having new kernel
>> infrastructure that does something like:
>>
>> " 'gather' all CPUs mentioned in <mask> via scheduling a separate
>> helper-kthread on every CPU that <mask> specifies, disable all
>> interrupts, and execute function once all CPUs have been
>> 'gathered' - and release all CPUs once has executed
>on each of
>> them."
>>
>> ?
>
>This is what is currently being done by cpufreq:
>
>a) get_some_cpu_hotplug_protection() [use either some global mechanism
> or a persubsystem mutex]
>
>b) actual_freq_change_driver_function(mask)
>/* You can check out cpufreq_p4_target() in
> * arch/i386/kernel/cpu/cpufreq/p4-clockmod.c
> */
>
> {
> for_each_cpu_mask(i, mask) {
> cpumask_t this_cpu = cpumask_of_cpu(i);
> set_cpus_allowed(current, this_cpu);
> function_to_change_frequency();
>
> }
> }
>

As there are many options being discussed here, let me propose one
more option that can eliminate the need for hotplug lock in
cpufreq_driver_target() path.

As Gautham clearly explained before, today we have cpufreq calling
cpufreq_driver_target() in each driver to change the CPU frequency
And the driver internally uses set_cpus_allowed to reschedule onto
different affected_cpus and change the frequency. That is the main
reason why we need to disable hotplug in this path.

But, if we make cpufreq more affected_cpus aware and have a per_cpu
target()
call by moving set_cpus_allowed() from driver into cpufreq core and
define
the target function to be atomic/non-sleeping type, then we really don't
need a hotplug lock for the driver any more. Driver can have
get_cpu/put_cpu
pair to disable preemption and then change the frequency.

This means a lot of changes as we need new interface changes to cpufreq
and
rewrite of bunch of drivers. But, this looks to me as the least
complicated solution.

Thanks,
Venki