From patchwork Sat Jan 28 02:18:23 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: develop--- via Libc-alpha X-Patchwork-Id: 63833 X-Patchwork-Delegate: carlos@redhat.com Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2DA233857803 for ; Sat, 28 Jan 2023 02:19:19 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2DA233857803 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1674872359; bh=+qL6X14k0T5H0mhNlVsCvo9CP1nbBUAOkp0uo/T6Hf8=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=bfA1GnWtp2b1SnFg2QdCRiJspZwPq05FxQb11nURJZPo5Hb4rFxyER/h9lwbUCoUK Hvyo3TcC/HvZ0w/vk4nNMLvUKm792cwyBmUsXRpBUTwtKmviRfB/ScfGgHgSPjxKG+ YqR0zJQjwnP7ulYN3j0JTj6v35cK2LdlGLNdEWEo= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from wout4-smtp.messagingengine.com (wout4-smtp.messagingengine.com [64.147.123.20]) by sourceware.org (Postfix) with ESMTPS id AD9FA3858C3A for ; Sat, 28 Jan 2023 02:18:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AD9FA3858C3A Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.west.internal (Postfix) with ESMTP id 931CC32009BA; Fri, 27 Jan 2023 21:18:44 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Fri, 27 Jan 2023 21:18:44 -0500 X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedruddvjedggeeiucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkofgjfhgggfestdekredtredttdenucfhrhhomhepmhgrlhht vghskhgrrhhuphhkvgesfhgrshhtmhgrihhlrdhfmhenucggtffrrghtthgvrhhnpeetge elgfeggeeuleeuffetveefgffgjedvgeehffdthfekteegtdeguefhffeftdenucevlhhu shhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehmrghlthgvshhkrg hruhhpkhgvsehfrghsthhmrghilhdrfhhm X-ME-Proxy: Feedback-ID: ifa6c408f:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 27 Jan 2023 21:18:43 -0500 (EST) To: libc-alpha@sourceware.org Cc: Malte Skarupke Subject: [PATCH 3/9] nptl: Remove unnecessary catch-all-wake in condvar group switch Date: Fri, 27 Jan 2023 21:18:23 -0500 Message-Id: <20230128021829.7990-4-malteskarupke@fastmail.fm> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230128021829.7990-1-malteskarupke@fastmail.fm> References: <20230128021829.7990-1-malteskarupke@fastmail.fm> MIME-Version: 1.0 X-Spam-Status: No, score=-13.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: malteskarupke--- via Libc-alpha From: develop--- via Libc-alpha Reply-To: malteskarupke@fastmail.fm Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" From: Malte Skarupke This wake is unnecessary. We only switch groups after every sleeper in a group has been woken. Sure, they may take a while to actually wake up and may still hold a reference, but waking them a second time doesn't speed that up. Instead this just makes the code more complicated and may hide problems. In particular this safety wake wouldn't even have helped with the bug that was fixed by Barrus' patch: The bug there was that pthread_cond_signal would not switch g1 when it should, so we wouldn't even have entered this code path. --- nptl/pthread_cond_common.c | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/nptl/pthread_cond_common.c b/nptl/pthread_cond_common.c index 227a7527e4..c8160036d0 100644 --- a/nptl/pthread_cond_common.c +++ b/nptl/pthread_cond_common.c @@ -221,13 +221,7 @@ __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq, * New waiters arriving concurrently with the group switching will all go into G2 until we atomically make the switch. Waiters existing in G2 are not affected. - * Waiters in G1 have already received a signal and been woken. If they - haven't woken yet, they will be closed out immediately by the advancing - of __g_signals to the next "lowseq" (low 31 bits of the new g1_start), - which will prevent waiters from blocking using a futex on - __g_signals since it provides enough signals for all possible - remaining waiters. As a result, they can each consume a signal - and they will eventually remove their group reference. */ + * Waiters in G1 have already received a signal and been woken. */ /* Update __g1_start, which finishes closing this group. The value we add will never be negative because old_orig_size can only be zero when we @@ -240,29 +234,6 @@ __condvar_quiesce_and_switch_g1 (pthread_cond_t *cond, uint64_t wseq, unsigned int lowseq = ((old_g1_start + old_orig_size) << 1) & ~1U; - /* If any waiters still hold group references (and thus could be blocked), - then wake them all up now and prevent any running ones from blocking. - This is effectively a catch-all for any possible current or future - bugs that can allow the group size to reach 0 before all G1 waiters - have been awakened or at least given signals to consume, or any - other case that can leave blocked (or about to block) older waiters.. */ - if ((atomic_fetch_or_release (cond->__data.__g_refs + g1, 0) >> 1) > 0) - { - /* First advance signals to the end of the group (i.e. enough signals - for the entire G1 group) to ensure that waiters which have not - yet blocked in the futex will not block. - Note that in the vast majority of cases, this should never - actually be necessary, since __g_signals will have enough - signals for the remaining g_refs waiters. As an optimization, - we could check this first before proceeding, although that - could still leave the potential for futex lost wakeup bugs - if the signal count was non-zero but the futex wakeup - was somehow lost. */ - atomic_store_relaxed (cond->__data.__g_signals + g1, lowseq); - - futex_wake (cond->__data.__g_signals + g1, INT_MAX, private); - } - /* At this point, the old G1 is now a valid new G2 (but not in use yet). No old waiter can neither grab a signal nor acquire a reference without noticing that __g1_start is larger.