From: richard -r. w. <ric...@gm...> - 2011-03-17 12:25:15
|
Lai, Your commit 2e12978a (futex,plist: Pass the real head of the priority list to plist_del()) triggers gazillions warnings on User Mode Linux (x86, Linus' tree as of today): ------------[ cut here ]------------ WARNING: at kernel/futex.c:786 __unqueue_futex+0x12/0x16() Modules linked in: 27d4cce8: [<081c13fe>] dump_stack+0x1c/0x20 27d4cd00: [<08071aa0>] warn_slowpath_common+0x49/0x5f 27d4cd18: [<08071acb>] warn_slowpath_null+0x15/0x19 27d4cd28: [<0808fdb8>] __unqueue_futex+0x12/0x16 27d4cd38: [<0808fef3>] futex_wait+0x137/0x1ed 27d4cdd0: [<080910c3>] do_futex+0x78/0x7c4 27d4cedc: [<080918e6>] sys_futex+0xd7/0xed 27d4cf28: [<0805abf6>] handle_syscall+0x7a/0x98 27d4cf78: [<080685d9>] userspace+0x2c9/0x370 27d4cfe0: [<08058be5>] fork_handler+0x53/0x5b 27d4cffc: [<00000000>] 0x0 ---[ end trace ed7709f235f82328 ]--- !spin_is_locked(q->lock_ptr) in the WARN_ON() at line 786 triggers the warnings. I'm not sure whether this shows a bug within UML or the WARN_ON() is wrong. What do you think? -- Thanks, //richard |
From: richard -r. w. <ric...@gm...> - 2011-03-17 14:27:35
|
On Thu, Mar 17, 2011 at 3:08 PM, Steven Rostedt <ro...@go...> wrote: > On Thu, 2011-03-17 at 13:25 +0100, richard -rw- weinberger wrote: >> Lai, >> >> Your commit 2e12978a >> (futex,plist: Pass the real head of the priority list to plist_del()) >> triggers gazillions warnings on User Mode Linux (x86, Linus' tree as of today): >> >> ------------[ cut here ]------------ >> WARNING: at kernel/futex.c:786 __unqueue_futex+0x12/0x16() >> Modules linked in: >> 27d4cce8: [<081c13fe>] dump_stack+0x1c/0x20 >> 27d4cd00: [<08071aa0>] warn_slowpath_common+0x49/0x5f >> 27d4cd18: [<08071acb>] warn_slowpath_null+0x15/0x19 >> 27d4cd28: [<0808fdb8>] __unqueue_futex+0x12/0x16 >> 27d4cd38: [<0808fef3>] futex_wait+0x137/0x1ed >> 27d4cdd0: [<080910c3>] do_futex+0x78/0x7c4 >> 27d4cedc: [<080918e6>] sys_futex+0xd7/0xed >> 27d4cf28: [<0805abf6>] handle_syscall+0x7a/0x98 >> 27d4cf78: [<080685d9>] userspace+0x2c9/0x370 >> 27d4cfe0: [<08058be5>] fork_handler+0x53/0x5b >> 27d4cffc: [<00000000>] 0x0 >> >> ---[ end trace ed7709f235f82328 ]--- >> >> !spin_is_locked(q->lock_ptr) in the WARN_ON() at line 786 >> triggers the warnings. > > Crap, I bet you are running CONFIG_SMP=n. I'll test this. We may need to > make that a WARN_ON_SMP(). BTW: When using WARN_ON_SMP() the if-statement in __unqueue_futex will not longer work. WARN_ON_SMP() is defined as "do { } while (0)" on non CONFIG_SMP systems. > -- Steve > >> >> I'm not sure whether this shows a bug within UML >> or the WARN_ON() is wrong. >> >> What do you think? >> > > > -- Thanks, //richard |
From: Steven R. <ro...@go...> - 2011-03-17 14:31:49
|
On Thu, 2011-03-17 at 15:27 +0100, richard -rw- weinberger wrote: > BTW: When using WARN_ON_SMP() the if-statement in __unqueue_futex > will not longer work. > > WARN_ON_SMP() is defined as "do { } while (0)" on non CONFIG_SMP systems. Good point. We probably should fix that too. -- Steve |
From: Geert U. <ge...@li...> - 2011-03-17 17:47:18
|
On Thu, Mar 17, 2011 at 18:02, Eric Dumazet <eri...@gm...> wrote: > Le jeudi 17 mars 2011 à 12:55 -0400, Steven Rostedt a écrit : >> Here, test this patch. I'm in the process of committing it now. >> It will be two patches, one for the WARN_ON_SMP() change, the other for >> the futex change. >> >> -- Steve >> >> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h >> index c2c9ba0..25f1e9e 100644 >> --- a/include/asm-generic/bug.h >> +++ b/include/asm-generic/bug.h >> @@ -168,7 +168,7 @@ extern void warn_slowpath_null(const char *file, const int line); >> #ifdef CONFIG_SMP >> # define WARN_ON_SMP(x) WARN_ON(x) >> #else >> -# define WARN_ON_SMP(x) do { } while (0) >> +# define WARN_ON_SMP(x) ({0;}) >> #endif >> > > You meant : > > # define WARN_ON_SMP(x) ({x;}) > > or > > # define WARN_ON_SMP(x) do { } while (x, 0) > > ? I don't know. It's not clear to me if "WARN_ON_SMP(x)" should always return "x", or only on SMP? E.g. if (WARN_ON_SMP(x)) { // Should we get here if "x" is true? // Or only if CONFIG_SMP and "x" are both true? } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@li... In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds |
From: Geert U. <ge...@li...> - 2011-03-17 18:25:44
|
On Thu, Mar 17, 2011 at 19:16, Steven Rostedt <ro...@go...> wrote: > On Thu, 2011-03-17 at 18:02 +0100, Eric Dumazet wrote: >> Le jeudi 17 mars 2011 à 12:55 -0400, Steven Rostedt a écrit : >> > Here, test this patch. I'm in the process of committing it now. >> > It will be two patches, one for the WARN_ON_SMP() change, the other for >> > the futex change. >> > >> > -- Steve >> > >> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h >> > index c2c9ba0..25f1e9e 100644 >> > --- a/include/asm-generic/bug.h >> > +++ b/include/asm-generic/bug.h >> > @@ -168,7 +168,7 @@ extern void warn_slowpath_null(const char *file, const int line); >> > #ifdef CONFIG_SMP >> > # define WARN_ON_SMP(x) WARN_ON(x) >> > #else >> > -# define WARN_ON_SMP(x) do { } while (0) >> > +# define WARN_ON_SMP(x) ({0;}) >> > #endif >> > >> >> You meant : >> >> # define WARN_ON_SMP(x) ({x;}) >> >> or >> >> # define WARN_ON_SMP(x) do { } while (x, 0) >> >> ? >> > > Does if (do { } while (x, 0)) work? No. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@li... In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds |
From: Steven R. <ro...@go...> - 2011-03-17 18:38:29
|
On Thu, 2011-03-17 at 19:25 +0100, Geert Uytterhoeven wrote: > > Does if (do { } while (x, 0)) work? > > No. Hehe, that was a rhetorical question ;) -- Steve |
From: Steven R. <ro...@go...> - 2011-03-17 14:24:36
|
On Thu, 2011-03-17 at 13:25 +0100, richard -rw- weinberger wrote: > Lai, > > Your commit 2e12978a > (futex,plist: Pass the real head of the priority list to plist_del()) > triggers gazillions warnings on User Mode Linux (x86, Linus' tree as of today): > > ------------[ cut here ]------------ > WARNING: at kernel/futex.c:786 __unqueue_futex+0x12/0x16() > Modules linked in: > 27d4cce8: [<081c13fe>] dump_stack+0x1c/0x20 > 27d4cd00: [<08071aa0>] warn_slowpath_common+0x49/0x5f > 27d4cd18: [<08071acb>] warn_slowpath_null+0x15/0x19 > 27d4cd28: [<0808fdb8>] __unqueue_futex+0x12/0x16 > 27d4cd38: [<0808fef3>] futex_wait+0x137/0x1ed > 27d4cdd0: [<080910c3>] do_futex+0x78/0x7c4 > 27d4cedc: [<080918e6>] sys_futex+0xd7/0xed > 27d4cf28: [<0805abf6>] handle_syscall+0x7a/0x98 > 27d4cf78: [<080685d9>] userspace+0x2c9/0x370 > 27d4cfe0: [<08058be5>] fork_handler+0x53/0x5b > 27d4cffc: [<00000000>] 0x0 > > ---[ end trace ed7709f235f82328 ]--- > > !spin_is_locked(q->lock_ptr) in the WARN_ON() at line 786 > triggers the warnings. Crap, I bet you are running CONFIG_SMP=n. I'll test this. We may need to make that a WARN_ON_SMP(). -- Steve > > I'm not sure whether this shows a bug within UML > or the WARN_ON() is wrong. > > What do you think? > |
From: richard -r. w. <ric...@gm...> - 2011-03-17 14:17:13
|
On Thu, Mar 17, 2011 at 3:08 PM, Steven Rostedt <ro...@go...> wrote: > On Thu, 2011-03-17 at 13:25 +0100, richard -rw- weinberger wrote: >> Lai, >> >> Your commit 2e12978a >> (futex,plist: Pass the real head of the priority list to plist_del()) >> triggers gazillions warnings on User Mode Linux (x86, Linus' tree as of today): >> >> ------------[ cut here ]------------ >> WARNING: at kernel/futex.c:786 __unqueue_futex+0x12/0x16() >> Modules linked in: >> 27d4cce8: [<081c13fe>] dump_stack+0x1c/0x20 >> 27d4cd00: [<08071aa0>] warn_slowpath_common+0x49/0x5f >> 27d4cd18: [<08071acb>] warn_slowpath_null+0x15/0x19 >> 27d4cd28: [<0808fdb8>] __unqueue_futex+0x12/0x16 >> 27d4cd38: [<0808fef3>] futex_wait+0x137/0x1ed >> 27d4cdd0: [<080910c3>] do_futex+0x78/0x7c4 >> 27d4cedc: [<080918e6>] sys_futex+0xd7/0xed >> 27d4cf28: [<0805abf6>] handle_syscall+0x7a/0x98 >> 27d4cf78: [<080685d9>] userspace+0x2c9/0x370 >> 27d4cfe0: [<08058be5>] fork_handler+0x53/0x5b >> 27d4cffc: [<00000000>] 0x0 >> >> ---[ end trace ed7709f235f82328 ]--- >> >> !spin_is_locked(q->lock_ptr) in the WARN_ON() at line 786 >> triggers the warnings. > > Crap, I bet you are running CONFIG_SMP=n. I'll test this. We may need to > make that a WARN_ON_SMP(). Yes, UML does not support SMP. > -- Steve > >> >> I'm not sure whether this shows a bug within UML >> or the WARN_ON() is wrong. >> >> What do you think? >> > > > -- Thanks, //richard |
From: Steven R. <ro...@go...> - 2011-03-17 16:55:17
|
Here, test this patch. I'm in the process of committing it now. It will be two patches, one for the WARN_ON_SMP() change, the other for the futex change. -- Steve diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index c2c9ba0..25f1e9e 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -168,7 +168,7 @@ extern void warn_slowpath_null(const char *file, const int line); #ifdef CONFIG_SMP # define WARN_ON_SMP(x) WARN_ON(x) #else -# define WARN_ON_SMP(x) do { } while (0) +# define WARN_ON_SMP(x) ({0;}) #endif #endif diff --git a/kernel/futex.c b/kernel/futex.c index 9fe9131..850d00b 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -785,8 +785,8 @@ static void __unqueue_futex(struct futex_q *q) { struct futex_hash_bucket *hb; - if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr) - || plist_node_empty(&q->list))) + if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr)) + || WARN_ON(plist_node_empty(&q->list))) return; hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock); |
From: richard -r. w. <ric...@gm...> - 2011-03-17 17:19:06
|
On Thu, Mar 17, 2011 at 5:55 PM, Steven Rostedt <ro...@go...> wrote: > Here, test this patch. I'm in the process of committing it now. > It will be two patches, one for the WARN_ON_SMP() change, the other for > the futex change. > > -- Steve > > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h > index c2c9ba0..25f1e9e 100644 > --- a/include/asm-generic/bug.h > +++ b/include/asm-generic/bug.h > @@ -168,7 +168,7 @@ extern void warn_slowpath_null(const char *file, const int line); > #ifdef CONFIG_SMP > # define WARN_ON_SMP(x) WARN_ON(x) > #else > -# define WARN_ON_SMP(x) do { } while (0) > +# define WARN_ON_SMP(x) ({0;}) > #endif > > #endif > diff --git a/kernel/futex.c b/kernel/futex.c > index 9fe9131..850d00b 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -785,8 +785,8 @@ static void __unqueue_futex(struct futex_q *q) > { > struct futex_hash_bucket *hb; > > - if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr) > - || plist_node_empty(&q->list))) > + if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr)) > + || WARN_ON(plist_node_empty(&q->list))) > return; > > hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock); > > > Tested-by: Richard Weinberger <ri...@no...> -- Thanks, //richard |
From: Eric D. <eri...@gm...> - 2011-03-17 17:11:49
|
Le jeudi 17 mars 2011 à 12:55 -0400, Steven Rostedt a écrit : > Here, test this patch. I'm in the process of committing it now. > It will be two patches, one for the WARN_ON_SMP() change, the other for > the futex change. > > -- Steve > > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h > index c2c9ba0..25f1e9e 100644 > --- a/include/asm-generic/bug.h > +++ b/include/asm-generic/bug.h > @@ -168,7 +168,7 @@ extern void warn_slowpath_null(const char *file, const int line); > #ifdef CONFIG_SMP > # define WARN_ON_SMP(x) WARN_ON(x) > #else > -# define WARN_ON_SMP(x) do { } while (0) > +# define WARN_ON_SMP(x) ({0;}) > #endif > You meant : # define WARN_ON_SMP(x) ({x;}) or # define WARN_ON_SMP(x) do { } while (x, 0) ? |
From: Steven R. <ro...@go...> - 2011-03-17 18:16:51
|
On Thu, 2011-03-17 at 18:02 +0100, Eric Dumazet wrote: > Le jeudi 17 mars 2011 à 12:55 -0400, Steven Rostedt a écrit : > > Here, test this patch. I'm in the process of committing it now. > > It will be two patches, one for the WARN_ON_SMP() change, the other for > > the futex change. > > > > -- Steve > > > > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h > > index c2c9ba0..25f1e9e 100644 > > --- a/include/asm-generic/bug.h > > +++ b/include/asm-generic/bug.h > > @@ -168,7 +168,7 @@ extern void warn_slowpath_null(const char *file, const int line); > > #ifdef CONFIG_SMP > > # define WARN_ON_SMP(x) WARN_ON(x) > > #else > > -# define WARN_ON_SMP(x) do { } while (0) > > +# define WARN_ON_SMP(x) ({0;}) > > #endif > > > > You meant : > > # define WARN_ON_SMP(x) ({x;}) > > or > > # define WARN_ON_SMP(x) do { } while (x, 0) > > ? > Does if (do { } while (x, 0)) work? And no, on SMP it should always return false. The point is, the warning is only valid if we are on an SMP box, this is useful for spin_locks() if (WARN_ON_SMP(!spin_is_locked(lock))) fail(); We don't want to fail. That spin_is_locked(lock) on !SMP returns 0 every time. -- Steve |
From: Steven R. <ro...@go...> - 2011-03-17 18:22:09
|
On Thu, 2011-03-17 at 18:43 +0100, Geert Uytterhoeven wrote: > I don't know. It's not clear to me if "WARN_ON_SMP(x)" should always return "x", > or only on SMP? > > E.g. > > if (WARN_ON_SMP(x)) { > // Should we get here if "x" is true? > // Or only if CONFIG_SMP and "x" are both true? > } > The "Or only if CONFIG_SMP and "x" are both true. I'll update the change to add a comment about that. -- Steve |
From: Steven R. <ro...@go...> - 2011-03-17 18:33:02
|
On Thu, 2011-03-17 at 14:22 -0400, Steven Rostedt wrote: > On Thu, 2011-03-17 at 18:43 +0100, Geert Uytterhoeven wrote: > > > I don't know. It's not clear to me if "WARN_ON_SMP(x)" should always return "x", > > or only on SMP? > > > > E.g. > > > > if (WARN_ON_SMP(x)) { > > // Should we get here if "x" is true? > > // Or only if CONFIG_SMP and "x" are both true? > > } > > > > The "Or only if CONFIG_SMP and "x" are both true. > > I'll update the change to add a comment about that. > We are the first user of this false return, as do { } while (0) doesn't return anything. I'll add a comment to explain the issue, but it makes sense to have it returns false on UNIPROCESSOR. -- Steve |
From: Eric D. <eri...@gm...> - 2011-03-17 18:39:15
|
Le jeudi 17 mars 2011 à 19:25 +0100, Geert Uytterhoeven a écrit : > On Thu, Mar 17, 2011 at 19:16, Steven Rostedt <ro...@go...> wrote: > > On Thu, 2011-03-17 at 18:02 +0100, Eric Dumazet wrote: > >> Le jeudi 17 mars 2011 à 12:55 -0400, Steven Rostedt a écrit : > >> > Here, test this patch. I'm in the process of committing it now. > >> > It will be two patches, one for the WARN_ON_SMP() change, the other for > >> > the futex change. > >> > > >> > -- Steve > >> > > >> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h > >> > index c2c9ba0..25f1e9e 100644 > >> > --- a/include/asm-generic/bug.h > >> > +++ b/include/asm-generic/bug.h > >> > @@ -168,7 +168,7 @@ extern void warn_slowpath_null(const char *file, const int line); > >> > #ifdef CONFIG_SMP > >> > # define WARN_ON_SMP(x) WARN_ON(x) > >> > #else > >> > -# define WARN_ON_SMP(x) do { } while (0) > >> > +# define WARN_ON_SMP(x) ({0;}) > >> > #endif > >> > > >> > >> You meant : > >> > >> # define WARN_ON_SMP(x) ({x;}) > >> > >> or > >> > >> # define WARN_ON_SMP(x) do { } while (x, 0) > >> > >> ? > >> > > > > Does if (do { } while (x, 0)) work? > My point was that WARN_ON(X) always evaluates X once And apparently, WARN_ON_SMP(X) doesnt evaluates X iF !SMP This should be documented, or fixed ;) |
From: Steven R. <ro...@go...> - 2011-03-17 18:43:45
|
On Thu, 2011-03-17 at 19:38 +0100, Eric Dumazet wrote: > > > My point was that WARN_ON(X) always evaluates X once > > And apparently, WARN_ON_SMP(X) doesnt evaluates X iF !SMP > > This should be documented, or fixed ;) My new patch has it documented. I even explain when to use the _SMP() version, which is mainly for !spin_is_locked() as spin_is_locked() always returns false, and !0 will trigger the warning. It can also be used to test values that only exist in SMP. struct foo { [...] #ifdef CONFIG_SMP int bar; #endif }; WARN_ON_SMP(zoo->bar); We don't want that zoo->bar even evaluated for that case. -- Steve |