Hello Ashok,
your change '[PATCH] create and destroy cpufreq sysfs entries based on cpu
notifiers' causes problems with powernow-k8 driver. powernow-k8 uses
set_cpus_allowed() (it even calls schedule() explicitly for no reason), and when
you've changed code from lock_cpu_hotplug() to preempt_disable()
set_cpus_allowed() now complains that schedule() is not allowed while preemption
is disabled...
I think that some better solution is needed. As I want cpufreq while my
system does not support CPU hotplug I've put preempt_{disable,enable} calls you
added to the __cpufreq_driver_target in my kernel under CONFIG_HOTPLUG_CPU, and
my system is now as happy as it was before your change. But I'm sure there is
solution to use both CPU hotplug and cpufreq at same time...
Kernel on the system is built with SMP and PREEMPT enabled, together with all
debugging you can think about.
Thanks,
Petr Vandrovec
-- Responsible change --
commit c32b6b8e524d2c337767d312814484d9289550cf
tree 02e634b0b48db6eccc8774369366daa1893921ea
parent d434fca737bee0862625c2377b987a7713b6b487
author Ashok Raj <[email protected]> Sun, 30 Oct 2005 14:59:54 -0800
committer Linus Torvalds <[email protected]> Sun, 30 Oct 2005 17:37:14 -0800
[PATCH] create and destroy cpufreq sysfs entries based on cpu notifiers
- Converting lock_cpu_hotplug()/unlock_cpu_hotplug() to disable/enable preempt.
The locking was smack in the middle of the notification path, when the
hotplug is already holding the lock. I tried another solution to avoid this
so avoid taking locks if we know we are from notification path. The solution
was getting very ugly and i decided this was probably good for this iteration
until someone who understands cpufreq could do a better job than me.
-- relevant dmesg part --
powernow-k8: Found 1 AMD Athlon 64 / Opteron processors (version 1.50.4)
powernow-k8: 0 : fid 0xc (2000 MHz), vid 0x6 (1400 mV)
powernow-k8: 1 : fid 0xa (1800 MHz), vid 0x8 (1350 mV)
powernow-k8: 2 : fid 0x2 (1000 MHz), vid 0x12 (1100 mV)
cpu_init done, current fid 0xc, vid 0x6
scheduling while atomic: modprobe/0x00000001/3920
Call Trace:<ffffffff801cc745>{sysfs_new_dirent+37} <ffffffff80371c3d>{schedule+125}
<ffffffff80172049>{kmem_cache_alloc+153}
<ffffffff803746d5>{_spin_unlock_irqrestore+21}
<ffffffff801371cf>{set_cpus_allowed+351}
<ffffffff801cc7c9>{sysfs_make_dirent+41}
<ffffffff8016f660>{check_poison_obj+48} <ffffffff8016f4a6>{poison_obj+70}
<ffffffff881c4074>{:powernow_k8:powernowk8_target+100}
<ffffffff803029aa>{__cpufreq_driver_target+90}
<ffffffff80303bbb>{cpufreq_governor_performance+27}
<ffffffff80302b31>{__cpufreq_governor+145}
<ffffffff80302fe2>{__cpufreq_set_policy+306}
<ffffffff80303077>{cpufreq_set_policy+87}
<ffffffff803020c4>{cpufreq_add_dev+916}
<ffffffff803024e0>{handle_update+0} <ffffffff8015b360>{__link_module+0}
<ffffffff8029d969>{sysdev_driver_register+169}
<ffffffff80303278>{cpufreq_register_driver+136}
<ffffffff8015b4d3>{sys_init_module+323} <ffffffff80126881>{ia32_sysret+0}
Debug: sleeping function called from invalid context at include/asm/semaphore.h:105
in_atomic():1, irqs_disabled():0
Call Trace:<ffffffff801392d6>{__might_sleep+198}
<ffffffff881c4123>{:powernow_k8:powernowk8_target+275}
<ffffffff803029aa>{__cpufreq_driver_target+90}
<ffffffff80303bbb>{cpufreq_governor_performance+27}
<ffffffff80302b31>{__cpufreq_governor+145}
<ffffffff80302fe2>{__cpufreq_set_policy+306}
<ffffffff80303077>{cpufreq_set_policy+87}
<ffffffff803020c4>{cpufreq_add_dev+916}
<ffffffff803024e0>{handle_update+0} <ffffffff8015b360>{__link_module+0}
<ffffffff8029d969>{sysdev_driver_register+169}
<ffffffff80303278>{cpufreq_register_driver+136}
<ffffffff8015b4d3>{sys_init_module+323} <ffffffff80126881>{ia32_sysret+0}
[hundreds of same/simillar messages removed]
On Thu, Nov 03, 2005 at 05:03:17PM +0100, Petr Vandrovec wrote:
> Hello Ashok,
> your change '[PATCH] create and destroy cpufreq sysfs entries based on cpu
> notifiers' causes problems with powernow-k8 driver. powernow-k8 uses
> set_cpus_allowed() (it even calls schedule() explicitly for no reason), and when
> you've changed code from lock_cpu_hotplug() to preempt_disable()
> set_cpus_allowed() now complains that schedule() is not allowed while preemption
> is disabled...
>
Rafael noticed this, please use the new patch
http://marc.theaimsgroup.com/?l=linux-kernel&m=113087258615780&w=2
He confirmed it works for him now.
Thanks
ashok
On Thu, 3 Nov 2005, Ashok Raj wrote:
> On Thu, Nov 03, 2005 at 05:03:17PM +0100, Petr Vandrovec wrote:
> > Hello Ashok,
> > your change '[PATCH] create and destroy cpufreq sysfs entries based on cpu
> > notifiers' causes problems with powernow-k8 driver. powernow-k8 uses
> > set_cpus_allowed() (it even calls schedule() explicitly for no reason), and when
> > you've changed code from lock_cpu_hotplug() to preempt_disable()
> > set_cpus_allowed() now complains that schedule() is not allowed while preemption
> > is disabled...
Hmm i submitted a patch
(http://groups.google.ca/group/fa.linux.kernel/browse_frm/thread/ec079d77dc1f6e80/edf10a39eede6246?tvc=1&q=Remove+p$
to remove those superfluous schedules, but perhaps it hasn't hit Linus'
tree yet via Davej.
> Rafael noticed this, please use the new patch
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=113087258615780&w=2
>
> He confirmed it works for him now.
Perhaps we need something like the following (untested), basically allow a
small window and if we migrate within that window, simply fail. This
should be fine as during hotplug notification that processor won't be
going offline as we hold the cpucontrol semaphore and during normal
operation without hotplug cpu the processor won't be going offline anyway.
Have i missed a special case? One thing i don't like about this approach
is that it relies on the driver writer to get this logic correct, higher
up might be better.
Zwane
Index: linux-2.6.14-rc5-mm1/arch/i386/kernel/cpu/cpufreq/powernow-k8.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.14-rc5-mm1/arch/i386/kernel/cpu/cpufreq/powernow-k8.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 powernow-k8.c
--- linux-2.6.14-rc5-mm1/arch/i386/kernel/cpu/cpufreq/powernow-k8.c 24 Oct 2005 15:38:38 -0000 1.1.1.1
+++ linux-2.6.14-rc5-mm1/arch/i386/kernel/cpu/cpufreq/powernow-k8.c 6 Nov 2005 01:10:34 -0000
@@ -463,6 +463,7 @@ static int check_supported_cpu(unsigned
oldmask = current->cpus_allowed;
set_cpus_allowed(current, cpumask_of_cpu(cpu));
+ preempt_disable();
if (smp_processor_id() != cpu) {
printk(KERN_ERR "limiting to cpu %u failed\n", cpu);
goto out;
@@ -495,6 +496,7 @@ static int check_supported_cpu(unsigned
rc = 1;
out:
+ preempt_enable();
set_cpus_allowed(current, oldmask);
return rc;
}
@@ -911,6 +913,7 @@ static int powernowk8_target(struct cpuf
oldmask = current->cpus_allowed;
set_cpus_allowed(current, cpumask_of_cpu(pol->cpu));
+ preempt_disable();
if (smp_processor_id() != pol->cpu) {
printk(KERN_ERR "limiting to cpu %u failed\n", pol->cpu);
goto err_out;
@@ -941,6 +944,7 @@ static int powernowk8_target(struct cpuf
if (cpufreq_frequency_table_target(pol, data->powernow_table, targfreq, relation, &newstate))
goto err_out;
+ preempt_enable();
down(&fidvid_sem);
powernow_k8_acpi_pst_values(data, newstate);
@@ -961,8 +965,11 @@ static int powernowk8_target(struct cpuf
pol->cur = find_khz_freq_from_fid(data->currfid);
ret = 0;
+ goto done;
err_out:
+ preempt_enable();
+done:
set_cpus_allowed(current, oldmask);
return ret;
}
@@ -1020,6 +1027,7 @@ static int __init powernowk8_cpu_init(st
oldmask = current->cpus_allowed;
set_cpus_allowed(current, cpumask_of_cpu(pol->cpu));
+ preempt_disable();
if (smp_processor_id() != pol->cpu) {
printk(KERN_ERR "limiting to cpu %u failed\n", pol->cpu);
goto err_out;
@@ -1036,6 +1044,7 @@ static int __init powernowk8_cpu_init(st
fidvid_msr_init();
/* run on any CPU again */
+ preempt_enable();
set_cpus_allowed(current, oldmask);
pol->governor = CPUFREQ_DEFAULT_GOVERNOR;
@@ -1070,6 +1079,7 @@ static int __init powernowk8_cpu_init(st
return 0;
err_out:
+ preempt_enable();
set_cpus_allowed(current, oldmask);
powernow_k8_cpu_exit_acpi(data);
@@ -1101,10 +1111,11 @@ static unsigned int powernowk8_get (unsi
unsigned int khz = 0;
set_cpus_allowed(current, cpumask_of_cpu(cpu));
+
+ preempt_disable();
if (smp_processor_id() != cpu) {
printk(KERN_ERR PFX "limiting to CPU %d failed in powernowk8_get\n", cpu);
- set_cpus_allowed(current, oldmask);
- return 0;
+ goto out;
}
if (query_current_values_with_pending_wait(data))
@@ -1113,6 +1124,7 @@ static unsigned int powernowk8_get (unsi
khz = find_khz_freq_from_fid(data->currfid);
out:
+ preempt_enable();
set_cpus_allowed(current, oldmask);
return khz;
}
On Sat, Nov 05, 2005 at 05:13:07PM -0800, Zwane Mwaikambo wrote:
> On Thu, 3 Nov 2005, Ashok Raj wrote:
>
> > On Thu, Nov 03, 2005 at 05:03:17PM +0100, Petr Vandrovec wrote:
> > > Hello Ashok,
> > > your change '[PATCH] create and destroy cpufreq sysfs entries based on cpu
> > > notifiers' causes problems with powernow-k8 driver. powernow-k8 uses
> > > set_cpus_allowed() (it even calls schedule() explicitly for no reason), and when
> > > you've changed code from lock_cpu_hotplug() to preempt_disable()
> > > set_cpus_allowed() now complains that schedule() is not allowed while preemption
> > > is disabled...
>
> Hmm i submitted a patch
> (http://groups.google.ca/group/fa.linux.kernel/browse_frm/thread/ec079d77dc1f6e80/edf10a39eede6246?tvc=1&q=Remove+p$
> to remove those superfluous schedules, but perhaps it hasn't hit Linus'
> tree yet via Davej.
I sent Linus a pull request last week, but I did it at the same
time osdl's mailserver had issues aparently.
I resent another request earlier this afternoon.
Dave