From patchwork Wed Dec 10 16:10:39 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bernie Ogden X-Patchwork-Id: 42090 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ee0-f72.google.com (mail-ee0-f72.google.com [74.125.83.72]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 1BC9326666 for ; Wed, 10 Dec 2014 16:11:29 +0000 (UTC) Received: by mail-ee0-f72.google.com with SMTP id e53sf2473593eek.11 for ; Wed, 10 Dec 2014 08:11:28 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:delivered-to:mailing-list :precedence:list-id:list-unsubscribe:list-subscribe:list-archive :list-post:list-help:sender:delivered-to:from:to:cc:subject:date :message-id:in-reply-to:references:x-original-sender :x-original-authentication-results; bh=L4Nmf00diGKTF3u7xfO2iMqkp6YdLt20uzLqHy9fVH0=; b=HjfydIL3ZtrMNQnYahZ8re/gQy9n3fnk8l9WSSCcblc/qLSQ7hUoAwlb833Dr3jJzn wUXUIf2PEpgBA99p/IH1bOTDmHb6HN6u/uDm7DwEMGk/8T7W4/SGG9mev8C1dPmMfwpj kjERur+VpInwEcbU7kGTpjw2ZtNG+bMByKr+0YQtaLt9s4utgDNJCFG3kflpVcvr4Qjl hTNRwIKqhZipvpmfe4ywDMUXH9SRg8KRi3ytn42od+bkECNCq/gN86eqJMj/e3mZEFjh AlTRkOCrYJSpBqmP6DUKBBshB6pTTzIssxyyz5CPGRAV+Nxt/TvgabFSjaW8aqAWQAh3 /G0g== X-Gm-Message-State: ALoCoQlGaHf/rIGpCLXY+veL3xE7G03Xe5kl5Lq6+NxSgsJOQ0HWiv7u6HSf9AayRU4nMyQx3IuP X-Received: by 10.180.82.34 with SMTP id f2mr1691140wiy.1.1418227888261; Wed, 10 Dec 2014 08:11:28 -0800 (PST) MIME-Version: 1.0 X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.44.135 with SMTP id e7ls2590lam.76.gmail; Wed, 10 Dec 2014 08:11:27 -0800 (PST) X-Received: by 10.112.162.106 with SMTP id xz10mr4828360lbb.91.1418227887955; Wed, 10 Dec 2014 08:11:27 -0800 (PST) Received: from mail-lb0-x233.google.com (mail-lb0-x233.google.com. [2a00:1450:4010:c04::233]) by mx.google.com with ESMTPS id ue9si5096389lbb.41.2014.12.10.08.11.27 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 10 Dec 2014 08:11:27 -0800 (PST) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 2a00:1450:4010:c04::233 as permitted sender) client-ip=2a00:1450:4010:c04::233; Received: by mail-lb0-f179.google.com with SMTP id z11so2553918lbi.24 for ; Wed, 10 Dec 2014 08:11:27 -0800 (PST) X-Received: by 10.112.219.37 with SMTP id pl5mr1691780lbc.25.1418227887787; Wed, 10 Dec 2014 08:11:27 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.142.69 with SMTP id ru5csp249742lbb; Wed, 10 Dec 2014 08:11:26 -0800 (PST) X-Received: by 10.68.97.131 with SMTP id ea3mr8467550pbb.144.1418227885264; Wed, 10 Dec 2014 08:11:25 -0800 (PST) Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id y3si7418065pdi.101.2014.12.10.08.11.24 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 10 Dec 2014 08:11:25 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-55230-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Received: (qmail 1184 invoked by alias); 10 Dec 2014 16:11:12 -0000 Mailing-List: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org Precedence: list List-Id: List-Unsubscribe: , List-Subscribe: List-Archive: List-Post: , List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 1163 invoked by uid 89); 10 Dec 2014 16:11:09 -0000 X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, FSL_HELO_NON_FQDN_1 autolearn=ham version=3.3.2 X-HELO: Ubuntu-1404-trusty-64-minimal From: Bernard Ogden To: triegel@redhat.com Cc: libc-alpha@sourceware.org, carlos@redhat.com, Bernard Ogden Subject: [PATCH v3] lowlevellock comments Date: Wed, 10 Dec 2014 16:10:39 +0000 Message-Id: <1418227839-18932-1-git-send-email-bernie.ogden@linaro.org> In-Reply-To: <'1418147443.25868.216.camel@triegel.csb'> References: <'1418147443.25868.216.camel@triegel.csb'> X-Original-Sender: bernie.ogden@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 2a00:1450:4010:c04::233 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org; dkim=pass header.i=@sourceware.org X-Google-Group-Id: 836684582541 Thanks. I fixed all the easy typos. A few more comments inline below: On 9 December 2014 at 17:50, Torvald Riegel wrote: > On Thu, 2014-10-23 at 13:12 +0100, Bernard Ogden wrote: >> diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c >> index 3525807..2f4f314 100644 >> --- a/nptl/lowlevelrobustlock.c >> +++ b/nptl/lowlevelrobustlock.c >> @@ -36,14 +36,17 @@ __lll_robust_lock_wait (int *futex, int private) >> >> do >> { >> + /* If the owner died, return the present value of the futex. */ >> if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) >> return oldval; >> >> + /* Attempt to put lock into state 'acquired, possibly with waiters'. */ > > "the lock", I guess. Also changed 'Again' to 'Try', just to make the comment fit on one line. >> +/* If LOCK is 0 (not acquired), set to 1 (acquired) and return 0. Otherwise >> + leave lock unchanged and return non-zero to indicate that the lock was not >> + acquired. */ > > At a later time, we should be more precise regarding the semantics of > trylock. See www.open-std.org/jtc1/sc22/WG14/www/docs/n1882.htm for a > related C11 issue. Interesting - thanks for sharing. (I updated the comment here as I noticed I'd just written (acquired) rather than (acquired with no waiters) like I have everywhere else - but that's not related to your point.) >> +/* If FUTEX is 0 (not acquired), set to 1 (acquired with no waiters) and >> + return. Otherwise block until we acquire the lock, at which point FUTEX is >> + 2 (acquired, possibly with waiters), then return. The lock is aways >> + acquired on return. */ > > I'd rearrange the second sentence a little, making sure that we change > to 2 before blocking. Changed to: /* If FUTEX is 0 (not acquired), set to 1 (acquired with no waiters) and return. Otherwise, ensure that it is >1 (acquired, possibly with waiters) and then block until we acquire the lock, at which point FUTEX will still be >1. The lock is always acquired on return. */ Note that I switched '2' to '>1' for consistency - generally I've said '>1' where I can't see the value being set to '2' within lowlevellock.h itself. This also prompted me to change the comment for __lll_robust_lock to indicate a similar point about the FUTEX_WAITERS bit: /* If FUTEX is 0 (not acquired), set to ID (acquired with no waiters) and return 0. Otherwise, ensure that it is set to FUTEX | FUTEX_WAITERS (acquired, possibly with waiters) and block until we acquire the lock. FUTEX will now be ID | FUTEX_WAITERS and we return 0. If the previous owner of the lock dies before we acquire the lock then FUTEX will be the value of id as set by the previous owner, with FUTEX_OWNER_DIED set (FUTEX_WAITERS may or may not be set). We return this value to indicate that the lock is not acquired. */ And then I changed the comment for __lll_robust_cond_lock to point at the __lll_robust_lock comment: /* As __lll_robust_lock, but set to ID | FUTEX_WAITERS (acquired, possibly with waiters) if FUTEX is 0. */ >> +/* Unconditionally set FUTEX to 2 (acquired, possibly with waiters). condvar >> + locks only have 'not acquired' and 'acquired, possibly with waiters' states, >> + so there is no need to check FUTEX before setting. > > I'm not sure whether there are just two states, necessarily. Rather, we > need 'acquired, possibly with waiters'. Generally, setting 'acquired, > possibly with waiters' instead of just 'acquired' is always allowed. I considered replacing this with some text about how it's generally allowable to use '2' rather than '1', but I'd already been much more terse for lll_cond_trylock so just removed the offending sentence. OK now? 2014-12-10 Bernard Ogden * nptl/lowlevellock.c: Add comments. * nptl/lowlevelrobustlock.c: Likewise. * sysdeps/nptl/lowlevellock.h: Likewise. diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c index 4c093fc..e1a203b 100644 --- a/nptl/lowlevellock.c +++ b/nptl/lowlevellock.c @@ -27,10 +27,10 @@ void __lll_lock_wait_private (int *futex) { if (*futex == 2) - lll_futex_wait (futex, 2, LLL_PRIVATE); + lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex == 2. */ while (atomic_exchange_acq (futex, 2) != 0) - lll_futex_wait (futex, 2, LLL_PRIVATE); + lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex == 2. */ } @@ -40,10 +40,10 @@ void __lll_lock_wait (int *futex, int private) { if (*futex == 2) - lll_futex_wait (futex, 2, private); + lll_futex_wait (futex, 2, private); /* Wait if *futex == 2. */ while (atomic_exchange_acq (futex, 2) != 0) - lll_futex_wait (futex, 2, private); + lll_futex_wait (futex, 2, private); /* Wait if *futex == 2. */ } @@ -75,7 +75,7 @@ __lll_timedlock_wait (int *futex, const struct timespec *abstime, int private) if (rt.tv_sec < 0) return ETIMEDOUT; - /* Wait. */ + /* If *futex == 2, wait until woken or timeout. */ lll_futex_timed_wait (futex, 2, &rt, private); } @@ -83,6 +83,11 @@ __lll_timedlock_wait (int *futex, const struct timespec *abstime, int private) } +/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex + wake-up when the clone terminates. The memory location contains the + thread ID while the clone is running and is reset to zero by the kernel + afterwards. The kernel up to version 3.16.3 does not use the private futex + operations for futex wake-up when the clone terminates. */ int __lll_timedwait_tid (int *tidp, const struct timespec *abstime) { @@ -113,8 +118,10 @@ __lll_timedwait_tid (int *tidp, const struct timespec *abstime) if (rt.tv_sec < 0) return ETIMEDOUT; - /* Wait until thread terminates. The kernel so far does not use - the private futex operations for this. */ + /* If *tidp == tid, wait until thread terminates or the wait times out. + The kernel up to version 3.16.3 does not use the private futex + operations for futex wake-up when the clone terminates. + */ if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT) return ETIMEDOUT; } diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c index 3525807..a8813d0 100644 --- a/nptl/lowlevelrobustlock.c +++ b/nptl/lowlevelrobustlock.c @@ -36,14 +36,17 @@ __lll_robust_lock_wait (int *futex, int private) do { + /* If the owner died, return the present value of the futex. */ if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) return oldval; + /* Try to put the lock into state 'acquired, possibly with waiters'. */ int newval = oldval | FUTEX_WAITERS; if (oldval != newval && atomic_compare_and_exchange_bool_acq (futex, newval, oldval)) continue; + /* If *futex == 2, wait until woken. */ lll_futex_wait (futex, newval, private); try: @@ -100,15 +103,17 @@ __lll_robust_timedlock_wait (int *futex, const struct timespec *abstime, return ETIMEDOUT; #endif - /* Wait. */ + /* If the owner died, return the present value of the futex. */ if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED)) return oldval; + /* Try to put the lock into state 'acquired, possibly with waiters'. */ int newval = oldval | FUTEX_WAITERS; if (oldval != newval && atomic_compare_and_exchange_bool_acq (futex, newval, oldval)) continue; + /* If *futex == 2, wait until woken or timeout. */ #if (!defined __ASSUME_FUTEX_CLOCK_REALTIME \ || !defined lll_futex_timed_wait_bitset) lll_futex_timed_wait (futex, newval, &rt, private); diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h index 28f4ba3..d00bc85 100644 --- a/sysdeps/nptl/lowlevellock.h +++ b/sysdeps/nptl/lowlevellock.h @@ -22,9 +22,53 @@ #include #include +/* Low-level locks use a combination of atomic operations (to acquire and + release lock ownership) and futex operations (to block until the state + of a lock changes). A lock can be in one of three states: + 0: not acquired, + 1: acquired with no waiters; no other threads are blocked or about to block + for changes to the lock state, + >1: acquired, possibly with waiters; there may be other threads blocked or + about to block for changes to the lock state. + + We expect that the common case is an uncontended lock, so we just need + to transition the lock between states 0 and 1; releasing the lock does + not need to wake any other blocked threads. If the lock is contended + and a thread decides to block using a futex operation, then this thread + needs to first change the state to >1; if this state is observed during + lock release, the releasing thread will wake one of the potentially + blocked threads. + + Much of this code takes a 'private' parameter. This may be: + LLL_PRIVATE: lock only shared within a process + LLL_SHARED: lock may be shared across processes. + + Condition variables contain an optimization for broadcasts that requeues + waiting threads on a lock's futex. Therefore, there is a special + variant of the locks (whose name contains "cond") that makes sure to + always set the lock state to >1 and not just 1. + + Robust locks set the lock to the id of the owner. This allows detection + of the case where the owner exits without releasing the lock. Flags are + OR'd with the owner id to record additional information about lock state. + Therefore the states of robust locks are: + 0: not acquired + id: acquired (by user identified by id & FUTEX_TID_MASK) + + The following flags may be set in the robust lock value: + FUTEX_WAITERS - possibly has waiters + FUTEX_OWNER_DIED - owning user has exited without releasing the futex. */ + + +/* If LOCK is 0 (not acquired), set to 1 (acquired with no waiters) and return + 0. Otherwise leave lock unchanged and return non-zero to indicate that the + lock was not acquired. */ #define lll_trylock(lock) \ atomic_compare_and_exchange_bool_acq (&(lock), 1, 0) +/* If LOCK is 0 (not acquired), set to 2 (acquired, possibly with waiters) and + return 0. Otherwise leave lock unchanged and return non-zero to indicate + that the lock was not acquired. */ #define lll_cond_trylock(lock) \ atomic_compare_and_exchange_bool_acq (&(lock), 2, 0) @@ -35,6 +79,13 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden; /* This is an expression rather than a statement even though its value is void, so that it can be used in a comma expression or as an expression that's cast to void. */ +/* The inner conditional compiles to a call to __lll_lock_wait_private if + private is known at compile time to be LLL_PRIVATE, and to a call to + __lll_lock_wait otherwise. */ +/* If FUTEX is 0 (not acquired), set to 1 (acquired with no waiters) and + return. Otherwise, ensure that it is >1 (acquired, possibly with waiters) + and then block until we acquire the lock, at which point FUTEX will still be + >1. The lock is always acquired on return. */ #define __lll_lock(futex, private) \ ((void) \ ({ \ @@ -52,6 +103,14 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden; __lll_lock (&(futex), private) +/* If FUTEX is 0 (not acquired), set to ID (acquired with no waiters) and + return 0. Otherwise, ensure that it is set to FUTEX | FUTEX_WAITERS + (acquired, possibly with waiters) and block until we acquire the lock. + FUTEX will now be ID | FUTEX_WAITERS and we return 0. + If the previous owner of the lock dies before we acquire the lock then FUTEX + will be the value of id as set by the previous owner, with FUTEX_OWNER_DIED + set (FUTEX_WAITERS may or may not be set). We return this value to indicate + that the lock is not acquired. */ #define __lll_robust_lock(futex, id, private) \ ({ \ int *__futex = (futex); \ @@ -69,6 +128,10 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden; /* This is an expression rather than a statement even though its value is void, so that it can be used in a comma expression or as an expression that's cast to void. */ +/* Unconditionally set FUTEX to 2 (acquired, possibly with waiters). If FUTEX + was 0 (not acquired) then return. Otherwise, block until the lock is + acquired, at which point FUTEX is 2 (acquired, possibly with waiters). The + lock is always acquired on return. */ #define __lll_cond_lock(futex, private) \ ((void) \ ({ \ @@ -79,6 +142,8 @@ extern int __lll_robust_lock_wait (int *futex, int private) attribute_hidden; #define lll_cond_lock(futex, private) __lll_cond_lock (&(futex), private) +/* As __lll_robust_lock, but set to ID | FUTEX_WAITERS (acquired, possibly with + waiters) if FUTEX is 0. */ #define lll_robust_cond_lock(futex, id, private) \ __lll_robust_lock (&(futex), (id) | FUTEX_WAITERS, private) @@ -88,8 +153,9 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *, extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, int private) attribute_hidden; -/* Take futex if it is untaken. - Otherwise block until either we get the futex or abstime runs out. */ + +/* As __lll_lock, but with a timeout. If the timeout occurs then return + ETIMEDOUT. If ABSTIME is invalid, return EINVAL. */ #define __lll_timedlock(futex, abstime, private) \ ({ \ int *__futex = (futex); \ @@ -104,6 +170,8 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, __lll_timedlock (&(futex), abstime, private) +/* As __lll_robust_lock, but with a timeout. If the timeout occurs then return + ETIMEDOUT. If ABSTIME is invalid, return EINVAL. */ #define __lll_robust_timedlock(futex, abstime, id, private) \ ({ \ int *__futex = (futex); \ @@ -121,6 +189,9 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, /* This is an expression rather than a statement even though its value is void, so that it can be used in a comma expression or as an expression that's cast to void. */ +/* Unconditionally set FUTEX to 0 (not acquired), releasing the lock. If FUTEX + was >1 (acquired, possibly with waiters), then wake any waiters. The waiter + that acquires the lock will set FUTEX to >1. */ #define __lll_unlock(futex, private) \ ((void) \ ({ \ @@ -136,6 +207,9 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, /* This is an expression rather than a statement even though its value is void, so that it can be used in a comma expression or as an expression that's cast to void. */ +/* Unconditionally set FUTEX to 0 (not acquired), releasing the lock. If FUTEX + had FUTEX_WAITERS set then wake any waiters. The waiter that acquires the + lock will set FUTEX_WAITERS. */ #define __lll_robust_unlock(futex, private) \ ((void) \ ({ \ @@ -159,15 +233,12 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, #define LLL_LOCK_INITIALIZER (0) #define LLL_LOCK_INITIALIZER_LOCKED (1) -/* The states of a lock are: - 0 - untaken - 1 - taken by one user - >1 - taken by more users */ /* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex - wakeup when the clone terminates. The memory location contains the - thread ID while the clone is running and is reset to zero - afterwards. */ + wake-up when the clone terminates. The memory location contains the + thread ID while the clone is running and is reset to zero by the kernel + afterwards. The kernel up to version 3.16.3 does not use the private futex + operations for futex wake-up when the clone terminates. */ #define lll_wait_tid(tid) \ do { \ __typeof (tid) __tid; \ @@ -178,6 +249,8 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, extern int __lll_timedwait_tid (int *, const struct timespec *) attribute_hidden; +/* As lll_wait_tid, but with a timeout. If the timeout occurs then return + ETIMEDOUT. If ABSTIME is invalid, return EINVAL. */ #define lll_timedwait_tid(tid, abstime) \ ({ \ int __res = 0; \