From owner-dev-commits-src-all@freebsd.org Wed Aug 11 22:50:25 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 8C64465E104; Wed, 11 Aug 2021 22:50:25 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from wout5-smtp.messagingengine.com (wout5-smtp.messagingengine.com [64.147.123.21]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4GlQ5F2VSrz4h1Q; Wed, 11 Aug 2021 22:50:25 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id 2A5873200916; Wed, 11 Aug 2021 18:50:18 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 11 Aug 2021 18:50:18 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsco.org; h= content-type:content-transfer-encoding:from:mime-version:subject :date:message-id:references:cc:in-reply-to:to; s=fm1; bh=bma5HVC 7MvaQRIzPhd6v4gyq1q4TO3YFqa8xq2Lsb08=; b=V1ZIeQq8BIEkYvyWVE2Z5VB 2rv5UVFMcWDCIF03y+n7SOK5vKos9RkWkzlheNNPRtza0KEDLM0myvHnQNYFy4k9 OBxV8IimNKv9TlxgTbBRnxwf8JyFLs3HGFMiGRNTjNIZ2Zip7jR3dzoVwn0r8aPc n5Lgl3cL/LmYLEGa+h28lLR2owd38NEgnWIvGB9xYJYkH6uwLF3wflljzSKGVllF R/Ov/iOOEhnEpGU+iyOGb4oskrw8RAoLcqgkAkrQwW1ibrOXWvpZo8wCxtNFOsMV awzLYEzbs/nhrmc4hTDU9jLobNH221zq8R97B3Y6b9YczBG9/vQNEOl2A3hoINw= = DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=bma5HVC7MvaQRIzPhd6v4gyq1q4TO3YFqa8xq2Lsb 08=; b=rBHlTKlxqSxCelTs/OO6OZBzPPq84zdzaU/d6WMxFAmuh2PIMfxYtyHqM GT9BLLQucYk0meoEl78rM4HFPJr7oV6A0+d74p1W5HaF+nWMbk+65neKiRjIt7Pz p6Dl3bPsrSpOt9VFG2sOUNWk4q81+TAC4z2dLHLKSu1ph0HPccWbeKPafzC6P6QB 5XRtdQnyIRad39VmZF94+mj+GrhOYpB19qrcaSpOpKyXpEWDi7Nmnxcvi19MnSoM Xc3/JkzXVgwakX+gDFAFOgGcdOOKfan/K0eX/6vHD7YvQ5mLFLABoMe3jCe+TdOB OZ8qUf5iFaz7P59k3kcESbgRVwKnA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrkedvgddugecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecunecujfgurheptgfghfggufffkfhfjgfvofesthhqmh dthhdtjeenucfhrhhomhepufgtohhtthcunfhonhhguceoshgtohhtthhlsehsrghmshgt ohdrohhrgheqnecuggftrfgrthhtvghrnhepfeevgfevveettdekhfduueduhefgieejtd elgeehieeuuefgueekffejtdeftdehnecuffhomhgrihhnpehfrhgvvggsshgurdhorhhg necuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepshgtoh htthhlsehsrghmshgtohdrohhrgh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 11 Aug 2021 18:50:17 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: Scott Long Mime-Version: 1.0 (1.0) Subject: Re: git: 35547df5c786 - main - Call wakeup() with the lock held to avoid missed wakeup races. Date: Wed, 11 Aug 2021 16:50:15 -0600 Message-Id: <8FB57A36-8D3B-4B7B-8397-6D742694DD75@samsco.org> References: <9587664b-5c82-cbe2-fb13-6de6abb9843d@FreeBSD.org> Cc: Luiz Otavio O Souza , Scott Long , src-committers , dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org In-Reply-To: <9587664b-5c82-cbe2-fb13-6de6abb9843d@FreeBSD.org> To: John Baldwin X-Mailer: iPhone Mail (18G82) X-Rspamd-Queue-Id: 4GlQ5F2VSrz4h1Q X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; TAGGED_RCPT(0.00)[]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Aug 2021 22:50:25 -0000 > On Aug 11, 2021, at 4:42 PM, John Baldwin wrote: >=20 > =EF=BB=BFOn 8/11/21 3:31 PM, Luiz Otavio O Souza wrote: >>> On Wed, Aug 11, 2021 at 7:03 PM John Baldwin wrote: >>>=20 >>> On 8/11/21 2:38 PM, Luiz Otavio O Souza wrote: >>>> On Wed, Aug 11, 2021 at 3:49 PM John Baldwin wrote: >>>>>=20 >>>>> On 8/10/21 3:41 PM, Scott Long wrote: >>>>>> The branch main has been updated by scottl: >>>>>>=20 >>>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=3D35547df5c78653b2da030f= 920323c0357056099f >>>>>>=20 >>>>>> commit 35547df5c78653b2da030f920323c0357056099f >>>>>> Author: Scott Long >>>>>> AuthorDate: 2021-08-10 22:36:38 +0000 >>>>>> Commit: Scott Long >>>>>> CommitDate: 2021-08-10 22:36:38 +0000 >>>>>>=20 >>>>>> Call wakeup() with the lock held to avoid missed wakeup races. >>>>>>=20 >>>>>> Submitted by: luiz >>>>>> Sponsored by: Rubicon Communications, LLC ("Netgate") >>>>>> --- >>>>>> sys/dev/sdhci/sdhci.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>=20 >>>>>> diff --git a/sys/dev/sdhci/sdhci.c b/sys/dev/sdhci/sdhci.c >>>>>> index d075c2e05000..573e6949b57e 100644 >>>>>> --- a/sys/dev/sdhci/sdhci.c >>>>>> +++ b/sys/dev/sdhci/sdhci.c >>>>>> @@ -2078,8 +2078,8 @@ sdhci_generic_release_host(device_t brdev __unu= sed, device_t reqdev) >>>>>> /* Deactivate led. */ >>>>>> WR1(slot, SDHCI_HOST_CONTROL, slot->hostctrl &=3D ~SDHCI_CTRL_= LED); >>>>>> slot->bus_busy--; >>>>>> - SDHCI_UNLOCK(slot); >>>>>> wakeup(slot); >>>>>> + SDHCI_UNLOCK(slot); >>>>>> return (0); >>>>>> } >>>>>=20 >>>>> Hmm, how does this avoid a race? The sleep is checking bus_busy under= >>>>> the lock and should never see a stale value and go back to sleep after= >>>>> the wakeup has occurred: >>>>>=20 >>>>> SDHCI_LOCK(slot); >>>>> while (slot->bus_busy) >>>>> msleep(slot, &slot->mtx, 0, "sdhciah", 0); >>>>> slot->bus_busy++; >>>>> /* Activate led. */ >>>>> WR1(slot, SDHCI_HOST_CONTROL, slot->hostctrl |=3D SDHCI_CTRL_= LED); >>>>> SDHCI_UNLOCK(slot); >>>>>=20 >>>>> Dropping the lock before wakeup() is a tiny optimization that avoids >>>>> having the second thread wakeup and immediately block on the lock befo= re >>>>> it has been released by the first thread. >>>>>=20 >>>>=20 >>>> 'race' is probably wrong here. this change will prevent a second >>>> thread from taking the bus before you call wakeup() - poking all other >>>> threads unnecessarily. >>>=20 >>> This change does not prevent that. The other thread and the thread that= >>> are awakened will race with each other to acquire the lock. wakeup() >>> doesn't do any sort of explicit lock handoff to the thread being awakene= d >>> and it's just as likely for a thread not yet asleep to acquire the lock a= s >>> for the thread being awakened to acquire the lock. If you have observed= >>> thundering herd problems with this wakeup() then you might want to chang= e >>> it to wakeup_one(). >> correct, but to be more specific, on the first thread, after you free >> the bus and release the lock, a new thread can run, successfully >> acquire the lock and grab the bus. at this point the first thread >> resumes and call wakeup(). when that happens, the new thread always >> wins, the threads being awakened won't have a chance. >=20 > Perhaps on a uniprocessor system this might be true, but otherwise > your new thread is likely spinning adaptively on the lock on another > CPU and grabs it as soon as it is released before the thread awakened by > wakeup() is even scheduled on a CPU and given a chance to run. That is, > I suspect the new thread always wins even with this change on any > multiprocessor system, but it now has to wait a bit longer before it > wins. Have you observed some specific behavior with traces that this > change seeks to address? >=20 > --=20 > John Baldwin Thats a lousy question, John. Yes, we have, and yes, we still support unipr= ocessor systems. This isnt fast path code, and it solves a problem for us. Scott=