2.6.15-git7 oopses in ext3 during LTP runs

2.6.15-git7 oopses in ext3 during LTP runs

[Andi Kleen]

Running LTP with the default runfile on a 4 virtual CPU x86-64 
system gives

To reproduce: run ltp 20040908 (newer one will probably work
too) with runltp -p -q -l `uname -r` on a ext3 file system

config is x86-64 defconfig.

-Andi

----------- [cut here ] --------- [please bite here ] ---------
Kernel BUG at /home/lsrc/quilt/linux/fs/ext3/super.c:2154
invalid opcode: 0000 [1] SMP 
CPU 0 
Modules linked in:
Pid: 14055, comm: pdflush Not tainted 2.6.15-git7 #90
RIP: 0010:[<ffffffff801d998e>] <ffffffff801d998e>{ext3_write_super+20}
RSP: 0018:ffff810117fb1e08  EFLAGS: 00010202
RAX: 0000000000000001 RBX: ffff81011f9ae800 RCX: ffffffff80492060
RDX: 0000000000000000 RSI: ffff810117fb0000 RDI: ffff81011f9ae888
RBP: ffff81011f9ae888 R08: ffff810117fb0000 R09: ffff810004f219e0
R10: 0000000000000000 R11: 0000000000000002 R12: ffff81011f9ae870
R13: ffffffff80159aee R14: ffff8100cdce1d68 R15: ffffffff80146b14
FS:  0000000000000000(0000) GS:ffffffff8060d000(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 00002aaaaab61000 CR3: 000000011baf3000 CR4: 00000000000006e0
Process pdflush (pid: 14055, threadinfo ffff810117fb0000, task ffff81011fe5c400)
Stack: ffff81011f9ae800 ffffffff8017aa43 0000000000000064 0000000000000000 
       ffff8100cdce1d78 ffffffff801592f9 ffff81011fe5c400 ffffffff80494000 
       0000000000000000 0000000000000000 
Call Trace: <ffffffff8017aa43>{sync_supers+133} <ffffffff801592f9>{wb_kupdate+49}
       <ffffffff80159aee>{pdflush+0} <ffffffff80159c40>{pdflush+338}
       <ffffffff801592c8>{wb_kupdate+0} <ffffffff80146c7f>{kthread+203}
       <ffffffff801107ca>{child_rip+8} <ffffffff80146b14>{keventd_create_kthread+0}
       <ffffffff80146bb4>{kthread+0} <ffffffff801107c2>{child_rip+0}

Code: 0f 0b 68 e9 0d 43 80 c2 6a 08 c6 43 21 00 5b c3 49 c7 c0 ca 
RIP <ffffffff801d998e>{ext3_write_super+20} RSP <ffff810117fb1e08>
 Badness in do_exit at /home/lsrc/quilt/linux/kernel/exit.c:796

Call Trace: <ffffffff80136800>{do_exit+84} <ffffffff804024dd>{_spin_unlock_irqrestore+8}
       <ffffffff80146b14>{keventd_create_kthread+0} <ffffffff801114ef>{kernel_math_error+0}
       <ffffffff80159aee>{pdflush+0} <ffffffff80111c61>{do_invalid_op+163}
       <ffffffff801d998e>{ext3_write_super+20} <ffffffff8040116a>{thread_return+0}
       <ffffffff80110611>{error_exit+0} <ffffffff80146b14>{keventd_create_kthread+0}
       <ffffffff80159aee>{pdflush+0} <ffffffff801d998e>{ext3_write_super+20}
       <ffffffff801d998a>{ext3_write_super+16} <ffffffff8017aa43>{sync_supers+133}
       <ffffffff801592f9>{wb_kupdate+49} <ffffffff80159aee>{pdflush+0}
       <ffffffff80159c40>{pdflush+338} <ffffffff801592c8>{wb_kupdate+0}
       <ffffffff80146c7f>{kthread+203} <ffffffff801107ca>{child_rip+8}
       <ffffffff80146b14>{keventd_create_kthread+0} <ffffffff80146bb4>{kthread+0}
       <ffffffff801107c2>{child_rip+0}

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Andrew Morton]

