Re: [PATCH roland/lll-futex] Separate Linuxisms from lowlevellock.h, make a generic one

[Bernard Ogden <bernie dot ogden at linaro dot org>]

Ugh - yes, I missed that some syscalls would return non-zero for success.

I have a few comments, 2 about other things I missed before, and 1 about the bug I was trying to fix.

On 3 Jul 2014, at 00:14, Roland McGrath <roland@hack.frob.com> wrote:

> +#define __lll_robust_trylock(futex, id) \
> +  (atomic_compare_and_exchange_val_acq (futex, id, 0) != 0)

My first thought was that the != 0 is superfluous. Either way you return 0 if you changed the memory, and non-zero if you didn't.

But looking at the call in pthread_mutex_trylock.c, I wonder if both patches have this wrong. Should we be returning the old value here? That's what (at least) m68k does, so there's a difference that'll need resolving.

> +#define __lll_cond_lock(futex, private)                         \
> +  ((void)                                                       \
> +   ({                                                           \
> +     int *__futex = (futex);                                    \
> +     if (__glibc_unlikely (atomic_exchange_acq (__futex, 2)))   \
> +       __lll_lock_wait (__futex, private);                      \
> +   }))

This is unconditionally setting the futex to 2, where my patch (based on Tile) sets it to 2 only if it was previously 0. Again, the platforms are inconsistent so it'll need resolving.

> +#define __lll_timedlock(futex, abstime, private)                \
> +  ({                                                            \
> +    int *__futex = (futex);                                     \
> +    int __val = 0;                                              \
> +                                                                \
> +    if (__glibc_unlikely (atomic_exchange_acq (__futex, 1)))    \
> +      __val = __lll_timedlock_wait (__futex, abstime, private); \
> +    __val;                                                      \
> +  })

This'll spread BZ 16892 (and be a binary difference on the currently unaffected platforms).

Should be something like:

if (__glibc_unlikely (atomic_compare_and_exchange_bool_acq (__futex, 1, 0)))