futex: Fix WARN_ON triggering on UP

GIT PULL Superseded Steven Rostedt 7 participants 19 messages Mar 17, 2011 – Mar 25, 2011 Generic include/asm header files

lore

Share & tools

lore

Steven Rostedt

author

2011-03-17 19:22 UTC #0 lore raw

Thomas,

Please pull the rt patches from:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-rt.git

branch: tip/futex/devel

Steven Rostedt (2):
WARN_ON_SMP(): Allow use in if statements on UP
futex: Fix WARN_ON() test for UP

----

 include/asm-generic/bug.h |   28 +++++++++++++++++++++++++++-
 kernel/futex.c            |    4 ++--
 2 files changed, 29 insertions(+), 3 deletions(-)

Steven Rostedt

author

2011-03-17 19:22 UTC #1 lore raw

PATCH 1/2 WARN_ON_SMP(): Allow use in if statements on UP

From: Steven Rostedt <[email protected]>

Both WARN_ON() and WARN_ON_SMP() should be able to be used in an if statement.

	if (WARN_ON_SMP(foo)) { ... }

Because WARN_ON_SMP() is defined as a do { } while (0) on UP, it can not be used this way.

Convert it to the same form that WARN_ON() is, even when CONFIG_SMP is off.

Signed-off-by Steven Rostedt <[email protected]>

---
 include/asm-generic/bug.h |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

include/asm-generic/bug.h +27 -1

index c2c9ba0..ac2f48a 100644
@@ -165,10 +165,36 @@ extern void warn_slowpath_null(const char *file, const int line);
 #define WARN_ON_RATELIMIT(condition, state)			\
 		WARN_ON((condition) && __ratelimit(state))
 
+/*
+ * WARN_ON_SMP() is for cases that the warning is either
+ * meaningless for !SMP or may even cause failures.
+ * This is usually used for cases that we have
+ * WARN_ON(!spin_is_locked(&lock)) checks, as spin_is_locked()
+ * returns 0 for uniprocessor settings.
+ * It can be also be used with values that are only defined
+ * on SMP:
+ *
+ * struct foo {
+ *  [...]
+ * #ifdef CONFIG_SMP
+ *	int bar;
+ * #endif
+ * };
+ *
+ * void func(struct foo *zoot)
+ * {
+ *	WARN_ON_SMP(!zoot->bar);
+ *
+ * For CONFIG_SMP, WARN_ON_SMP() should act the same as WARN_ON(),
+ * and should be a nop and return false for uniprocessor.
+ *
+ * if (WARN_ON_SMP(x)) returns true only when CONFIG_SMP is set
+ * and x is true.
+ */
 #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

Signature · 1 line

1.7.2.3

Darren Hart

2011-03-17 20:46 UTC #2 lore raw

Re: [PATCH 1/2] WARN_ON_SMP(): Allow use in if statements on UP

Quoted 23 lines · Steven Rostedt

From: Steven Rostedt<[email protected]>
Both WARN_ON() and WARN_ON_SMP() should be able to be used in an if statement.
	if (WARN_ON_SMP(foo)) { ... }
Because WARN_ON_SMP() is defined as a do { } while (0) on UP, it can not be used this way.
Convert it to the same form that WARN_ON() is, even when CONFIG_SMP is off.
Signed-off-by Steven Rostedt<[email protected]>
---
  include/asm-generic/bug.h |   28 +++++++++++++++++++++++++++-
  1 files changed, 27 insertions(+), 1 deletions(-)
include/asm-generic/bug.h +0 -0
index c2c9ba0..ac2f48a 100644

include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 174

Quoted 10 lines

include/asm-generic/bug.h +7 -0

@@ -165,10 +165,36 @@ extern void warn_slowpath_null(const char *file, const int line);
  #define WARN_ON_RATELIMIT(condition, state)			\
  		WARN_ON((condition)&&  __ratelimit(state))
 
+/*
+ * WARN_ON_SMP() is for cases that the warning is either
+ * meaningless for !SMP or may even cause failures.
+ * This is usually used for cases that we have
+ * WARN_ON(!spin_is_locked(&lock)) checks, as spin_is_locked()
+ * returns 0 for uniprocessor settings.
+ * It can be also be used with values that are only defined

Typo: can be also be

--
Darren

Quoted 28 lines

include/asm-generic/bug.h +20 -2

+ * on SMP:
+ *
+ * struct foo {
+ *  [...]
+ * #ifdef CONFIG_SMP
+ *	int bar;
+ * #endif
+ * };
+ *
+ * void func(struct foo *zoot)
+ * {
+ *	WARN_ON_SMP(!zoot->bar);
+ *
+ * For CONFIG_SMP, WARN_ON_SMP() should act the same as WARN_ON(),
+ * and should be a nop and return false for uniprocessor.
+ *
+ * if (WARN_ON_SMP(x)) returns true only when CONFIG_SMP is set
+ * and x is true.
+ */
  #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
