From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 52320 invoked by alias); 17 Sep 2018 13:38:35 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 51835 invoked by uid 89); 17 Sep 2018 13:38:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=english, contemporary, English, Per X-HELO: mx1.redhat.com Subject: Re: [PATCH] Fix race in pthread_mutex_lock while promoting to PTHREAD_MUTEX_ELISION_NP [BZ #23275] To: Stefan Liebler , GNU C Library Cc: Torvald Riegel , Carlos O'Donell References: From: Florian Weimer Message-ID: Date: Mon, 17 Sep 2018 13:38:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2018-09/txt/msg00236.txt.bz2 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