powernow-k8 schedules in atomic since sunday :-(

powernow-k8 schedules in atomic since sunday :-(

[Petr Vandrovec]

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 <ashok.raj@intel.com> Sun, 30 Oct 2005 14:59:54 -0800
committer Linus Torvalds <torvalds@g5.osdl.org> 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]

Re: powernow-k8 schedules in atomic since sunday :-(

[Ashok Raj]

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

Re: powernow-k8 schedules in atomic since sunday :-(

[Zwane Mwaikambo]

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

Re: powernow-k8 schedules in atomic since sunday :-(

[Dave Jones]

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