Date: Wed, 11 Aug 2021 18:38:55 -0300 From: Luiz Otavio O Souza <loos.br@gmail.com> 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 Subject: Re: git: 35547df5c786 - main - Call wakeup() with the lock held to avoid missed wakeup races. Message-ID: <CAJ8CS7pT9A=9OTvwTkxUfHxxB9MjWUF7=cy=yA1ANzNO9tptww@mail.gmail.com> In-Reply-To: <9b1ec22c-1545-52ea-7bf9-a96140737711@FreeBSD.org> References: <202108102241.17AMfr63025604@gitrepo.freebsd.org> <9b1ec22c-1545-52ea-7bf9-a96140737711@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. luiz
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ8CS7pT9A=9OTvwTkxUfHxxB9MjWUF7=cy=yA1ANzNO9tptww>