From patchwork Mon Jan 2 19:26:14 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Torvald Riegel X-Patchwork-Id: 89573 Delivered-To: patch@linaro.org Received: by 10.140.20.101 with SMTP id 92csp7715946qgi; Mon, 2 Jan 2017 11:26:33 -0800 (PST) X-Received: by 10.84.234.9 with SMTP id m9mr97976965plk.49.1483385193769; Mon, 02 Jan 2017 11:26:33 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id f35si66545183plh.212.2017.01.02.11.26.33 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 02 Jan 2017 11:26:33 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-76552-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org; spf=pass (google.com: domain of libc-alpha-return-76552-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-76552-patch=linaro.org@sourceware.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:subject:from:to:date:in-reply-to :references:content-type:mime-version; q=dns; s=default; b=dFs2I KOPQCMl3RgbrK0LARDsEW3C2ZywpyRHtTHYZm8SNIq4yy5ZvRssrh2uMK1hN03dL Tj+i1PJMYowYIbTEGui7Y9SR7OgHq8AvBsccvk4crRirS2Y/VwQebhoPM3wnXugl sANidjmdKG1XJbNglQ7sCKjSkGxv4/Ub0GMOo8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:subject:from:to:date:in-reply-to :references:content-type:mime-version; s=default; bh=ahTiwI6Seyy Wh+Sb/PuCoBLZrB4=; b=ovjUNWOb6T5nvuhjoRb6pk5jkiOjtFhBEqKLsABvcZT f3pBrSWXB6TALSyLe5GYK9GpoWf5gpHApE+IBZfMRlKBNiMGy3a16dJTEYLfmOp6 dSnGVqFa1FrVNPPZmfZaO6OG/Dz/z/oBUk9Lxi719nV1vvR9w/iqLdZtjc1a9XRs = Received: (qmail 115912 invoked by alias); 2 Jan 2017 19:26:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk 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 115865 invoked by uid 89); 2 Jan 2017 19:26:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=wake, acquisition, 72, 6, ebusy X-HELO: mx1.redhat.com Message-ID: <1483385174.13143.124.camel@redhat.com> Subject: Re: [PATCH 2/2] New pthread rwlock that is more scalable. From: Torvald Riegel To: GLIBC Devel Date: Mon, 02 Jan 2017 20:26:14 +0100 In-Reply-To: <1469655868.19224.34.camel@localhost.localdomain> References: <1469655533.19224.27.camel@localhost.localdomain> <1469655868.19224.34.camel@localhost.localdomain> Mime-Version: 1.0 On Wed, 2016-07-27 at 23:44 +0200, Torvald Riegel wrote: > This replaces the pthread rwlock with a new implementation that uses a > more scalable algorithm (primarily through not using a critical section > anymore to make state changes). The fast path for rdlock acquisition > and release is now basically a single atomic read-modify write or CAS > and a few branches. See nptl/pthread_rwlock_common.c for details. I have noticed two small oversights, which are taken care of in the attached patch. The first is a mssign overflow check (a lock acquired too often as a reader) in one of the tryrdlock branches. The second is a that I had forgotten to apply a cleanup (no correctness change; the former code did more than it had to). commit 59c2c0dafb1c1460a457037f222032ade9fc5a74 Author: Torvald Riegel Date: Mon Jan 2 17:50:37 2017 +0100 Fix a minor issue and an oversight (not a correctness bug) in tryrdlock diff --git a/nptl/pthread_rwlock_tryrdlock.c b/nptl/pthread_rwlock_tryrdlock.c index e002f15..6c3014c 100644 --- a/nptl/pthread_rwlock_tryrdlock.c +++ b/nptl/pthread_rwlock_tryrdlock.c @@ -51,12 +51,6 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock) == PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP)) return EBUSY; rnew = r + (1 << PTHREAD_RWLOCK_READER_SHIFT); - /* If we could have caused an overflow or take effect during an - overflow, we just can / need to return EAGAIN. There is no need - to have modified the number of readers because we could have - done that and cleaned up immediately. */ - if (rnew >= PTHREAD_RWLOCK_READER_OVERFLOW) - return EAGAIN; } else { @@ -72,6 +66,12 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock) ^ PTHREAD_RWLOCK_WRPHASE; } } + /* If we could have caused an overflow or take effect during an + overflow, we just can / need to return EAGAIN. There is no need to + have actually modified the number of readers because we could have + done that and cleaned up immediately. */ + if (rnew >= PTHREAD_RWLOCK_READER_OVERFLOW) + return EAGAIN; } /* If the CAS fails, we retry; this prevents that tryrdlock fails spuriously (i.e., fails to acquire the lock although there is no writer), which is @@ -84,16 +84,25 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock) readers or writers that acquire and release in the meantime. Using randomized exponential back-off to make a live-lock unlikely should be sufficient. + TODO Back-off. Acquire MO so we synchronize with prior writers. */ while (!atomic_compare_exchange_weak_acquire (&rwlock->__data.__readers, &r, rnew)); if ((r & PTHREAD_RWLOCK_WRPHASE) != 0) { - //FIXME / TODO same as in rdlock_full - int private = __pthread_rwlock_get_private (rwlock); - atomic_store_release (&rwlock->__data.__wrphase_futex, 0); - futex_wake (&rwlock->__data.__wrphase_futex, INT_MAX, private); + /* Same as in __pthread_rwlock_rdlock_full: + We started the read phase, so we are also responsible for + updating the write-phase futex. Relaxed MO is sufficient. + Note that there can be no other reader that we have to wake + because all other readers will see the read phase started by us + (or they will try to start it themselves); if a writer started + the read phase, we cannot have started it. Furthermore, we + cannot discard a PTHREAD_RWLOCK_FUTEX_USED flag because we will + overwrite the value set by the most recent writer (or the readers + before it in case of explicit hand-over) and we know that there + are no waiting readers. */ + atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 0); } return 0;