Re: 2.6.15-git7 oopses in ext3 during LTP runs

[Jesper Juhl <jesper.juhl@gmail.com>]

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