This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix race in pthread_mutex_lock while promoting to PTHREAD_MUTEX_ELISION_NP [BZ #23275]
- From: Florian Weimer <fweimer at redhat dot com>
- To: Stefan Liebler <stli at linux dot ibm dot com>, GNU C Library <libc-alpha at sourceware dot org>
- Cc: Torvald Riegel <triegel at redhat dot com>, Carlos O'Donell <carlos at redhat dot com>
- Date: Mon, 17 Sep 2018 15:38:27 +0200
- Subject: Re: [PATCH] Fix race in pthread_mutex_lock while promoting to PTHREAD_MUTEX_ELISION_NP [BZ #23275]
- References: <f3fc33e5-aa98-f772-a1bf-c02ddf9352c7@linux.ibm.com>
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”.
There are three places where a comma is used before “that” (as a
conjunction). This comma is no longer used in contemporary standard
English.
The comment in force-elision.h references
PTHREAD_MUTEX_TIMED_NO_ELISION_NP and not PTHREAD_MUTEX_NO_ELISION_NP.
Is this deliberate? 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.
Thanks,
Florian