On 1/11/06, Pavel Machek <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
