On Thu, Dec 22, 2016 at 10:39:12AM -0800, John Baldwin wrote:
> On Thursday, December 22, 2016 05:51:44 PM Mark Johnston wrote:
> > Author: markj
> > Date: Thu Dec 22 17:51:44 2016
> > New Revision: 310423
> > URL: https://svnweb.freebsd.org/changeset/base/310423
> >
> > Log:
> > Revert part of r300109.
> >
> > The removal of TAILQ_FOREACH_SAFE introduced a small race: when the last
> > thread on a sleepqueue is awoken, it reclaims the sleepqueue and may begin
> > executing on a different CPU before sleepq_resume_thread() returns. This
> > leaves a window during which it may go back to sleep and incorrectly be
> > awoken again by the caller of sleepq_broadcast().
>
> This is very subtle.
:(
> The issue is that the last sleepq_resume_thread transfers
> ownership of 'sq' from the wait channel that the sleepq_broadcast has locked,
> to the thread being resumed.
Right, that's what I meant by "reclaims the sleepqueue." One other
requirement for hitting the race is that the thread goes back to sleep
on a wait channel that hashes to a different sleepchain, else the
sleepchain lock held by the sleepq_broadcast() caller is, I believe,
sufficient to prevent the reuse of the sleepqueue before the loop has
terminated.
> I thought about using a local TAILQ_HEAD and
> using TAILQ_CONCAT to move the list of threads out of the sleep queue and then
> walking that list. However, a comment explaining this transfer of ownership
> (and that we can't safely access 'sq' after the last thread is resumed) is
> probably sufficient (but necessary I think). Do you feel like adding one?
How about:
Index: subr_sleepqueue.c
===================================================================
--- subr_sleepqueue.c (revision 310423)
+++ subr_sleepqueue.c (working copy)
@@ -892,7 +892,12 @@
KASSERT(sq->sq_type == (flags & SLEEPQ_TYPE),
("%s: mismatch between sleep/wakeup and cv_*", __func__));
- /* Resume all blocked threads on the sleep queue. */
+ /*
+ * Resume all blocked threads on the sleep queue. The last thread will
+ * be given ownership of sq and may re-enqueue itself before
+ * sleepq_resume_thread() returns, so we must cache the "next" queue
+ * item at the beginning of the final iteration.
+ */
wakeup_swapper = 0;
TAILQ_FOREACH_SAFE(td, &sq->sq_blocked[queue], td_slpq, tdn) {
thread_lock(td);
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
