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(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index c2c9ba0..ac2f48a 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -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
--
1.7.2.3
On 03/17/2011 12:21 PM, Steven Rostedt wrote:
> 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(-)
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index c2c9ba0..ac2f48a 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -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
> + * 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
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
On Thu, 2011-03-17 at 15:21 -0400, Steven Rostedt wrote:
> 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]>
Acked-by: Peter Zijlstra <[email protected]>
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(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index c2c9ba0..f2d2faf 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -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
On Fri, Mar 25, 2011 at 3:48 AM, tip-bot for Steven Rostedt
<[email protected]> wrote:
> -# 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
On Fri, 2011-03-25 at 09:45 -0700, Linus Torvalds wrote:
> On Fri, Mar 25, 2011 at 3:48 AM, tip-bot for Steven Rostedt
> <[email protected]> wrote:
> > -# 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
On Fri, 2011-03-25 at 14:16 -0400, Steven Rostedt wrote:
> Also, a quick test shows that
>
> 0;
>
> gives the warning:
>
> "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
*/
-- Steve
On Fri, Mar 25, 2011 at 02:19:23PM -0400, Steven Rostedt wrote:
> On Fri, 2011-03-25 at 14:16 -0400, Steven Rostedt wrote:
>
> > Also, a quick test shows that
> >
> > 0;
> >
> > gives the warning:
> >
> > "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)
On Fri, 2011-03-25 at 21:31 +0200, Alexey Dobriyan wrote:
> Try ((void)0)
But then if ((void)0) wont work.
-- Steve
On Fri, 2011-03-25 at 21:31 +0200, Alexey Dobriyan wrote:
> > 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
On Fri, Mar 25, 2011 at 03:40:34PM -0400, Steven Rostedt wrote:
> On Fri, 2011-03-25 at 21:31 +0200, Alexey Dobriyan wrote:
>
> > > 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:
Ah!
I always thought if (WARN_ON(x)) is confusing and should be banned.
On Fri, 2011-03-25 at 21:47 +0200, Alexey Dobriyan wrote:
> I always thought if (WARN_ON(x)) is confusing and should be banned.
I think that's the preferred method (I use it all the time).
if (WARN_ON(blah))
goto fail;
Better than:
if (x) {
WARN_ON(1);
goto fail;
}
-- Steve
* Alexey Dobriyan <[email protected]> wrote:
> On Fri, Mar 25, 2011 at 03:40:34PM -0400, Steven Rostedt wrote:
> > On Fri, 2011-03-25 at 21:31 +0200, Alexey Dobriyan wrote:
> >
> > > > 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:
>
> 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