Andi Kleen <ak@suse.de> wrote:
>
> 
> Running LTP with the default runfile on a 4 virtual CPU x86-64 
> system gives
> 
> To reproduce: run ltp 20040908 (newer one will probably work
> too) with runltp -p -q -l `uname -r` on a ext3 file system
> 
> config is x86-64 defconfig.
> 

mutex_trylock() is returning the wrong value.  fs/super.c:write_super()
clearly took the lock.

Ingo, weren't you hitting this occasionally?

> 
> ----------- [cut here ] --------- [please bite here ] ---------
> Kernel BUG at /home/lsrc/quilt/linux/fs/ext3/super.c:2154
> invalid opcode: 0000 [1] SMP 
> CPU 0 
> Modules linked in:
> Pid: 14055, comm: pdflush Not tainted 2.6.15-git7 #90
> RIP: 0010:[<ffffffff801d998e>] <ffffffff801d998e>{ext3_write_super+20}
> RSP: 0018:ffff810117fb1e08  EFLAGS: 00010202
> RAX: 0000000000000001 RBX: ffff81011f9ae800 RCX: ffffffff80492060
> RDX: 0000000000000000 RSI: ffff810117fb0000 RDI: ffff81011f9ae888
> RBP: ffff81011f9ae888 R08: ffff810117fb0000 R09: ffff810004f219e0
> R10: 0000000000000000 R11: 0000000000000002 R12: ffff81011f9ae870
> R13: ffffffff80159aee R14: ffff8100cdce1d68 R15: ffffffff80146b14
> FS:  0000000000000000(0000) GS:ffffffff8060d000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
> CR2: 00002aaaaab61000 CR3: 000000011baf3000 CR4: 00000000000006e0
> Process pdflush (pid: 14055, threadinfo ffff810117fb0000, task ffff81011fe5c400)
> Stack: ffff81011f9ae800 ffffffff8017aa43 0000000000000064 0000000000000000 
>        ffff8100cdce1d78 ffffffff801592f9 ffff81011fe5c400 ffffffff80494000 
>        0000000000000000 0000000000000000 
> Call Trace: <ffffffff8017aa43>{sync_supers+133} <ffffffff801592f9>{wb_kupdate+49}
>        <ffffffff80159aee>{pdflush+0} <ffffffff80159c40>{pdflush+338}
>        <ffffffff801592c8>{wb_kupdate+0} <ffffffff80146c7f>{kthread+203}
>        <ffffffff801107ca>{child_rip+8} <ffffffff80146b14>{keventd_create_kthread+0}
>        <ffffffff80146bb4>{kthread+0} <ffffffff801107c2>{child_rip+0}
> 
> Code: 0f 0b 68 e9 0d 43 80 c2 6a 08 c6 43 21 00 5b c3 49 c7 c0 ca 
> RIP <ffffffff801d998e>{ext3_write_super+20} RSP <ffff810117fb1e08>
>  Badness in do_exit at /home/lsrc/quilt/linux/kernel/exit.c:796
> 
> Call Trace: <ffffffff80136800>{do_exit+84} <ffffffff804024dd>{_spin_unlock_irqrestore+8}
>        <ffffffff80146b14>{keventd_create_kthread+0} <ffffffff801114ef>{kernel_math_error+0}
>        <ffffffff80159aee>{pdflush+0} <ffffffff80111c61>{do_invalid_op+163}
>        <ffffffff801d998e>{ext3_write_super+20} <ffffffff8040116a>{thread_return+0}
>        <ffffffff80110611>{error_exit+0} <ffffffff80146b14>{keventd_create_kthread+0}
>        <ffffffff80159aee>{pdflush+0} <ffffffff801d998e>{ext3_write_super+20}
>        <ffffffff801d998a>{ext3_write_super+16} <ffffffff8017aa43>{sync_supers+133}
>        <ffffffff801592f9>{wb_kupdate+49} <ffffffff80159aee>{pdflush+0}
>        <ffffffff80159c40>{pdflush+338} <ffffffff801592c8>{wb_kupdate+0}
>        <ffffffff80146c7f>{kthread+203} <ffffffff801107ca>{child_rip+8}
>        <ffffffff80146b14>{keventd_create_kthread+0} <ffffffff80146bb4>{kthread+0}
>        <ffffffff801107c2>{child_rip+0}

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Arjan van de Ven]

