Date: Fri, 15 May 2015 09:55:07 -0400 From: John Baldwin <jhb@freebsd.org> To: src-committers@freebsd.org Cc: svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r282971 - in head/sys: kern sys Message-ID: <6021980.e9743X4Cvi@ralph.baldwin.cx> In-Reply-To: <201505151350.t4FDocQT054144@svn.freebsd.org> References: <201505151350.t4FDocQT054144@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, May 15, 2015 01:50:38 PM John Baldwin wrote: > Author: jhb > Date: Fri May 15 13:50:37 2015 > New Revision: 282971 > URL: https://svnweb.freebsd.org/changeset/base/282971 > > Log: > Previously, cv_waiters was only updated by cv_signal or cv_wait. If a > thread awakened due to a time out, then cv_waiters was not decremented. > If INT_MAX threads timed out on a cv without an intervening cv_broadcast, > then cv_waiters could overflow. To fix this, have each sleeping thread > decrement cv_waiters when it resumes. > > Note that previously cv_waiters was protected by the sleepq chain lock. > However, that lock is not held when threads resume from sleep. In > addition, the interlock is also not always reacquired after resuming > (cv_wait_unlock), nor is it always held by callers of cv_signal() or > cv_broadcast(). Instead, use atomic ops to update cv_waiters. Since > the sleepq chain lock is still held on every increment, it should > still be safe to compare cv_waiters against zero while holding the > lock in the wakeup routines as the only way the race should be lost > would result in extra calls to sleepq_signal() or sleepq_broadcast(). > > Differential Revision: https://reviews.freebsd.org/D2427 > Reviewed by: benno > Reported by: benno (wrap of cv_waiters in the field) > MFC after: 2 weeks With the additional overhead of the atomic ops it might be worth running some benchmarks to compare this with removing cv_waiters entirely. The theoretical gain from cv_waiters is avoiding looking in the hash table for a matching sleepqueue when there are no waiters. When cv_waiters was a simple integer the cost of having it around was very small, so even a tiny gain was worth having. (It is worth noting that in pre-SMPng code it was fairly common practice to keep a "WANTED" flag around that was set by waiters and would result in wakeup() being skipped if it wasn't set. These flags were maintained by each caller, not centrally. cv_waiters makes this sort of thing centrally maintained rather than something that each caller has to do.) Now that cv_waiters is updated with atomics, the cost is not quite as small. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6021980.e9743X4Cvi>