From owner-dev-commits-src-main@freebsd.org Wed Aug 11 22:31:44 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 8326C65DC2F;
Wed, 11 Aug 2021 22:31:44 +0000 (UTC)
(envelope-from loos.br@gmail.com)
Received: from mail-qk1-x730.google.com (mail-qk1-x730.google.com
[IPv6:2607:f8b0:4864:20::730])
(using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)
key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256
client-signature RSA-PSS (2048 bits) client-digest SHA256)
(Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK))
by mx1.freebsd.org (Postfix) with ESMTPS id 4GlPgh32SYz4gW5;
Wed, 11 Aug 2021 22:31:44 +0000 (UTC)
(envelope-from loos.br@gmail.com)
Received: by mail-qk1-x730.google.com with SMTP id w197so4294693qkb.1;
Wed, 11 Aug 2021 15:31:44 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;
h=mime-version:references:in-reply-to:from:date:message-id:subject:to
:cc; bh=y3inyOdz+CZvfBR3bMYZj/d/IL7aDNBNpL8a76JFhwg=;
b=Fgid74aCtibd1WSDOsEyDxr1jI0xcHeRMC0z48RomzVU3Kkr1DqYLLSzB1WbRCcu40
8qC6Dqh51BH/fs9BKrjjH7zEvvFWmAN+N90hO2eHb/GaFamRpll018DT+1I54e8VGmkX
5ZQbidVmISDXhnwPfFmDrymxD1obsM6Gf/yfa6x6ZkVl4kelEzjnIKXA2SCJ8s1UFnUe
W0P25WWcwpQwqgCYG/WaVBck2BxQ0FeYu6ol5WSq5GNsKn0E1fM5hIAEWf6MWmePwXme
qshvRbIsv9clideHoi5Gm5xn5HDDzrBRQ4UzcLkAY0VBWhQcVTWpbRI32IJrWL4as9j4
8cwA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:mime-version:references:in-reply-to:from:date
:message-id:subject:to:cc;
bh=y3inyOdz+CZvfBR3bMYZj/d/IL7aDNBNpL8a76JFhwg=;
b=XeNI+BvKXrjtBGGDbUTwgibB+Xl3P1ZhNL44ypjdCTx3KndEpBprfdeVsnEwHCwuOY
Tf9qHSqAf5RJd+oxpFK+ctBd4MFdTLWbOJp898K1mVSwbuTAgwvYbrWv5BwVc9ScbwL2
OL4YLW8wCEMbyyCIcDmdkTyb45jgLb8/0CTRtfrj3ZqerY6CajBpl+DN8SyltxCLq+83
DbRZh23ioXsA1vnLGv8ni4apyIfCvmb9+CAdzP1z2LCMublw5l3UNTuHbIBTtAm5H52Z
WIxxpcdrHyvLDJiUYenqmMA8h1dgbtPW50VntdP9H9brzwQfP4kkGBGWX39JBtMtqgcx
iKZg==
X-Gm-Message-State: AOAM531QoIhOnuUgyq3VY115G9mco6xiKEAgu0Tt1awQYN4scZWQWB8L
YCvFJ9u6+jWxs7alf9PyOoSnUvMT9rRRKOPtIbnLbytBxFc=
X-Google-Smtp-Source: ABdhPJwA1NT9HU1JY1tpR7V04HKZ32axm8HqChz9Zx7M/oxwfqsluRbFQ2lseN6ofM/dYzer9zqDAhf9LPCYEDX7gMU=
X-Received: by 2002:a05:620a:49c:: with SMTP id
28mr1351290qkr.389.1628721103821;
Wed, 11 Aug 2021 15:31:43 -0700 (PDT)
MIME-Version: 1.0
References: <202108102241.17AMfr63025604@gitrepo.freebsd.org>
<9b1ec22c-1545-52ea-7bf9-a96140737711@FreeBSD.org>
<CAJ8CS7pT9A=9OTvwTkxUfHxxB9MjWUF7=cy=yA1ANzNO9tptww@mail.gmail.com>
<b68cf1db-bbc2-bfea-317a-11aa4f6daa78@FreeBSD.org>
In-Reply-To: <b68cf1db-bbc2-bfea-317a-11aa4f6daa78@FreeBSD.org>
From: Luiz Otavio O Souza <loos.br@gmail.com>
Date: Wed, 11 Aug 2021 19:31:32 -0300
Message-ID: <CAJ8CS7pQ=HWB9+y0H6sCECD4MUR1omnwu+g_xBoYnt1OW2NtLg@mail.gmail.com>
Subject: Re: git: 35547df5c786 - main - Call wakeup() with the lock held to
avoid missed wakeup races.
To: John Baldwin <jhb@freebsd.org>
Cc: Scott Long <scottl@freebsd.org>,
src-committers <src-committers@freebsd.org>,
dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org
Content-Type: text/plain; charset="UTF-8"
X-Rspamd-Queue-Id: 4GlPgh32SYz4gW5
X-Spamd-Bar: ----
Authentication-Results: mx1.freebsd.org;
none
X-Spamd-Result: default: False [-4.00 / 15.00]; TAGGED_FROM(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:31:44 -0000
On Wed, Aug 11, 2021 at 7:03 PM John Baldwin <jhb@freebsd.org> wrote:
>
> 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:
> >>
> >> 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 <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
> >>>
> >>> 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().
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.
luiz