um: WARNING: at kernel/futex.c:786 __unqueue_futex

um: WARNING: at kernel/futex.c:786 __unqueue_futex

[richard -rw- weinberger]

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

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Steven Rostedt]

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?
> 

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[richard -rw- weinberger]

On Thu, Mar 17, 2011 at 3:08 PM, Steven Rostedt <rostedt@goodmis.org> 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
--
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/

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[richard -rw- weinberger]

On Thu, Mar 17, 2011 at 3:08 PM, Steven Rostedt <rostedt@goodmis.org> 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
--
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/

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Steven Rostedt]

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

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Steven Rostedt]

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

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Eric Dumazet]

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)

?


--
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/

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[richard -rw- weinberger]

On Thu, Mar 17, 2011 at 5:55 PM, Steven Rostedt <rostedt@goodmis.org> 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 <richard@nod.at>

-- 
Thanks,
//richard
--
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/

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Geert Uytterhoeven]

On Thu, Mar 17, 2011 at 18:02, Eric Dumazet <eric.dumazet@gmail.com> 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 -- geert@linux-m68k.org

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

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Steven Rostedt]

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


--
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/

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Steven Rostedt]

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

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Geert Uytterhoeven]

On Thu, Mar 17, 2011 at 19:16, Steven Rostedt <rostedt@goodmis.org> 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 -- geert@linux-m68k.org

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

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Steven Rostedt]

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

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Steven Rostedt]

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

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Eric Dumazet]

Le jeudi 17 mars 2011 à 19:25 +0100, Geert Uytterhoeven a écrit :
> On Thu, Mar 17, 2011 at 19:16, Steven Rostedt <rostedt@goodmis.org> 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 ;)



--
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/

Re: um: WARNING: at kernel/futex.c:786 __unqueue_futex

[Steven Rostedt]

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