-- 1.7.2.3

Signature · 3 lines

Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Peter Zijlstra

Acked-by on v1 1/2

2011-03-25 09:33 UTC #3 lore raw

Re: [PATCH 1/2] WARN_ON_SMP(): Allow use in if statements on UP

In reply to Steven Rostedt

Quoted 7 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>
Both WARN_ON() and WARN_ON_SMP() should be able to be used in an if statement.
        if (WARN_ON_SMP(foo)) { ... }

Quoted 7 lines

Because WARN_ON_SMP() is defined as a do { } while (0) on UP, it can not be used this way.
Convert it to the same form that WARN_ON() is, even when CONFIG_SMP is off.
Signed-off-by Steven Rostedt <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

tip-bot for Steven Rostedt

2011-03-25 10:49 UTC #4 lore raw

[tip:core/urgent] WARN_ON_SMP(): Allow use in if() statements on UP

In reply to Steven Rostedt

Commit-ID:  2092e6be82ec71ecbf5a8ceeef004bbcbdb78812
Gitweb:     http://git.kernel.org/tip/2092e6be82ec71ecbf5a8ceeef004bbcbdb78812
Author:     Steven Rostedt <[email protected]>
AuthorDate: Thu, 17 Mar 2011 15:21:06 -0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 25 Mar 2011 11:32:09 +0100

WARN_ON_SMP(): Allow use in if() statements on UP

Both WARN_ON() and WARN_ON_SMP() should be able to be used in an if statement.

	if (WARN_ON_SMP(foo)) { ... }

Because WARN_ON_SMP() is defined as a do { } while (0) on UP, it can not be used this way.

Convert it to the same form that WARN_ON() is, even when CONFIG_SMP is off.

Signed-off-by Steven Rostedt <[email protected]>

Acked-by Thomas Gleixner <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

Acked-by Darren Hart <[email protected]>

Cc Lai Jiangshan <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Andrew Morton <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 include/asm-generic/bug.h |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

include/asm-generic/bug.h +27 -1

index c2c9ba0..f2d2faf 100644
@@ -165,10 +165,36 @@ extern void warn_slowpath_null(const char *file, const int line);
 #define WARN_ON_RATELIMIT(condition, state)			\
 		WARN_ON((condition) && __ratelimit(state))
 
+/*
+ * WARN_ON_SMP() is for cases that the warning is either
+ * meaningless for !SMP or may even cause failures.
+ * This is usually used for cases that we have
+ * WARN_ON(!spin_is_locked(&lock)) checks, as spin_is_locked()
+ * returns 0 for uniprocessor settings.
+ * It can also be used with values that are only defined
+ * on SMP:
+ *
+ * struct foo {
+ *  [...]
+ * #ifdef CONFIG_SMP
+ *	int bar;
+ * #endif
+ * };
+ *
+ * void func(struct foo *zoot)
+ * {
+ *	WARN_ON_SMP(!zoot->bar);
+ *
+ * For CONFIG_SMP, WARN_ON_SMP() should act the same as WARN_ON(),
+ * and should be a nop and return false for uniprocessor.
+ *
+ * if (WARN_ON_SMP(x)) returns true only when CONFIG_SMP is set
+ * and x is true.
+ */
 #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

Linus Torvalds

2011-03-25 16:54 UTC #5 lore raw

Re: [tip:core/urgent] WARN_ON_SMP(): Allow use in if() statements on UP

include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 197

Quoted 2 lines · tip-bot for Steven Rostedt

include/asm-generic/bug.h +1 -1

-# define WARN_ON_SMP(x)                        do { } while (0)
+# define WARN_ON_SMP(x)                        ({0;})

That's a VERY odd way of writing "0".

Am I missing something subtle?

Linus

Steven Rostedt

author

2011-03-25 18:16 UTC #6 lore raw

Re: [tip:core/urgent] WARN_ON_SMP(): Allow use in if() statements on UP

Quoted 8 lines · Linus Torvalds

include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 197
Quoted 2 lines · tip-bot for Steven Rostedt
include/asm-generic/bug.h +1 -1
-# define WARN_ON_SMP(x)                        do { } while (0)
+# define WARN_ON_SMP(x)                        ({0;})
That's a VERY odd way of writing "0".
Am I missing something subtle?

I thought about using "0", but when WARN_ON_SMP() is used outside of an if statement, it turns into:

0;

Which seems strange to me. Thus the ({0;}) was basically a way to state that this is also a function and not just a 0 value.

Also, a quick test shows that

0;

gives the warning:

"warning: statement with no effect"

-- Steve

Alexey Dobriyan

