Skip site navigation (1)Skip section navigation (2)
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>