From owner-svn-src-projects@FreeBSD.ORG Sun Sep 9 19:46:04 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 990EB106566C; Sun, 9 Sep 2012 19:46:04 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 66C5D8FC0C; Sun, 9 Sep 2012 19:46:04 +0000 (UTC) Received: from John-Baldwins-MacBook-Air.local (c-68-39-198-164.hsd1.de.comcast.net [68.39.198.164]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 9B777B977; Sun, 9 Sep 2012 15:46:03 -0400 (EDT) Message-ID: <504CF1FB.9090106@FreeBSD.org> Date: Sun, 09 Sep 2012 15:46:03 -0400 From: John Baldwin User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20120824 Thunderbird/15.0 MIME-Version: 1.0 To: attilio@FreeBSD.org References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <504CEAE0.704@FreeBSD.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Sun, 09 Sep 2012 15:46:03 -0400 (EDT) Cc: Konstantin Belousov , Davide Italiano , src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 09 Sep 2012 19:46:04 -0000 On 9/9/12 3:23 PM, Attilio Rao wrote: > On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin wrote: >> On 9/9/12 11:03 AM, Attilio Rao wrote: >>> On 8/2/12, Attilio Rao wrote: >>>> On 7/30/12, John Baldwin wrote: >>> >>> [ trimm ] >>> >>>>> --- //depot/projects/smpng/sys/kern/subr_turnstile.c 2012-06-04 >>>>> 18:27:32.000000000 0000 >>>>> +++ //depot/user/jhb/lock/kern/subr_turnstile.c 2012-06-05 >>>>> 00:27:57.000000000 0000 >>>>> @@ -684,6 +684,7 @@ >>>>> if (owner) >>>>> MPASS(owner->td_proc->p_magic == P_MAGIC); >>>>> MPASS(queue == TS_SHARED_QUEUE || queue == TS_EXCLUSIVE_QUEUE); >>>>> + KASSERT(!TD_IS_IDLETHREAD(td), ("idle threads cannot block on locks")); >>>>> >>>>> /* >>>>> * If the lock does not already have a turnstile, use this thread's >>>> >>>> I'm wondering if we should also use similar checks in places doing >>>> adaptive spinning (including the TD_NO_SLEEPING check). Likely yes. >>> >>> So what do you think about this? >> >> This is isn't really good enough then. An idle thread should not >> acquire any lock that isn't a spin lock. Instead, you would be >> better off removing the assert I added above and adding an assert to >> mtx_lock(), rw_{rw}lock(), sx_{sx}lock(), lockmgr(), rm_{rw}lock() and >> all the try variants of those. > > While this is true, I thought about this route but I didn't want to go > for it because it would pollute much more code than the current > approach + patch I proposed, which would enough to find offending > cases. > I'm not sure I want to pollute all the kernel locking with checks for > idlethread, yet I think the current code is not complete and thus I > still think my patch is a reasonable compromise. I don't quite agree. We already pollute pretty much all of those with 'curthread != NULL' checks. This isn't all that different from just adding one of those. Also, just about all of those functions above do adaptive spinning and require a patch via your method, so it's really not that much more pollution to just do the full check. -- John Baldwin