Date: Wed, 11 Aug 2021 16:50:15 -0600 From: Scott Long <scottl@samsco.org> To: John Baldwin <jhb@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 Subject: Re: git: 35547df5c786 - main - Call wakeup() with the lock held to avoid missed wakeup races. Message-ID: <8FB57A36-8D3B-4B7B-8397-6D742694DD75@samsco.org> In-Reply-To: <9587664b-5c82-cbe2-fb13-6de6abb9843d@FreeBSD.org> References: <9587664b-5c82-cbe2-fb13-6de6abb9843d@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> 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=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8FB57A36-8D3B-4B7B-8397-6D742694DD75>