On Wed, 2006-01-11 at 12:46 -0800, Andrew Morton wrote:
> Andi Kleen <ak@suse.de> wrote:
> >
> > 
> > Running LTP with the default runfile on a 4 virtual CPU x86-64 
> > system gives
> > 
> > To reproduce: run ltp 20040908 (newer one will probably work
> > too) with runltp -p -q -l `uname -r` on a ext3 file system
> > 
> > config is x86-64 defconfig.
> > 
> 
> mutex_trylock() is returning the wrong value.  fs/super.c:write_super()
> clearly took the lock.


the conversion is buggy.

mutex_trylock has the same convention as spin_try_lock (which is the
opposite of down_trylock). THe conversion forgot to add a !

--- linux-2.6.15/fs/ext3/super.c~	2006-01-11 21:54:13.000000000 +0100
+++ linux-2.6.15/fs/ext3/super.c	2006-01-11 21:54:13.000000000 +0100
@@ -2150,7 +2150,7 @@
 
 static void ext3_write_super (struct super_block * sb)
 {
-	if (mutex_trylock(&sb->s_lock) != 0)
+	if (!mutex_trylock(&sb->s_lock) != 0)
 		BUG();
 	sb->s_dirt = 0;
 }

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Andrew Morton]

Arjan van de Ven <arjan@infradead.org> wrote:
>
> On Wed, 2006-01-11 at 12:46 -0800, Andrew Morton wrote:
> > Andi Kleen <ak@suse.de> wrote:
> > >
> > > 
> > > Running LTP with the default runfile on a 4 virtual CPU x86-64 
> > > system gives
> > > 
> > > To reproduce: run ltp 20040908 (newer one will probably work
> > > too) with runltp -p -q -l `uname -r` on a ext3 file system
> > > 
> > > config is x86-64 defconfig.
> > > 
> > 
> > mutex_trylock() is returning the wrong value.  fs/super.c:write_super()
> > clearly took the lock.
> 
> 
> the conversion is buggy.
> 
> mutex_trylock has the same convention as spin_try_lock (which is the
> opposite of down_trylock). THe conversion forgot to add a !
> 
> --- linux-2.6.15/fs/ext3/super.c~	2006-01-11 21:54:13.000000000 +0100
> +++ linux-2.6.15/fs/ext3/super.c	2006-01-11 21:54:13.000000000 +0100
> @@ -2150,7 +2150,7 @@
>  
>  static void ext3_write_super (struct super_block * sb)
>  {
> -	if (mutex_trylock(&sb->s_lock) != 0)
> +	if (!mutex_trylock(&sb->s_lock) != 0)
>  		BUG();
>  	sb->s_dirt = 0;
>  }

We expect the lock to be held on entry.  Hence we expect mutex_trylock()
to return zero.

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Jan Engelhardt]

>the conversion is buggy.
>
>mutex_trylock has the same convention as spin_try_lock (which is the
>opposite of down_trylock). THe conversion forgot to add a !
>
> static void ext3_write_super (struct super_block * sb)
> {
>-	if (mutex_trylock(&sb->s_lock) != 0)
>+	if (!mutex_trylock(&sb->s_lock) != 0)

How about a nicer...?

        if(mutex_trylock(&sb->s_lock) == 0)



Jan Engelhardt
-- 

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Arjan van de Ven]

> 15/fs/ext3/super.c~	2006-01-11 21:54:13.000000000 +0100
> > +++ linux-2.6.15/fs/ext3/super.c	2006-01-11 21:54:13.000000000 +0100
> > @@ -2150,7 +2150,7 @@
> >  
> >  static void ext3_write_super (struct super_block * sb)
> >  {
> > -	if (mutex_trylock(&sb->s_lock) != 0)
> > +	if (!mutex_trylock(&sb->s_lock) != 0)
> >  		BUG();
> >  	sb->s_dirt = 0;
> >  }
> 
> We expect the lock to be held on entry.  Hence we expect mutex_trylock()
> to return zero.

you are correct, and the x86-64 mutex.h is buggy

