From owner-dev-commits-src-all@freebsd.org Wed Aug 11 22:03:24 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 EFBA665CB6F; Wed, 11 Aug 2021 22:03:24 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (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-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4GlP3068Z6z4dV7; Wed, 11 Aug 2021 22:03:24 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro.local (ralph.baldwin.cx [66.234.199.215]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 3D48DC8BA; Wed, 11 Aug 2021 22:03:24 +0000 (UTC) (envelope-from jhb@FreeBSD.org) To: Luiz Otavio O Souza Cc: Scott Long , src-committers , dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org References: <202108102241.17AMfr63025604@gitrepo.freebsd.org> <9b1ec22c-1545-52ea-7bf9-a96140737711@FreeBSD.org> From: John Baldwin Subject: Re: git: 35547df5c786 - main - Call wakeup() with the lock held to avoid missed wakeup races. Message-ID: Date: Wed, 11 Aug 2021 15:03:19 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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:03:25 -0000 On 8/11/21 2:38 PM, Luiz Otavio O Souza wrote: > On Wed, Aug 11, 2021 at 3:49 PM John Baldwin wrote: >> >> On 8/10/21 3:41 PM, Scott Long wrote: >>> The branch main has been updated by scottl: >>> >>> URL: https://cgit.FreeBSD.org/src/commit/?id=35547df5c78653b2da030f920323c0357056099f >>> >>> commit 35547df5c78653b2da030f920323c0357056099f >>> Author: Scott Long >>> AuthorDate: 2021-08-10 22:36:38 +0000 >>> Commit: Scott Long >>> CommitDate: 2021-08-10 22:36:38 +0000 >>> >>> Call wakeup() with the lock held to avoid missed wakeup races. >>> >>> Submitted by: luiz >>> Sponsored by: Rubicon Communications, LLC ("Netgate") >>> --- >>> sys/dev/sdhci/sdhci.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> 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 __unused, device_t reqdev) >>> /* Deactivate led. */ >>> WR1(slot, SDHCI_HOST_CONTROL, slot->hostctrl &= ~SDHCI_CTRL_LED); >>> slot->bus_busy--; >>> - SDHCI_UNLOCK(slot); >>> wakeup(slot); >>> + SDHCI_UNLOCK(slot); >>> return (0); >>> } >> >> 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: >> >> 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 |= SDHCI_CTRL_LED); >> SDHCI_UNLOCK(slot); >> >> Dropping the lock before wakeup() is a tiny optimization that avoids >> having the second thread wakeup and immediately block on the lock before >> it has been released by the first thread. >> > > '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. 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 awakened and it's just as likely for a thread not yet asleep to acquire the lock as for the thread being awakened to acquire the lock. If you have observed thundering herd problems with this wakeup() then you might want to change it to wakeup_one(). -- John Baldwin