2011-03-25 19:31 UTC #8 lore raw

Re: [tip:core/urgent] WARN_ON_SMP(): Allow use in if() statements on UP

Quoted 8 lines · Steven Rostedt

Quoted 6 lines · Steven Rostedt
Also, a quick test shows that
	0;
gives the warning:

Quoted 8 lines

Quoted 1 line
"warning: statement with no effect"
Would you like me to add a comment that states:
	/*
	 * Use ({0;}) as just "0" will cause gcc to output:
	 *   warning: statement with no effect
	 */

Try ((void)0)

Steven Rostedt

2011-03-25 19:38 UTC #9 lore raw

Re: [tip:core/urgent] WARN_ON_SMP(): Allow use in if() statements on UP

Quoted 1 line · Alexey Dobriyan

Try ((void)0)

But then if ((void)0) wont work.

-- Steve

Steven Rostedt

author

2011-03-25 19:40 UTC #10 lore raw

Re: [tip:core/urgent] WARN_ON_SMP(): Allow use in if() statements on UP

In reply to Alexey Dobriyan

Quoted 8 lines · Alexey Dobriyan

Quoted 6 lines
Would you like me to add a comment that states:
	/*
	 * Use ({0;}) as just "0" will cause gcc to output:
	 *   warning: statement with no effect
	 */
Try ((void)0)

Maybe I should update the comment to stop further confusion:

/*
* Use ({0;}) as it works nicely in both an if statement and
* as a stand alone line statement.
* "0;" causes "warning: statement with no effect"
* ((void)0) does not work in an if () condition.
*/

-- Steve

Alexey Dobriyan

2011-03-25 19:47 UTC #11 lore raw

Re: [tip:core/urgent] WARN_ON_SMP(): Allow use in if() statements on UP

Quoted 4 lines · Steven Rostedt

Quoted 2 lines · Alexey Dobriyan
Quoted 2 lines
Would you like me to add a comment that states:

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
	/*
	 * Use ({0;}) as just "0" will cause gcc to output:
	 *   warning: statement with no effect
	 */
Try ((void)0)
Maybe I should update the comment to stop further confusion:

Ah!

I always thought if (WARN_ON(x)) is confusing and should be banned.

Ingo Molnar

2011-03-25 19:57 UTC #13 lore raw

Re: [tip:core/urgent] WARN_ON_SMP(): Allow use in if() statements on UP

In reply to Alexey Dobriyan

Quoted 10 lines · Alexey Dobriyan

Quoted 9 lines · Steven Rostedt
Quoted 7 lines · Alexey Dobriyan
Quoted 6 lines
Would you like me to add a comment that states:
	/*
	 * Use ({0;}) as just "0" will cause gcc to output:
	 *   warning: statement with no effect
	 */

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Try ((void)0)
Maybe I should update the comment to stop further confusion:
Ah!
I always thought if (WARN_ON(x)) is confusing and should be banned.

