* Re: sched: WARNING: at include/linux/cpumask.h:108 select_fallback_rq+0x241/0x280()
2012-03-30 14:10 ` Srivatsa S. Bhat
@ 2012-03-30 14:45 ` Peter Zijlstra
2012-03-31 7:52 ` Srivatsa S. Bhat
2012-03-30 15:00 ` Sasha Levin
2012-03-31 9:46 ` [tip:sched/urgent] sched: Fix incorrect usage of for_each_cpu_mask () in select_fallback_rq() tip-bot for Srivatsa S. Bhat
2 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2012-03-30 14:45 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Sasha Levin, Ingo Molnar, Thomas Gleixner,
linux-kernel@vger.kernel.org List, Dave Jones, mingo,
Liu, Chuansheng, vapier, rusty
On Fri, 2012-03-30 at 19:40 +0530, Srivatsa S. Bhat wrote:
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: sched: Fix incorrect usage of for_each_cpu_mask() in select_fallback_rq()
>
> The function for_each_cpu_mask() expects a *pointer* to struct cpumask
> as its second argument, whereas select_fallback_rq() passes the value
> itself. And moreover, for_each_cpu_mask() has been marked as obselete
> in include/linux/cpumask.h. So move to the more appropriate for_each_cpu()
> variant.
Gah.. so why did it compile to begin with!?
> Reported-by: Sasha Levin <levinsasha928@gmail.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
>
> kernel/sched/core.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index e3ed0ec..e85046d 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1270,7 +1270,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
> int dest_cpu;
>
> /* Look for allowed, online CPU in same node. */
> - for_each_cpu_mask(dest_cpu, *nodemask) {
> + for_each_cpu(dest_cpu, nodemask) {
> if (!cpu_online(dest_cpu))
> continue;
> if (!cpu_active(dest_cpu))
> @@ -1281,7 +1281,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
>
> for (;;) {
> /* Any allowed, online CPU? */
> - for_each_cpu_mask(dest_cpu, *tsk_cpus_allowed(p)) {
> + for_each_cpu(dest_cpu, tsk_cpus_allowed(p)) {
> if (!cpu_online(dest_cpu))
> continue;
> if (!cpu_active(dest_cpu))
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: sched: WARNING: at include/linux/cpumask.h:108 select_fallback_rq+0x241/0x280()
2012-03-30 14:45 ` Peter Zijlstra
@ 2012-03-31 7:52 ` Srivatsa S. Bhat
2012-04-14 9:17 ` Peter Zijlstra
0 siblings, 1 reply; 10+ messages in thread
From: Srivatsa S. Bhat @ 2012-03-31 7:52 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Sasha Levin, Ingo Molnar, Thomas Gleixner,
linux-kernel@vger.kernel.org List, Dave Jones, mingo,
Liu, Chuansheng, vapier, rusty
On 03/30/2012 08:15 PM, Peter Zijlstra wrote:
> On Fri, 2012-03-30 at 19:40 +0530, Srivatsa S. Bhat wrote:
>
>> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Subject: sched: Fix incorrect usage of for_each_cpu_mask() in select_fallback_rq()
>>
>> The function for_each_cpu_mask() expects a *pointer* to struct cpumask
>> as its second argument, whereas select_fallback_rq() passes the value
>> itself. And moreover, for_each_cpu_mask() has been marked as obselete
>> in include/linux/cpumask.h. So move to the more appropriate for_each_cpu()
>> variant.
>
> Gah.. so why did it compile to begin with!?
One of the perils of using macros instead of true function calls :-(
Regards,
Srivatsa S. Bhat
>
>> Reported-by: Sasha Levin <levinsasha928@gmail.com>
>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>> kernel/sched/core.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index e3ed0ec..e85046d 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -1270,7 +1270,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
>> int dest_cpu;
>>
>> /* Look for allowed, online CPU in same node. */
>> - for_each_cpu_mask(dest_cpu, *nodemask) {
>> + for_each_cpu(dest_cpu, nodemask) {
>> if (!cpu_online(dest_cpu))
>> continue;
>> if (!cpu_active(dest_cpu))
>> @@ -1281,7 +1281,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
>>
>> for (;;) {
>> /* Any allowed, online CPU? */
>> - for_each_cpu_mask(dest_cpu, *tsk_cpus_allowed(p)) {
>> + for_each_cpu(dest_cpu, tsk_cpus_allowed(p)) {
>> if (!cpu_online(dest_cpu))
>> continue;
>> if (!cpu_active(dest_cpu))
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: sched: WARNING: at include/linux/cpumask.h:108 select_fallback_rq+0x241/0x280()
2012-03-31 7:52 ` Srivatsa S. Bhat
@ 2012-04-14 9:17 ` Peter Zijlstra
2012-04-14 11:40 ` Sasha Levin
0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2012-04-14 9:17 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Sasha Levin, Ingo Molnar, Thomas Gleixner,
linux-kernel@vger.kernel.org List, Dave Jones, mingo,
Liu, Chuansheng, vapier, rusty
On Sat, 2012-03-31 at 13:22 +0530, Srivatsa S. Bhat wrote:
>
> One of the perils of using macros instead of true function calls :-(
>
You can do type checking in macros too, its not pretty, but there's
several such things already, see min()/max() for example.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sched: WARNING: at include/linux/cpumask.h:108 select_fallback_rq+0x241/0x280()
2012-04-14 9:17 ` Peter Zijlstra
@ 2012-04-14 11:40 ` Sasha Levin
2012-04-14 18:12 ` Peter Zijlstra
0 siblings, 1 reply; 10+ messages in thread
From: Sasha Levin @ 2012-04-14 11:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Srivatsa S. Bhat, Ingo Molnar, Thomas Gleixner,
linux-kernel@vger.kernel.org List, Dave Jones, mingo,
Liu, Chuansheng, vapier, rusty
On Sat, Apr 14, 2012 at 11:17 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Sat, 2012-03-31 at 13:22 +0530, Srivatsa S. Bhat wrote:
>>
>> One of the perils of using macros instead of true function calls :-(
>>
> You can do type checking in macros too, its not pretty, but there's
> several such things already, see min()/max() for example.
Would it make sense to somehow standardize type checking in kernel
macros? Possibly a set of wrappers that would make type checking easy
to get into new and existing macros?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sched: WARNING: at include/linux/cpumask.h:108 select_fallback_rq+0x241/0x280()
2012-04-14 11:40 ` Sasha Levin
@ 2012-04-14 18:12 ` Peter Zijlstra
2012-04-14 18:37 ` Sasha Levin
0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2012-04-14 18:12 UTC (permalink / raw)
To: Sasha Levin
Cc: Srivatsa S. Bhat, Ingo Molnar, Thomas Gleixner,
linux-kernel@vger.kernel.org List, Dave Jones, mingo,
Liu, Chuansheng, vapier, rusty
On Sat, 2012-04-14 at 13:40 +0200, Sasha Levin wrote:
> On Sat, Apr 14, 2012 at 11:17 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Sat, 2012-03-31 at 13:22 +0530, Srivatsa S. Bhat wrote:
> >>
> >> One of the perils of using macros instead of true function calls :-(
> >>
> > You can do type checking in macros too, its not pretty, but there's
> > several such things already, see min()/max() for example.
>
> Would it make sense to somehow standardize type checking in kernel
> macros? Possibly a set of wrappers that would make type checking easy
> to get into new and existing macros?
I had a quick go with the below and that doesn't quite work for no
obvious reasons as of yet.. Also, that call out to cpumask_next() should
already do type validation for us, so still no clue why my earlier code
compiled at all.
---
include/linux/cpumask.h | 1 +
include/linux/kernel.h | 10 ++++++++++
2 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index a2c819d..aec8355 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -201,6 +201,7 @@ int cpumask_any_but(const struct cpumask *mask, unsigned int cpu);
* After the loop, cpu is >= nr_cpu_ids.
*/
#define for_each_cpu(cpu, mask) \
+ match_type(const struct cpumask *, (mask)); \
for ((cpu) = -1; \
(cpu) = cpumask_next((cpu), (mask)), \
(cpu) < nr_cpu_ids;)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 645231c..bcb7da3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -549,6 +549,16 @@ ftrace_vprintk(const char *fmt, va_list ap)
static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
#endif /* CONFIG_TRACING */
+#define match_type_type(t1, t2) \
+do { \
+ t1 uninitialized_var(____t1); \
+ t2 uninitialized_var(____t2); \
+ (void) (&____t1 == &____t2); \
+} while (0)
+
+#define match_type(t, v) \
+ match_type_type(t, typeof(v))
+
/*
* min()/max()/clamp() macros that also do
* strict type-checking.. See the
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: sched: WARNING: at include/linux/cpumask.h:108 select_fallback_rq+0x241/0x280()
2012-04-14 18:12 ` Peter Zijlstra
@ 2012-04-14 18:37 ` Sasha Levin
0 siblings, 0 replies; 10+ messages in thread
From: Sasha Levin @ 2012-04-14 18:37 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Srivatsa S. Bhat, Ingo Molnar, Thomas Gleixner,
linux-kernel@vger.kernel.org List, Dave Jones, mingo,
Liu, Chuansheng, vapier, rusty
On Sat, Apr 14, 2012 at 8:12 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Sat, 2012-04-14 at 13:40 +0200, Sasha Levin wrote:
>> On Sat, Apr 14, 2012 at 11:17 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Sat, 2012-03-31 at 13:22 +0530, Srivatsa S. Bhat wrote:
>> >>
>> >> One of the perils of using macros instead of true function calls :-(
>> >>
>> > You can do type checking in macros too, its not pretty, but there's
>> > several such things already, see min()/max() for example.
>>
>> Would it make sense to somehow standardize type checking in kernel
>> macros? Possibly a set of wrappers that would make type checking easy
>> to get into new and existing macros?
>
>
> I had a quick go with the below and that doesn't quite work for no
> obvious reasons as of yet.. Also, that call out to cpumask_next() should
> already do type validation for us, so still no clue why my earlier code
> compiled at all.
I've also started working on it, I have something that looks pretty
good and detects the bug which has caused this discussion.
I want to apply it on several more macros around the kernel and I'll
send a RFC. Basically I want to deal with the cpumask thing, update
min()/max()/etc to use it and convert the list or rbtree macros to use
it as well. It'll give us a good look on how something like that would
look around the kernel.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: sched: WARNING: at include/linux/cpumask.h:108 select_fallback_rq+0x241/0x280()
2012-03-30 14:10 ` Srivatsa S. Bhat
2012-03-30 14:45 ` Peter Zijlstra
@ 2012-03-30 15:00 ` Sasha Levin
2012-03-31 9:46 ` [tip:sched/urgent] sched: Fix incorrect usage of for_each_cpu_mask () in select_fallback_rq() tip-bot for Srivatsa S. Bhat
2 siblings, 0 replies; 10+ messages in thread
From: Sasha Levin @ 2012-03-30 15:00 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Ingo Molnar, Thomas Gleixner, Peter Zijlstra,
linux-kernel@vger.kernel.org List, Dave Jones, a.p.zijlstra,
mingo, Liu, Chuansheng, vapier, rusty
On Fri, Mar 30, 2012 at 5:10 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 03/30/2012 02:02 AM, Sasha Levin wrote:
>
>> (and now with lkml)
>>
>> Hi all,
>>
>> I got the following spew using trinity in a kvm tools guest on the
>> latest linux-next kernel.
>>
>> This is the result of trying to offline CPU1. I'm not sure how to
>> reproduce it easily besides putting some pressure on the system and
>> shutting down CPUs until it happens.
>>
>> [ 317.238839] Cannot set affinity for irq 0
>> [ 317.238839] ------------[ cut here ]------------
>> [ 317.238839] WARNING: at include/linux/cpumask.h:108
>> select_fallback_rq+0x241/0x280()
>> [ 317.238839] Pid: 13, comm: migration/1 Not tainted
>> 3.3.0-next-20120329-sasha #4
>> [ 317.238839] Call Trace:
>> [ 317.238839] [<ffffffff810b26b5>] warn_slowpath_common+0x75/0xb0
>> [ 317.238839] [<ffffffff810b2705>] warn_slowpath_null+0x15/0x20
>> [ 317.238839] [<ffffffff810e5991>] select_fallback_rq+0x241/0x280
>> [ 317.238839] [<ffffffff810f1a40>] ? dequeue_task_fair+0x100/0x100
>> [ 317.238839] [<ffffffff810f1a40>] ? dequeue_task_fair+0x100/0x100
>> [ 317.238839] [<ffffffff810ecd20>] migrate_tasks+0x80/0xf0
>> [ 317.238839] [<ffffffff826fe03a>] ? migration_call+0xae/0x16b
>> [ 317.238839] [<ffffffff826fe073>] migration_call+0xe7/0x16b
>> [ 317.238839] [<ffffffff810ddebf>] notifier_call_chain+0x5f/0x150
>> [ 317.238839] [<ffffffff810ddfb9>] __raw_notifier_call_chain+0x9/0x10
>> [ 317.238839] [<ffffffff810b489b>] __cpu_notify+0x1b/0x30
>> [ 317.238839] [<ffffffff826c3fbd>] take_cpu_down+0x2d/0x40
>> [ 317.238839] [<ffffffff811357fa>] stop_machine_cpu_stop+0xda/0x1a0
>> [ 317.238839] [<ffffffff81135720>] ? queue_stop_cpus_work+0x190/0x190
>> [ 317.238839] [<ffffffff811352ae>] cpu_stopper_thread+0xee/0x200
>> [ 317.238839] [<ffffffff82705c1a>] ? __schedule+0x49a/0x860
>> [ 317.238839] [<ffffffff811351c0>] ? res_counter_init+0x50/0x50
>> [ 317.238839] [<ffffffff810d715e>] kthread+0xbe/0xd0
>> [ 317.238839] [<ffffffff82709e74>] kernel_thread_helper+0x4/0x10
>> [ 317.238839] [<ffffffff810e3ee0>] ? finish_task_switch+0x80/0x110
>> [ 317.238839] [<ffffffff82708174>] ? retint_restore_args+0x13/0x13
>> [ 317.238839] [<ffffffff810d70a0>] ? __init_kthread_worker+0x70/0x70
>> [ 317.238839] [<ffffffff82709e70>] ? gs_change+0x13/0x13
>> [ 317.238839] ---[ end trace 79079cf527253aab ]---
>> [ 317.250645] [sched_delayed] process 2267 (trinity) no longer affine to cpu1
>> [ 317.323711] CPU 1 is now offline
>> [ 317.591059] [sched_delayed] process 1956 (trinity) no longer affine to cpu1
>> [ 317.812110] [sched_delayed] process 2004 (trinity) no longer affine to cpu1
>> [ 318.401016] [sched_delayed] process 2228 (trinity) no longer affine to cpu1
>> [ 318.581015] [sched_delayed] process 2099 (trinity) no longer affine to cpu1
>> --
>
>
>
> Does this patch help?
>
> ---
>
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: sched: Fix incorrect usage of for_each_cpu_mask() in select_fallback_rq()
>
> The function for_each_cpu_mask() expects a *pointer* to struct cpumask
> as its second argument, whereas select_fallback_rq() passes the value
> itself. And moreover, for_each_cpu_mask() has been marked as obselete
> in include/linux/cpumask.h. So move to the more appropriate for_each_cpu()
> variant.
>
> Reported-by: Sasha Levin <levinsasha928@gmail.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
Works for me.
Tested-by: Sasha Levin <levinsasha928@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip:sched/urgent] sched: Fix incorrect usage of for_each_cpu_mask () in select_fallback_rq()
2012-03-30 14:10 ` Srivatsa S. Bhat
2012-03-30 14:45 ` Peter Zijlstra
2012-03-30 15:00 ` Sasha Levin
@ 2012-03-31 9:46 ` tip-bot for Srivatsa S. Bhat
2 siblings, 0 replies; 10+ messages in thread
From: tip-bot for Srivatsa S. Bhat @ 2012-03-31 9:46 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, a.p.zijlstra, davej, srivatsa.bhat,
levinsasha928, tglx, chuansheng.liu
Commit-ID: e3831edd59edf57ca11fc289f08961b20baf5146
Gitweb: http://git.kernel.org/tip/e3831edd59edf57ca11fc289f08961b20baf5146
Author: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
AuthorDate: Fri, 30 Mar 2012 19:40:28 +0530
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 31 Mar 2012 10:43:36 +0200
sched: Fix incorrect usage of for_each_cpu_mask() in select_fallback_rq()
The function for_each_cpu_mask() expects a *pointer* to struct
cpumask as its second argument, whereas select_fallback_rq()
passes the value itself.
And moreover, for_each_cpu_mask() has been marked as obselete
in include/linux/cpumask.h. So move to the more appropriate
for_each_cpu() variant.
Reported-by: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Dave Jones <davej@redhat.com>
Cc: Liu Chuansheng <chuansheng.liu@intel.com>
Cc: vapier@gentoo.org
Cc: rusty@rustcorp.com.au
Link: http://lkml.kernel.org/r/4F75BED4.9050005@linux.vnet.ibm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/sched/core.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 985f6e5..8773176 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1268,7 +1268,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
int dest_cpu;
/* Look for allowed, online CPU in same node. */
- for_each_cpu_mask(dest_cpu, *nodemask) {
+ for_each_cpu(dest_cpu, nodemask) {
if (!cpu_online(dest_cpu))
continue;
if (!cpu_active(dest_cpu))
@@ -1279,7 +1279,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
for (;;) {
/* Any allowed, online CPU? */
- for_each_cpu_mask(dest_cpu, *tsk_cpus_allowed(p)) {
+ for_each_cpu(dest_cpu, tsk_cpus_allowed(p)) {
if (!cpu_online(dest_cpu))
continue;
if (!cpu_active(dest_cpu))
^ permalink raw reply related [flat|nested] 10+ messages in thread