From owner-dev-commits-src-main@freebsd.org Wed Aug 11 22:50:25 2021
Return-Path: <owner-dev-commits-src-main@freebsd.org>
Delivered-To: dev-commits-src-main@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: <xms:KVQUYU6D4jogLiKiYtLZQgPGHbDAXok_DmVZqceeimBuKHYooH1gFA>
<xme:KVQUYV4YSIyAf1WQDW5sm2nHpWKWnKHFy7HyylRzNIfnXOg7ozSr4AmkQK81D-huY
jCdTUDJ887udnhLhw>
X-ME-Received: <xmr:KVQUYTcsn3K3ikoPxWptqeIkH_2J5hnAtD6nCVxCpkcI3xklyuIv9pl1d03TWn6UJNM9wduRhIyJFO_VLq2VBdQCnxpuUKkFs9ar>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrkedvgddugecutefuodetggdotefrodftvf
curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu
uegrihhlohhuthemuceftddtnecunecujfgurheptgfghfggufffkfhfjgfvofesthhqmh
dthhdtjeenucfhrhhomhepufgtohhtthcunfhonhhguceoshgtohhtthhlsehsrghmshgt
ohdrohhrgheqnecuggftrfgrthhtvghrnhepfeevgfevveettdekhfduueduhefgieejtd
elgeehieeuuefgueekffejtdeftdehnecuffhomhgrihhnpehfrhgvvggsshgurdhorhhg
necuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepshgtoh
htthhlsehsrghmshgtohdrohhrgh
X-ME-Proxy: <xmx:KVQUYZKpFok1Ji_L46sHYf4_sMvfAqeDPFFHI-rvopzlgMfphFCLjA>
<xmx:KVQUYYIj2-UDMdguiqGFfwDMFCUKausy7wwpnnx12s2PcS32MJW49Q>
<xmx:KVQUYayG87hgTTqk8_KGKG41yTRKGtQDscKHemPz9Rofzehph9hSCg>
<xmx:KVQUYdGVbtDQedwnwPZi74lzDZ9cFvnmsZQT66w6dRDV3KaiIof2DA>
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 <scottl@samsco.org>
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 <loos.br@gmail.com>, Scott Long <scottl@freebsd.org>,
src-committers <src-committers@freebsd.org>,
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 <jhb@freebsd.org>
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-main@freebsd.org
X-Mailman-Version: 2.1.34
Precedence: list
List-Id: Commit messages for the main branch of the src repository
<dev-commits-src-main.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/dev-commits-src-main>,
<mailto:dev-commits-src-main-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/dev-commits-src-main/>
List-Post: <mailto:dev-commits-src-main@freebsd.org>
List-Help: <mailto:dev-commits-src-main-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/dev-commits-src-main>,
<mailto:dev-commits-src-main-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Wed, 11 Aug 2021 22:50:25 -0000
> On Aug 11, 2021, at 4:42 PM, John Baldwin <jhb@freebsd.org> 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 <jhb@freebsd.org> 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 <jhb@freebsd.org> 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 <scottl@FreeBSD.org>
>>>>>> AuthorDate: 2021-08-10 22:36:38 +0000
>>>>>> Commit: Scott Long <scottl@FreeBSD.org>
>>>>>> 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=