I'd encourage you to read kernel/lockdep.c one day:

		return DEBUG_LOCKS_WARN_ON(1);
		DEBUG_LOCKS_WARN_ON(1);
	if (DEBUG_LOCKS_WARN_ON(class->subclass != subclass))
	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
		if (DEBUG_LOCKS_WARN_ON(id >= MAX_LOCKDEP_KEYS))
	if (DEBUG_LOCKS_WARN_ON(unlikely(early_boot_irqs_disabled)))
	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
	if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
		DEBUG_LOCKS_WARN_ON(!softirq_count());
	if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)))
	if (DEBUG_LOCKS_WARN_ON(!name)) {
	if (DEBUG_LOCKS_WARN_ON(!key))
		DEBUG_LOCKS_WARN_ON(1);
	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
	if (DEBUG_LOCKS_WARN_ON(depth >= MAX_LOCK_DEPTH))
	if (DEBUG_LOCKS_WARN_ON(!class))
	if (DEBUG_LOCKS_WARN_ON(id >= MAX_LOCKDEP_KEYS))
		if (DEBUG_LOCKS_WARN_ON(chain_key != 0))
	if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
		if (DEBUG_LOCKS_WARN_ON(!class))
		if (DEBUG_LOCKS_WARN_ON(!hlock->nest_lock))
	if (DEBUG_LOCKS_WARN_ON(!depth))
	if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth))
	if (DEBUG_LOCKS_WARN_ON(!depth))
	if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth - 1))
	if (DEBUG_LOCKS_WARN_ON(!depth && (hlock->prev_chain_key != 0)))
		if (DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)) {
		if (DEBUG_LOCKS_WARN_ON(!current->hardirqs_enabled)) {
			DEBUG_LOCKS_WARN_ON(current->softirqs_enabled);
			DEBUG_LOCKS_WARN_ON(!current->softirqs_enabled);
	if (DEBUG_LOCKS_WARN_ON(!depth))
	if (DEBUG_LOCKS_WARN_ON(!depth))

These conditional warnings made the flow a lot clearer and simpler - while still giving us a very robust lockdep machinery that wont crash no matter what kind of anomaly happens.

But yes, i can imagine such constructs being misused as well when mixed into real, non-debug functionality. As usual, it's just a tool, with good and evil uses as well - so your 'it should be banned' position is too extreme IMHO.

Thanks,

Ingo

Steven Rostedt

author

2011-03-17 19:22 UTC #14 lore raw

PATCH 2/2 futex: Fix WARN_ON() test for UP

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

An update of the futex code had a

	WARN_ON(!spin_is_locked(q->lock_ptr))

But on UP, spin_is_locked() is always false, and will trigger this warning, and even worse, it will exit the function without doing the necessary work.

Converting this to a WARN_ON_SMP() fixes the problem.

Reported-by Richard Weinberger <[email protected]>

Tested-by Richard Weinberger <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/futex.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

kernel/futex.c +2 -2

index 9fe9131..850d00b 100644
@@ -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);

Signature · 1 line

1.7.2.3

Darren Hart

Acked-by on v1 2/2

2011-03-17 20:00 UTC #15 lore raw

Re: [PATCH 2/2] futex: Fix WARN_ON() test for UP

Quoted 10 lines · Steven Rostedt

From: Steven Rostedt<[email protected]>
An update of the futex code had a
	WARN_ON(!spin_is_locked(q->lock_ptr))
But on UP, spin_is_locked() is always false, and will trigger this warning, and even worse, it will exit the function without doing the necessary work.

Quoted 5 lines

Converting this to a WARN_ON_SMP() fixes the problem.
Reported-by Richard Weinberger<[email protected]>
Tested-by Richard Weinberger<[email protected]>
Signed-off-by Steven Rostedt<[email protected]>

Acked-by Darren Hart <[email protected]>

Quoted 23 lines

---
  kernel/futex.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

kernel/futex.c +2 -3

index 9fe9131..850d00b 100644
@@ -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);
-- 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe

linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/

Signature · 3 lines

Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Peter Zijlstra

Acked-by on v1 2/2

2011-03-25 09:33 UTC #16 lore raw

Re: [PATCH 2/2] futex: Fix WARN_ON() test for UP

In reply to Steven Rostedt

Quoted 10 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>
An update of the futex code had a
        WARN_ON(!spin_is_locked(q->lock_ptr))
But on UP, spin_is_locked() is always false, and will trigger this warning, and even worse, it will exit the function without doing the necessary work.

Quoted 5 lines

Converting this to a WARN_ON_SMP() fixes the problem.
Reported-by Richard Weinberger <[email protected]>
Tested-by Richard Weinberger <[email protected]>
Signed-off-by Steven Rostedt <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

tip-bot for Steven Rostedt

2011-03-25 10:49 UTC #17 lore raw

[tip:core/urgent] futex: Fix WARN_ON() test for UP

In reply to Steven Rostedt

Commit-ID:  29096202176ceaa5016a17ea2dd1aea19a4e90e2
Gitweb:     http://git.kernel.org/tip/29096202176ceaa5016a17ea2dd1aea19a4e90e2
Author:     Steven Rostedt <[email protected]>
AuthorDate: Thu, 17 Mar 2011 15:21:07 -0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 25 Mar 2011 11:32:11 +0100

futex: Fix WARN_ON() test for UP

An update of the futex code had a

	WARN_ON(!spin_is_locked(q->lock_ptr))

But on UP, spin_is_locked() is always false, and will trigger this warning, and even worse, it will exit the function without doing the necessary work.

Converting this to a WARN_ON_SMP() fixes the problem.

Reported-by Richard Weinberger <[email protected]>

Tested-by Richard Weinberger <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

Acked-by Thomas Gleixner <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

Acked-by Darren Hart <[email protected]>

Cc Lai Jiangshan <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/futex.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

kernel/futex.c +2 -2

index bda4157..823aae3 100644
@@ -782,8 +782,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);

Steven Rostedt

author

2011-03-17 19:28 UTC #18 lore raw

In reply to Steven Rostedt

Quoted 3 lines · Steven Rostedt

Thomas,
Please pull the rt patches from:

Oops, sorry this is for mainline not -rt, I used the wrong boiler plate.

-- Steve

Quoted 14 lines

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-rt.git
branch: tip/futex/devel
Steven Rostedt (2):
WARN_ON_SMP(): Allow use in if statements on UP
futex: Fix WARN_ON() test for UP
----
 include/asm-generic/bug.h |   28 +++++++++++++++++++++++++++-
 kernel/futex.c            |    4 ++--
 2 files changed, 29 insertions(+), 3 deletions(-)