Re: [PATCH] powerpc: remove linux lowlevellock.h

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2014-09-23 at 14:35 -0300, Adhemerval Zanella wrote:
> diff --git a/sysdeps/unix/sysv/linux/powerpc/pthread_spin_unlock.c b/sysdeps/unix/sysv/linux/powerpc/pthread_spin_unlock.c
> index 2b8c84d..4906205 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/pthread_spin_unlock.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/pthread_spin_unlock.c
> @@ -22,7 +22,7 @@
>  int
>  pthread_spin_unlock (pthread_spinlock_t *lock)
>  {
> -  __asm __volatile (__lll_rel_instr ::: "memory");
> +  __asm __volatile (__ARCH_REL_INSTR ::: "memory");
>    *lock = 0;
>    return 0;
>  }

I'd prefer if this would be a memory_order_release barrier (which we
don't have yet, I know, so bear with me).  However, I see that the
current situation is a bit messy:
* It seems that atomic_write_barrier is intended to be a C11
memory_order_release barrier/fence; at least it's used like that in all
what I saw in glibc.  So atomic_write_barrier would be our current way
to express a release barrier.
* However, PowerPC atomic_write_barrier is "eieio" and is not using
__ARCH_REL_INSTR.  Can you clarify the difference?

I'm interested in having a release barrier there so that this won't get
overlooked once we do the actual transition to C11 atomics.

Also, related to that, it would be ideal if we could use the generic
implementation (nptl/pthread_spin_unlock.c) on Power as well.  However,
this one uses atomic_full_barrier, which is stronger than what's used on
the major archs (x86, Power, ARM).
I strongly believe this should use a release barrier, despite what POSIX
is claiming (see #16432), but we'll have to deal with that separately.

> diff --git a/sysdeps/unix/sysv/linux/powerpc/sem_post.c b/sysdeps/unix/sysv/linux/powerpc/sem_post.c
> index f222d9a..831a8dd 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/sem_post.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/sem_post.c
> @@ -30,9 +30,9 @@ __new_sem_post (sem_t *sem)
>  {
>    struct new_sem *isem = (struct new_sem *) sem;
>  
> -  __asm __volatile (__lll_rel_instr ::: "memory");
> +  __asm __volatile (__ARCH_REL_INSTR ::: "memory");
>    atomic_increment (&isem->value);
> -  __asm __volatile (__lll_acq_instr ::: "memory");
> +  __asm __volatile (__ARCH_ACQ_INSTR ::: "memory");
>    if (isem->nwaiters > 0)
>      {
>        int err = lll_futex_wake (&isem->value, 1,
> @@ -55,7 +55,7 @@ __old_sem_post (sem_t *sem)
>  {
>    int *futex = (int *) sem;
>  
> -  __asm __volatile (__lll_rel_instr ::: "memory");
> +  __asm __volatile (__ARCH_REL_INSTR ::: "memory");
>    (void) atomic_increment_val (futex);
>    /* We always have to assume it is a shared semaphore.  */
>    int err = lll_futex_wake (futex, 1, LLL_SHARED);

These are fine, though, as we'll need to change the semaphore algorithm
anyway to fulfill the destruction guarantees (I have a WIP
implementation that I plan to post in the near future).