--- linux-2.6.15/include/asm-x86_64/mutex.h.org	2006-01-11 22:25:37.000000000 +0100
+++ linux-2.6.15/include/asm-x86_64/mutex.h	2006-01-11 22:25:43.000000000 +0100
@@ -104,7 +104,7 @@
 static inline int
 __mutex_fastpath_trylock(atomic_t *count, int (*fail_fn)(atomic_t *))
 {
-	if (likely(atomic_cmpxchg(count, 1, 0)) == 1)
+	if (likely(atomic_cmpxchg(count, 1, 0) == 1))
 		return 1;
 	else
 		return 0;

changes the asm to be the correct one for me.
This is odd/evil though..

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> > We expect the lock to be held on entry.  Hence we expect mutex_trylock()
> > to return zero.
> 
> you are correct, and the x86-64 mutex.h is buggy

ahh ... indeed! And i386 trylock was buggy too.

	Ingo

----

fix typo in asm-i386/mutex.h:__mutex_fastpath_trylock - noticed by
Arjan van de Ven.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- include/asm-i386/mutex.h.orig
+++ include/asm-i386/mutex.h
@@ -125,7 +125,7 @@ __mutex_fastpath_trylock(atomic_t *count
 	 * the mutex state would be.
 	 */
 #ifdef __HAVE_ARCH_CMPXCHG
-	if (likely(atomic_cmpxchg(count, 1, 0)) == 1)
+	if (likely(atomic_cmpxchg(count, 1, 0) == 1))
 		return 1;
 	return 0;
 #else

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Pavel Machek]

On St 11-01-06 22:27:55, Arjan van de Ven wrote:
> > We expect the lock to be held on entry.  Hence we expect mutex_trylock()
> > to return zero.
> 
> you are correct, and the x86-64 mutex.h is buggy
> 
> --- linux-2.6.15/include/asm-x86_64/mutex.h.org	2006-01-11 22:25:37.000000000 +0100
> +++ linux-2.6.15/include/asm-x86_64/mutex.h	2006-01-11 22:25:43.000000000 +0100
> @@ -104,7 +104,7 @@
>  static inline int
>  __mutex_fastpath_trylock(atomic_t *count, int (*fail_fn)(atomic_t *))
>  {
> -	if (likely(atomic_cmpxchg(count, 1, 0)) == 1)
> +	if (likely(atomic_cmpxchg(count, 1, 0) == 1))
>  		return 1;
>  	else
>  		return 0;
> 
> changes the asm to be the correct one for me.
> This is odd/evil though..

likely is the evil part here. What about this? Should make this bug
impossible to do....

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -59,8 +59,8 @@ extern void __chk_io_ptr(void __iomem *)
  * specific implementations come from the above header files
  */
 
-#define likely(x)	__builtin_expect(!!(x), 1)
-#define unlikely(x)	__builtin_expect(!!(x), 0)
+#define likely(x)	(__builtin_expect(!!(x), 1))
+#define unlikely(x)	(__builtin_expect(!!(x), 0))
 
 /* Optimization barrier */
 #ifndef barrier
diff --git a/kernel/sched.c b/kernel/sched.c
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -367,7 +367,7 @@ repeat_lock_task:
 	local_irq_save(*flags);
 	rq = task_rq(p);
 	spin_lock(&rq->lock);
-	if (unlikely(rq != task_rq(p))) {
+	if unlikely(rq != task_rq(p)) {
 		spin_unlock_irqrestore(&rq->lock, *flags);
 		goto repeat_lock_task;
 	}
@@ -751,7 +751,7 @@ static int recalc_task_prio(task_t *p, u
 	else
 		sleep_time = (unsigned long)__sleep_time;
 
-	if (likely(sleep_time > 0)) {
+	if likely(sleep_time > 0) {
 		/*
 		 * User tasks that sleep a long time are categorised as
 		 * idle and will get just interactive status to stay active &
@@ -876,7 +876,7 @@ static void resched_task(task_t *p)
 
 	assert_spin_locked(&task_rq(p)->lock);
 
-	if (unlikely(test_tsk_thread_flag(p, TIF_NEED_RESCHED)))
+	if unlikely(test_tsk_thread_flag(p, TIF_NEED_RESCHED))
 		return;
 
 	set_tsk_thread_flag(p, TIF_NEED_RESCHED);


-- 
Thanks, Sharp!

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[David S. Miller]

From: Pavel Machek <pavel@suse.cz>
Date: Wed, 11 Jan 2006 23:40:13 +0100

> likely is the evil part here. What about this? Should make this bug
> impossible to do....
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>

This doesn't let you do:

     if (likely(y) || unlikely(x))

which I have had reason to use in the past.

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Jesper Juhl]

On 1/11/06, Pavel Machek <pavel@suse.cz> wrote:
> On St 11-01-06 22:27:55, Arjan van de Ven wrote:
> > > We expect the lock to be held on entry.  Hence we expect mutex_trylock()
> > > to return zero.
> >
> > you are correct, and the x86-64 mutex.h is buggy
> >
> > --- linux-2.6.15/include/asm-x86_64/mutex.h.org       2006-01-11 22:25:37.000000000 +0100
> > +++ linux-2.6.15/include/asm-x86_64/mutex.h   2006-01-11 22:25:43.000000000 +0100
> > @@ -104,7 +104,7 @@
> >  static inline int
> >  __mutex_fastpath_trylock(atomic_t *count, int (*fail_fn)(atomic_t *))
> >  {
> > -     if (likely(atomic_cmpxchg(count, 1, 0)) == 1)
> > +     if (likely(atomic_cmpxchg(count, 1, 0) == 1))
> >               return 1;
> >       else
> >               return 0;
> >
> > changes the asm to be the correct one for me.
> > This is odd/evil though..
>
> likely is the evil part here. What about this? Should make this bug
> impossible to do....
>
> Signed-off-by: Pavel Machek <pavel@suse.cz>
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -59,8 +59,8 @@ extern void __chk_io_ptr(void __iomem *)
>   * specific implementations come from the above header files
>   */
>
> -#define likely(x)      __builtin_expect(!!(x), 1)
> -#define unlikely(x)    __builtin_expect(!!(x), 0)
> +#define likely(x)      (__builtin_expect(!!(x), 1))
> +#define unlikely(x)    (__builtin_expect(!!(x), 0))
>
>  /* Optimization barrier */
>  #ifndef barrier
> diff --git a/kernel/sched.c b/kernel/sched.c
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -367,7 +367,7 @@ repeat_lock_task:
>         local_irq_save(*flags);
>         rq = task_rq(p);
>         spin_lock(&rq->lock);
> -       if (unlikely(rq != task_rq(p))) {
> +       if unlikely(rq != task_rq(p)) {

This is confusing to read. Why not keep the parenthesis around
(unlikely(...)) ?  Yes, it's an extra set of parenthesis that are not
strictly needed now that you've added them to the likely/unlikely
macros, but they don't do any harm either and make the code less
surprising to read...   I know that I at least think *BUG* at once
when I read a line like
   if unlikely(rq != task_rq(p)) {
and then when I find that it actually compiles fine I go dig for the
reason for that, find the macro and see that all is well, but that
just wasted a lot of time.


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Ingo Molnar]

* David S. Miller <davem@davemloft.net> wrote:

> From: Pavel Machek <pavel@suse.cz>
> Date: Wed, 11 Jan 2006 23:40:13 +0100
> 
> > likely is the evil part here. What about this? Should make this bug
> > impossible to do....
> > 
> > Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> This doesn't let you do:
> 
>      if (likely(y) || unlikely(x))

hm, why not? It will expand to:

   if ((__builtin_expect(!!(y), 1)) || (__builtin_expect(!!(x), 0)))

which seems correct to me. Pavel only added extra parantheses, to make 
the simple case more readable.

	Ingo

Re: 2.6.15-git7 oopses in ext3 during LTP runs II - more problems

[Andi Kleen]

On Wednesday 11 January 2006 22:27, Arjan van de Ven wrote:
> > 15/fs/ext3/super.c~	2006-01-11 21:54:13.000000000 +0100
> > > +++ linux-2.6.15/fs/ext3/super.c	2006-01-11 21:54:13.000000000 +0100
> > > @@ -2150,7 +2150,7 @@
> > >  
> > >  static void ext3_write_super (struct super_block * sb)
> > >  {
> > > -	if (mutex_trylock(&sb->s_lock) != 0)
> > > +	if (!mutex_trylock(&sb->s_lock) != 0)
> > >  		BUG();
> > >  	sb->s_dirt = 0;
> > >  }
> > 
> > We expect the lock to be held on entry.  Hence we expect mutex_trylock()
> > to return zero.
> 
> you are correct, and the x86-64 mutex.h is buggy

While this patch seemed  to fix LTP my desktop running the same kernel (with 
mutex fix) hung the mailer while sending an unrelated mail. Again on ext3.

I unfortunately don't have the backtraces anymore because I couldn't
save them to disk before the reboot (and forgot to copy them
to another system sorry), but they were also hanging in some JBD/ext3
functions in D, with all disk accesses hanging.

So things appear to be still broken in ext3 land.

-Andi

Re: 2.6.15-git7 oopses in ext3 during LTP runs II - more problems

[Andrew Morton]

Andi Kleen <ak@suse.de> wrote:
>
> On Wednesday 11 January 2006 22:27, Arjan van de Ven wrote:
> > > 15/fs/ext3/super.c~	2006-01-11 21:54:13.000000000 +0100
> > > > +++ linux-2.6.15/fs/ext3/super.c	2006-01-11 21:54:13.000000000 +0100
> > > > @@ -2150,7 +2150,7 @@
> > > >  
> > > >  static void ext3_write_super (struct super_block * sb)
> > > >  {
> > > > -	if (mutex_trylock(&sb->s_lock) != 0)
> > > > +	if (!mutex_trylock(&sb->s_lock) != 0)
> > > >  		BUG();
> > > >  	sb->s_dirt = 0;
> > > >  }
> > > 
> > > We expect the lock to be held on entry.  Hence we expect mutex_trylock()
> > > to return zero.
> > 
> > you are correct, and the x86-64 mutex.h is buggy
> 
> While this patch seemed  to fix LTP my desktop running the same kernel (with 
> mutex fix) hung the mailer while sending an unrelated mail. Again on ext3.
> 
> I unfortunately don't have the backtraces anymore because I couldn't
> save them to disk before the reboot (and forgot to copy them
> to another system sorry), but they were also hanging in some JBD/ext3
> functions in D, with all disk accesses hanging.
> 
> So things appear to be still broken in ext3 land.

Are you using MD?

sata?   If so, which?

Re: 2.6.15-git7 oopses in ext3 during LTP runs II - more problems

[Andi Kleen]

On Thursday 12 January 2006 02:14, Andrew Morton wrote:

> Are you using MD?
> 
> sata?   If so, which?

None of both. old style IDE using VIA driver.

-Andi

Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Pavel Machek]

On St 11-01-06 23:47:53, Jesper Juhl wrote:
> On 1/11/06, Pavel Machek <pavel@suse.cz> wrote:
> > On St 11-01-06 22:27:55, Arjan van de Ven wrote:
> > > > We expect the lock to be held on entry.  Hence we expect mutex_trylock()
> > > > to return zero.
> > >
> > > you are correct, and the x86-64 mutex.h is buggy
> > >
> > > --- linux-2.6.15/include/asm-x86_64/mutex.h.org       2006-01-11 22:25:37.000000000 +0100
> > > +++ linux-2.6.15/include/asm-x86_64/mutex.h   2006-01-11 22:25:43.000000000 +0100
> > > @@ -104,7 +104,7 @@
> > >  static inline int
> > >  __mutex_fastpath_trylock(atomic_t *count, int (*fail_fn)(atomic_t *))
> > >  {
> > > -     if (likely(atomic_cmpxchg(count, 1, 0)) == 1)
> > > +     if (likely(atomic_cmpxchg(count, 1, 0) == 1))
> > >               return 1;
> > >       else
> > >               return 0;
> > >
> > > changes the asm to be the correct one for me.
> > > This is odd/evil though..
....
> > +++ b/kernel/sched.c
> > @@ -367,7 +367,7 @@ repeat_lock_task:
> >         local_irq_save(*flags);
> >         rq = task_rq(p);
> >         spin_lock(&rq->lock);
> > -       if (unlikely(rq != task_rq(p))) {
> > +       if unlikely(rq != task_rq(p)) {
> 
> This is confusing to read. Why not keep the parenthesis around
> (unlikely(...)) ?  Yes, it's an extra set of parenthesis that are not
> strictly needed now that you've added them to the likely/unlikely
> macros, but they don't do any harm either and make the code less
> surprising to read...   I know that I at least think *BUG* at once

Read the email from the start, there's very nice example why the extra
()'s are evil in mutex_fast_trylock.

								Pavel
-- 
Thanks, Sharp!