* Preemption Broken: centrino_target busted under SMP on 2.6.20.4 @ 2007-04-05 20:33 Jeff V. Merkey 2007-04-05 20:50 ` Dave Jones 0 siblings, 1 reply; 9+ messages in thread From: Jeff V. Merkey @ 2007-04-05 20:33 UTC (permalink / raw) To: Linux kernel BUG: using smp_processor_id() in preemptible [00000001] code: kondemand/0/2473 caller is centrino_target+0xfb/0x600 [<401e3646>] debug_smp_processor_id+0x9e/0xb0 [<40112afb>] centrino_target+0xfb/0x600 [<40112a00>] centrino_target+0x0/0x600 [<40305bd9>] __cpufreq_driver_target+0x5c/0x6b [<f897a537>] do_dbs_timer+0x1bc/0x208 [cpufreq_ondemand] [<40134a46>] run_workqueue+0x85/0x125 [<40374f7f>] _spin_lock_irqsave+0x18/0x66 [<f897a37b>] do_dbs_timer+0x0/0x208 [cpufreq_ondemand] [<401353fb>] worker_thread+0xf9/0x124 [<401213b9>] default_wake_function+0x0/0xc [<40135302>] worker_thread+0x0/0x124 [<40137b37>] kthread+0xb0/0xd9 [<40137a87>] kthread+0x0/0xd9 [<40104b2f>] kernel_thread_helper+0x7/0x10 ======================= Jeff ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Preemption Broken: centrino_target busted under SMP on 2.6.20.4 2007-04-05 20:33 Preemption Broken: centrino_target busted under SMP on 2.6.20.4 Jeff V. Merkey @ 2007-04-05 20:50 ` Dave Jones 2007-04-10 0:26 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Dave Jones @ 2007-04-05 20:50 UTC (permalink / raw) To: Jeff V. Merkey; +Cc: Linux kernel, venki On Thu, Apr 05, 2007 at 02:33:03PM -0600, Jeff V. Merkey wrote: > > BUG: using smp_processor_id() in preemptible [00000001] code: > kondemand/0/2473 > caller is centrino_target+0xfb/0x600 > [<401e3646>] debug_smp_processor_id+0x9e/0xb0 > [<40112afb>] centrino_target+0xfb/0x600 > [<40112a00>] centrino_target+0x0/0x600 > [<40305bd9>] __cpufreq_driver_target+0x5c/0x6b > [<f897a537>] do_dbs_timer+0x1bc/0x208 [cpufreq_ondemand] > [<40134a46>] run_workqueue+0x85/0x125 > [<40374f7f>] _spin_lock_irqsave+0x18/0x66 > [<f897a37b>] do_dbs_timer+0x0/0x208 [cpufreq_ondemand] > [<401353fb>] worker_thread+0xf9/0x124 > [<401213b9>] default_wake_function+0x0/0xc > [<40135302>] worker_thread+0x0/0x124 > [<40137b37>] kthread+0xb0/0xd9 > [<40137a87>] kthread+0x0/0xd9 > [<40104b2f>] kernel_thread_helper+0x7/0x10 Given speedstep-centrino is obsoleted in favour of acpi-cpufreq, I'm reluctant to spend too much time turd-polishing. This big-hammer diff should fix this.. Dave diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c index f43b987..824d0a2 100644 --- a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c +++ b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c @@ -708,6 +708,7 @@ static int centrino_target (struct cpufreq_policy *policy, saved_mask = current->cpus_allowed; first_cpu = 1; cpus_clear(covered_cpus); + preempt_disable(); for_each_cpu_mask(j, online_policy_cpus) { /* * Support for SMP systems. @@ -798,6 +799,7 @@ static int centrino_target (struct cpufreq_policy *policy, } migrate_end: + preempt_enable(); set_cpus_allowed(current, saved_mask); return 0; } -- http://www.codemonkey.org.uk ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Preemption Broken: centrino_target busted under SMP on 2.6.20.4 2007-04-05 20:50 ` Dave Jones @ 2007-04-10 0:26 ` Andrew Morton 2007-04-10 2:31 ` Dave Jones 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2007-04-10 0:26 UTC (permalink / raw) To: Dave Jones; +Cc: Jeff V. Merkey, Linux kernel, venki On Thu, 5 Apr 2007 16:50:34 -0400 Dave Jones <davej@redhat.com> wrote: > On Thu, Apr 05, 2007 at 02:33:03PM -0600, Jeff V. Merkey wrote: > > > > BUG: using smp_processor_id() in preemptible [00000001] code: > > kondemand/0/2473 > > caller is centrino_target+0xfb/0x600 > > [<401e3646>] debug_smp_processor_id+0x9e/0xb0 > > [<40112afb>] centrino_target+0xfb/0x600 > > [<40112a00>] centrino_target+0x0/0x600 > > [<40305bd9>] __cpufreq_driver_target+0x5c/0x6b > > [<f897a537>] do_dbs_timer+0x1bc/0x208 [cpufreq_ondemand] > > [<40134a46>] run_workqueue+0x85/0x125 > > [<40374f7f>] _spin_lock_irqsave+0x18/0x66 > > [<f897a37b>] do_dbs_timer+0x0/0x208 [cpufreq_ondemand] > > [<401353fb>] worker_thread+0xf9/0x124 > > [<401213b9>] default_wake_function+0x0/0xc > > [<40135302>] worker_thread+0x0/0x124 > > [<40137b37>] kthread+0xb0/0xd9 > > [<40137a87>] kthread+0x0/0xd9 > > [<40104b2f>] kernel_thread_helper+0x7/0x10 > > Given speedstep-centrino is obsoleted in favour of acpi-cpufreq, > I'm reluctant to spend too much time turd-polishing. > This big-hammer diff should fix this.. > > Dave > > diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > index f43b987..824d0a2 100644 > --- a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > +++ b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > @@ -708,6 +708,7 @@ static int centrino_target (struct cpufreq_policy *policy, > saved_mask = current->cpus_allowed; > first_cpu = 1; > cpus_clear(covered_cpus); > + preempt_disable(); > for_each_cpu_mask(j, online_policy_cpus) { > /* > * Support for SMP systems. > @@ -798,6 +799,7 @@ static int centrino_target (struct cpufreq_policy *policy, > } > > migrate_end: > + preempt_enable(); > set_cpus_allowed(current, saved_mask); > return 0; > } This means we'll call set_cpus_allowed() while in atomic state, but set_cpus_allowed() does sleepy stuff. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Preemption Broken: centrino_target busted under SMP on 2.6.20.4 2007-04-10 0:26 ` Andrew Morton @ 2007-04-10 2:31 ` Dave Jones 2007-04-10 2:41 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Dave Jones @ 2007-04-10 2:31 UTC (permalink / raw) To: Andrew Morton; +Cc: Jeff V. Merkey, Linux kernel, Venki Pallipadi On Mon, Apr 09, 2007 at 05:26:51PM -0700, Andrew Morton wrote: > On Thu, 5 Apr 2007 16:50:34 -0400 > Dave Jones <davej@redhat.com> wrote: > > > diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > > index f43b987..824d0a2 100644 > > --- a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > > +++ b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > > @@ -708,6 +708,7 @@ static int centrino_target (struct cpufreq_policy *policy, > > saved_mask = current->cpus_allowed; > > first_cpu = 1; > > cpus_clear(covered_cpus); > > + preempt_disable(); > > for_each_cpu_mask(j, online_policy_cpus) { > > /* > > * Support for SMP systems. > > @@ -798,6 +799,7 @@ static int centrino_target (struct cpufreq_policy *policy, > > } > > > > migrate_end: > > + preempt_enable(); > > set_cpus_allowed(current, saved_mask); > > return 0; > > } > > This means we'll call set_cpus_allowed() while in atomic state, but > set_cpus_allowed() does sleepy stuff. Puzzled. This diff shouldn't change anything about the context we're in when we call set_cpus_allowed, and as we're not seeing warnings now, I'm not sure what I'm missing? [which may be 'the obvious', you wouldn't believe the evening I've had involving gas leaks and noxious fumes. Wheee, floaty head.] Dave -- http://www.codemonkey.org.uk ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Preemption Broken: centrino_target busted under SMP on 2.6.20.4 2007-04-10 2:31 ` Dave Jones @ 2007-04-10 2:41 ` Andrew Morton 2007-04-10 3:05 ` Dave Jones 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2007-04-10 2:41 UTC (permalink / raw) To: Dave Jones; +Cc: Jeff V. Merkey, Linux kernel, Venki Pallipadi On Mon, 9 Apr 2007 22:31:08 -0400 Dave Jones <davej@redhat.com> wrote: > On Mon, Apr 09, 2007 at 05:26:51PM -0700, Andrew Morton wrote: > > On Thu, 5 Apr 2007 16:50:34 -0400 > > Dave Jones <davej@redhat.com> wrote: > > > > > diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > > > index f43b987..824d0a2 100644 > > > --- a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > > > +++ b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > > > @@ -708,6 +708,7 @@ static int centrino_target (struct cpufreq_policy *policy, > > > saved_mask = current->cpus_allowed; > > > first_cpu = 1; > > > cpus_clear(covered_cpus); > > > + preempt_disable(); > > > for_each_cpu_mask(j, online_policy_cpus) { > > > /* > > > * Support for SMP systems. > > > @@ -798,6 +799,7 @@ static int centrino_target (struct cpufreq_policy *policy, > > > } > > > > > > migrate_end: > > > + preempt_enable(); > > > set_cpus_allowed(current, saved_mask); > > > return 0; > > > } > > > > This means we'll call set_cpus_allowed() while in atomic state, but > > set_cpus_allowed() does sleepy stuff. > > Puzzled. This diff shouldn't change anything about the context we're in > when we call set_cpus_allowed, and as we're not seeing warnings now, > I'm not sure what I'm missing? set_cpus_allowed() will only sleep in special circumstances: when we're telling the target task that it is not allwed to run on a CPU upon which it is presently executing. So it needs to be synchronously migrated off that CPU, which requires that the set_cpus_allowed() caller block. You're probably just not hitting that case. Probably we should have a might_sleep() in set_cpus_allowed(), although there might be callers who are guaranteeed to never hit that case and who might legitimately want special treatment to avoid the warning. > [which may be 'the obvious', you wouldn't believe the evening I've had > involving gas leaks and noxious fumes. Wheee, floaty head.] Yeah, I get a lot of patches like that. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Preemption Broken: centrino_target busted under SMP on 2.6.20.4 2007-04-10 2:41 ` Andrew Morton @ 2007-04-10 3:05 ` Dave Jones 2007-04-10 3:08 ` Dave Jones 0 siblings, 1 reply; 9+ messages in thread From: Dave Jones @ 2007-04-10 3:05 UTC (permalink / raw) To: Andrew Morton; +Cc: Jeff V. Merkey, Linux kernel, Venki Pallipadi On Mon, Apr 09, 2007 at 07:41:42PM -0700, Andrew Morton wrote: > > > This means we'll call set_cpus_allowed() while in atomic state, but > > > set_cpus_allowed() does sleepy stuff. > > > > Puzzled. This diff shouldn't change anything about the context we're in > > when we call set_cpus_allowed, and as we're not seeing warnings now, > > I'm not sure what I'm missing? > > set_cpus_allowed() will only sleep in special circumstances: when we're > telling the target task that it is not allwed to run on a CPU upon which it > is presently executing. So it needs to be synchronously migrated off that > CPU, which requires that the set_cpus_allowed() caller block. > > You're probably just not hitting that case. Oh, now I see it. The set_cpus_allowed that was inside the preempt stuff I was adding. (that the diff elided). Yeah, that's a problem. Bugger. > Probably we should have a might_sleep() in set_cpus_allowed(), although > there might be callers who are guaranteeed to never hit that case and who > might legitimately want special treatment to avoid the warning. This whole file is going away in .22, and we have a viable alternative in .21 (acpi-cpufreq), so I'm not overly worried about fixing this up given it only shows up in debug kernels, especially at this stage in -rc. (Yeah, it's a cop-out, but unless someone with more interest in this problem steps up, I've bigger fishes to fry). Dave -- http://www.codemonkey.org.uk ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Preemption Broken: centrino_target busted under SMP on 2.6.20.4 2007-04-10 3:05 ` Dave Jones @ 2007-04-10 3:08 ` Dave Jones 2007-04-10 3:51 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Dave Jones @ 2007-04-10 3:08 UTC (permalink / raw) To: Andrew Morton, Jeff V. Merkey, Linux kernel, Venki Pallipadi On Mon, Apr 09, 2007 at 11:05:00PM -0400, Dave Jones wrote: > This whole file is going away in .22, and we have a viable alternative in > .21 (acpi-cpufreq), so I'm not overly worried about fixing this up > given it only shows up in debug kernels, especially at this stage in -rc. > > (Yeah, it's a cop-out, but unless someone with more interest in this problem > steps up, I've bigger fishes to fry). One last try... (I didn't think too long about this, so this might be equally busted, but if so, see comment above). Dave diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c index f43b987..38e31ce 100644 --- a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c +++ b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c @@ -720,11 +720,13 @@ static int centrino_target (struct cpufreq_policy *policy, cpu_set(j, set_mask); set_cpus_allowed(current, set_mask); + preempt_disable(); if (unlikely(!cpu_isset(smp_processor_id(), set_mask))) { dprintk("couldn't limit to CPUs in this domain\n"); retval = -EAGAIN; if (first_cpu) { /* We haven't started the transition yet. */ + preempt_enable(); goto migrate_end; } break; @@ -765,6 +767,7 @@ static int centrino_target (struct cpufreq_policy *policy, break; cpu_set(j, covered_cpus); + preempt_enable(); } for_each_cpu_mask(k, online_policy_cpus) { -- http://www.codemonkey.org.uk ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Preemption Broken: centrino_target busted under SMP on 2.6.20.4 2007-04-10 3:08 ` Dave Jones @ 2007-04-10 3:51 ` Andrew Morton 2007-04-10 4:29 ` Dave Jones 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2007-04-10 3:51 UTC (permalink / raw) To: Dave Jones; +Cc: Jeff V. Merkey, Linux kernel, Venki Pallipadi On Mon, 9 Apr 2007 23:08:23 -0400 Dave Jones <davej@redhat.com> wrote: > > This whole file is going away in .22, and we have a viable alternative in > > .21 (acpi-cpufreq), so I'm not overly worried about fixing this up > > given it only shows up in debug kernels, especially at this stage in -rc. > > > > (Yeah, it's a cop-out, but unless someone with more interest in this problem > > steps up, I've bigger fishes to fry). > > One last try... > (I didn't think too long about this, so this might be equally busted, > but if so, see comment above). > > Dave > > diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > index f43b987..38e31ce 100644 > --- a/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > +++ b/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c > @@ -720,11 +720,13 @@ static int centrino_target (struct cpufreq_policy *policy, > cpu_set(j, set_mask); > > set_cpus_allowed(current, set_mask); > + preempt_disable(); > if (unlikely(!cpu_isset(smp_processor_id(), set_mask))) { > dprintk("couldn't limit to CPUs in this domain\n"); > retval = -EAGAIN; > if (first_cpu) { > /* We haven't started the transition yet. */ > + preempt_enable(); > goto migrate_end; > } > break; > @@ -765,6 +767,7 @@ static int centrino_target (struct cpufreq_policy *policy, > break; > > cpu_set(j, covered_cpus); > + preempt_enable(); > } > Yes, I expect that should squish the warnings. It looks all racy wrt cpu hotplug and against async set_cpus_allowed(), but if those are our worst problems, we're good. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Preemption Broken: centrino_target busted under SMP on 2.6.20.4 2007-04-10 3:51 ` Andrew Morton @ 2007-04-10 4:29 ` Dave Jones 0 siblings, 0 replies; 9+ messages in thread From: Dave Jones @ 2007-04-10 4:29 UTC (permalink / raw) To: Andrew Morton; +Cc: Jeff V. Merkey, Linux kernel, Venki Pallipadi On Mon, Apr 09, 2007 at 08:51:36PM -0700, Andrew Morton wrote: > On Mon, 9 Apr 2007 23:08:23 -0400 Dave Jones <davej@redhat.com> wrote: > > > > This whole file is going away in .22, and we have a viable alternative in > > > .21 (acpi-cpufreq), so I'm not overly worried about fixing this up > > > given it only shows up in debug kernels, especially at this stage in -rc. > > > > > > (Yeah, it's a cop-out, but unless someone with more interest in this problem > > > steps up, I've bigger fishes to fry). > > > > One last try... > > (I didn't think too long about this, so this might be equally busted, > > but if so, see comment above). > > Yes, I expect that should squish the warnings. It looks all racy wrt cpu hotplug > and against async set_cpus_allowed(), but if those are our worst problems, we're > good. It probably needs a couple more preempt_enable()'s sprinkled throughout the function to take care of the break's. I also missed a goto case. Meh, this cure is as bad as the disease. Dave -- http://www.codemonkey.org.uk ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2007-04-10 4:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-04-05 20:33 Preemption Broken: centrino_target busted under SMP on 2.6.20.4 Jeff V. Merkey 2007-04-05 20:50 ` Dave Jones 2007-04-10 0:26 ` Andrew Morton 2007-04-10 2:31 ` Dave Jones 2007-04-10 2:41 ` Andrew Morton 2007-04-10 3:05 ` Dave Jones 2007-04-10 3:08 ` Dave Jones 2007-04-10 3:51 ` Andrew Morton 2007-04-10 4:29 ` Dave Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox