On 09/17/2018 03:38 PM, Florian Weimer wrote:
> On 06/12/2018 04:24 PM, Stefan Liebler wrote:
> 
>> ChangeLog:
>>
>>      [BZ #23275]
>>      * nptl/tst-mutex10.c: New File.
>>      * nptl/Makefile (tests): Add tst-mutex10.
>>      (tst-mutex-ENV): New variable.
>>      * sysdeps/unix/sysv/linux/s390/force-elision.h: (FORCE_ELISION):
>>      Ensure that elision path is used if elision is available.
>>      * sysdeps/unix/sysv/linux/powerpc/force-elision.h
>>      (FORCE_ELISION): Likewise.
>>      * sysdeps/unix/sysv/linux/x86/force-elision.h: (FORCE_ELISION):
>>      Likewise.
>>      * nptl/pthreadP.h (PTHREAD_MUTEX_TYPE,
>>      PTHREAD_MUTEX_TYPE_ELISION, PTHREAD_MUTEX_PSHARED):
>>      Use atomic_load_relaxed.
>>      * nptl/pthread_mutex_consistent.c (pthread_mutex_consistent):
>>      Likewise.
>>      * nptl/pthread_mutex_getprioceiling.c
>>      (pthread_mutex_getprioceiling): Likewise.
>>      * nptl/pthread_mutex_lock.c (__pthread_mutex_lock_full,
>>      __pthread_mutex_cond_lock_adjust): Likewise.
>>      * nptl/pthread_mutex_setprioceiling.c
>>      (pthread_mutex_setprioceiling): Likewise.
>>      * nptl/pthread_mutex_timedlock.c (__pthread_mutex_timedlock):
>>      Likewise.
>>      * nptl/pthread_mutex_trylock.c (__pthread_mutex_trylock):
>>      Likewise.
>>      * nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full):
>>      Likewise.
>>      * sysdeps/nptl/bits/thread-shared-types.h
>>      (struct __pthread_mutex_s): Add comments.
>>      * nptl/pthread_mutex_destroy.c (__pthread_mutex_destroy):
>>      Use atomic_load_relaxed and atomic_store_relaxed.
>>      * nptl/pthread_mutex_init.c (__pthread_mutex_init):
>>      Use atomic_store_relaxed.
> 
> I had another look at this.  I think the code changes are okay, but:
> 
> There is a reference to “pthread_mutex_destroy()” in the new test.  Per 
> GNU style, this should just be “pthread_mutex_destroy”.
done

> 
> There are three places where a comma is used before “that” (as a 
> conjunction).  This comma is no longer used in contemporary standard 
> English.
Changed it in the three force-elision.h files.

> 
> The comment in force-elision.h references 
> PTHREAD_MUTEX_TIMED_NO_ELISION_NP and not PTHREAD_MUTEX_NO_ELISION_NP. > Is this deliberate?
Yes, you are right. This should be PTHREAD_MUTEX_NO_ELISION_NP as 
PTHREAD_MUTEX_TIMED_NO_ELISION_NP is a more specialized flag as 
PTHREAD_MUTEX_NO_ELISION_NP. Changed it.

> I also find the second part of this comment 
> confusing (which contains the flag reference) a bit confusing.  I assume 
> that PTHREAD_MUTEX_NO_ELISION_NP is somehow checked before FORCE_ELISION 
> is called, and that check is not racy because 
> PTHREAD_MUTEX_NO_ELISION_NP is unchanged after mutex initialization.  So 
> for each particular mutex, we either always enable elision as part of 
> the first locking operation, or we never do.
The flag PTHREAD_MUTEX_NO_ELISION_NP is not explicitly checked before 
FORCE_ELISION is called. But only one of those two flags can be set.
Your remaining assumption is correct.
I've changed the comment in the three force-elision.h files. Is this okay?

I've attached the new version of the patch. Only comments are changed. 
The code is unchanged.

> 
> Thanks,
> Florian
> 

Thanks,
Stefan