Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

[H. Peter Anvin]

On 03/28/2011 07:45 AM, tip-bot for Steven Rostedt wrote:
> 
> WARN_ON_SMP(): Add comment to explain ({0;})
> 
> The define to use ({0;}) for the !CONFIG_SMP case of WARN_ON_SMP()
> can be confusing. As the WARN_ON_SMP() needs to be a nop when
> CONFIG_SMP is not set, including all its parameters must not be
> evaluated, and that it must work as both a stand alone statement
> and inside an if condition, we define it to a funky ({0;}).
> 
> A simple "0" will not work as it causes gcc to give the warning that
> the statement has no effect.
> 
> As this strange definition has raised a few eyebrows from some
> major kernel developers, it is wise to document why we create such
> a work of art.
> 

What the heck is wrong with the idiomatic and non-gcc-extension-using:

	((void)0)

?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

[richard -rw- weinberger]

On Mon, Mar 28, 2011 at 4:51 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 03/28/2011 07:45 AM, tip-bot for Steven Rostedt wrote:
>>
>> WARN_ON_SMP(): Add comment to explain ({0;})
>>
>> The define to use ({0;}) for the !CONFIG_SMP case of WARN_ON_SMP()
>> can be confusing. As the WARN_ON_SMP() needs to be a nop when
>> CONFIG_SMP is not set, including all its parameters must not be
>> evaluated, and that it must work as both a stand alone statement
>> and inside an if condition, we define it to a funky ({0;}).
>>
>> A simple "0" will not work as it causes gcc to give the warning that
>> the statement has no effect.
>>
>> As this strange definition has raised a few eyebrows from some
>> major kernel developers, it is wise to document why we create such
>> a work of art.
>>
>
> What the heck is wrong with the idiomatic and non-gcc-extension-using:
>
>        ((void)0)
>
> ?

AFAIK you cannot use it within an if-statement.

>        -hpa
>
> --
> H. Peter Anvin, Intel Open Source Technology Center
> I work for Intel.  I don't speak on their behalf.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

-- 
Thanks,
//richard

Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

[H. Peter Anvin]

On 03/28/2011 07:56 AM, richard -rw- weinberger wrote:
>>
>> What the heck is wrong with the idiomatic and non-gcc-extension-using:
>>
>>        ((void)0)
>>
>> ?
> 
> AFAIK you cannot use it within an if-statement.
> 

OK, fair enough.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

[Steven Rostedt]

On Mon, 2011-03-28 at 07:58 -0700, H. Peter Anvin wrote:
> On 03/28/2011 07:56 AM, richard -rw- weinberger wrote:
> >>
> >> What the heck is wrong with the idiomatic and non-gcc-extension-using:
> >>
> >>        ((void)0)
> >>
> >> ?
> > 
> > AFAIK you cannot use it within an if-statement.
> > 
> 
> OK, fair enough.

If people hate the ({0;}) so much, we could replace it with:

static inline int _WARN_ON_SMP(void) { return 0; }
#define WARN_ON_SMP(x) _WARN_ON_SMP()

That would pretty much do the same thing.

 o Keeps the parameters from being evaluated, as they may not be defined
   for SMP

 o Can be used as a standalone statement without gcc complaining

 o Can be used within an if condition.

Geeze, I never expected such a fuss over a simple change ;)


-- Steve

Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

[Alexey Dobriyan]

On Mon, Mar 28, 2011 at 11:09:00AM -0400, Steven Rostedt wrote:
> Geeze, I never expected such a fuss over a simple change ;)

And it's still broken ;-)

	[spoiler space]































WARN_ON_SMP(x) should expand to x, not 0.

Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

[Ingo Molnar]

* Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Mon, Mar 28, 2011 at 11:09:00AM -0400, Steven Rostedt wrote:
> > Geeze, I never expected such a fuss over a simple change ;)
> 
> And it's still broken ;-)
> 
> WARN_ON_SMP(x) should expand to x, not 0.

Dunno, we really do not want to *guarantee* side-effects in WARN_ON() 
constructs.

Thanks,

	Ingo

Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

[Steven Rostedt]

On Tue, 2011-03-29 at 07:32 -0700, H. Peter Anvin wrote:
> I think the question is what are the expected semantics if WARN_ON_SMP
> is used in an if... especially for UP.

Well, actually this whole patchset was created because we added the
semantic if (WARN_ON_SMP(x)).

This patch: https://lkml.org/lkml/2011/3/11/463


Added the following code:


+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)))
+		return;
+
+	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
+	plist_del(&q->list, &hb->chain);
+}


Which on !CONFIG_SMP broke, because that "|| !spin_is_locked()" always
return true and we trigger the WARN_ON() as well as returning early,
breaking the code, and triggering bugs, as we never do the
"plist_del()".

To fix this, I added: https://lkml.org/lkml/2011/3/17/312

Which does this change:

@@ -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);


But as WARN_ON_SMP() currently had no if () context, I needed to first
make the change of: https://lkml.org/lkml/2011/3/17/313

#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

But if this is such a problem, we can revert all this and do:

@@ -785,8 +785,11 @@ 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)))
+#ifdef CONFIG_SMP
+	if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr))
+		return;
+#endif
+	if (WARN_ON(plist_node_empty(&q->list)))
 		return;
 
 	hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);


And leave the WARN_ON_SMP() alone.

-- Steve

Re: [tip:core/urgent] WARN_ON_SMP(): Add comment to explain ({0;})

[Steven Rostedt]

On Mon, 2011-03-28 at 21:18 +0300, Alexey Dobriyan wrote:
> On Mon, Mar 28, 2011 at 11:09:00AM -0400, Steven Rostedt wrote:
> > Geeze, I never expected such a fuss over a simple change ;)
> 
> And it's still broken ;-)
> 
> 	[spoiler space]

I didn't understand this "space" and never went down to look more.
Should have said "see below".

> WARN_ON_SMP(x) should expand to x, not 0.

No it should expand to zero, please READ THE COMMENT!

OK, it was not part of this patch, but it was in the patch that this
patch was added for.

+/*
+ * 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.
+ */

-- Steve