From owner-svn-src-all@freebsd.org Thu Dec 22 19:19:40 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 153D0C8DD90; Thu, 22 Dec 2016 19:19:40 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qt0-x243.google.com (mail-qt0-x243.google.com [IPv6:2607:f8b0:400d:c0d::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id C64DD1CD7; Thu, 22 Dec 2016 19:19:39 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-qt0-x243.google.com with SMTP id 3so4644657qtr.2; Thu, 22 Dec 2016 11:19:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=0d+Murcr8TY6GCYjcuNlhgXUE+4M1ScE8+ItgGwEi6U=; b=vNaa051gwlk8+L3et4o3FulWnQfGwVVuyiySzQRYuns6xKk35MGb8Biwtq6WpyqOBK t6opxNdvU11jaOutzhGdoXKr3xQ7gdStvFlHPTyxf/E11UZNFiKFtf8xx/qUd+piggkD h2EVVV3flDFDXCv5qJxvkpWl0FHV8UnLcdPR8bSePgEpUUIut2GoNA7Ip5qlhhdHiAXA ihf7/OzR9NjpHSfmD3+CsWO4CGR18XcaOsYpvoTfz7IirYB5y45HIF1g7dc/6eWB6q3N M1uTOc2YQHa3H1ou0rCGBKW5a5Fx7U7ys6VMZz23yUlQQjxs7cSzfWQ3eChJ44PsCvjY JDKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=0d+Murcr8TY6GCYjcuNlhgXUE+4M1ScE8+ItgGwEi6U=; b=CzkSPCdFMyRDeWUksH3M7L131swH+0JFjGnSteaHQROr9xiDnIdZrEVTMcOkDV+nKa jEztFhZwkEP+F8CCnaiV2MVY7f7bdKBa7mSimr/s+/94Ez/DuYxxyr1kMZH/Yb31YW2/ cd1k9k+G4tnplepINNuNKRo+563wFIWNHzi3uAIh4VW0UZZaUSs5XPrp7MBirPk/7Xzq BcKVMO4dp7q/GsBajgxJYdgnFQ9H+ZWaJ6aFqv+ctube+1Koj0IiRPf43x5UpHS5k663 22kGNmRwShMLDWCz4tb+7ly+hpSUg0WPQHCkk4LWCsNDqqj5Mm60R2ZwyKHzsLoXaBiH WDLQ== X-Gm-Message-State: AIkVDXKa4/PXZ111Gu/W7W7PgrhtgnWa/cfD0/5eRBfIBDRMIYlhmPCqQgJbURcTTpH6Ig== X-Received: by 10.200.41.9 with SMTP id y9mr12269673qty.26.1482434378787; Thu, 22 Dec 2016 11:19:38 -0800 (PST) Received: from wkstn-mjohnston.west.isilon.com (c-76-104-201-218.hsd1.wa.comcast.net. [76.104.201.218]) by smtp.gmail.com with ESMTPSA id y22sm18597334qtb.26.2016.12.22.11.19.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Dec 2016 11:19:38 -0800 (PST) Sender: Mark Johnston Date: Thu, 22 Dec 2016 11:26:01 -0800 From: Mark Johnston To: John Baldwin Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r310423 - head/sys/kern Message-ID: <20161222192601.GA78778@wkstn-mjohnston.west.isilon.com> References: <201612221751.uBMHpim4062786@repo.freebsd.org> <6562460.a4qdZuDa0s@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6562460.a4qdZuDa0s@ralph.baldwin.cx> User-Agent: Mutt/1.7.2 (2016-11-26) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Dec 2016 19:19:40 -0000 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);