From owner-svn-src-projects@FreeBSD.ORG Tue Sep 11 01:11:23 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 CCED81065674; Tue, 11 Sep 2012 01:11:23 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-vc0-f182.google.com (mail-vc0-f182.google.com [209.85.220.182]) by mx1.freebsd.org (Postfix) with ESMTP id 293388FC08; Tue, 11 Sep 2012 01:11:23 +0000 (UTC) Received: by mail-vc0-f182.google.com with SMTP id fw7so2919758vcb.13 for ; Mon, 10 Sep 2012 18:11:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=81qCLvzq8f06q5ZCV5lr7PLiclQcZL41yd1PjzZrQ3E=; b=Rpi9Jgx/91ocwXQ9cN+kmQvkA0FjnW4COzT0flOUmG8TvSfFt0IDmxI6sdd6Qs/x17 JY18M0RPugRoFvVq+3ghtVf1XJlkJ+0r+bxkzHV/qXRmzS2u1+5YUmchjze5P+MWbAMo ++V03rYnb+FpemJkkLoz5ka0Oth6ATsZpqgfKsI65+IBsNjFkuHLY21f378YZM/+9swN 6Iwlq4Ja8pUdCFUBtFBM65ljlUMiGwrKnSkugDzmNad8TSEN8tKAg2uuiNt4+NDCVwWs AkBzPolO2A23rBLQ8YI8SeN9QkuQVcih5CetammnPub6g/3U1EH05C0llxAH5cqw7Tqi Qjxg== MIME-Version: 1.0 Received: by 10.220.116.9 with SMTP id k9mr15465497vcq.0.1347325882923; Mon, 10 Sep 2012 18:11:22 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.220.106.6 with HTTP; Mon, 10 Sep 2012 18:11:22 -0700 (PDT) In-Reply-To: <504CF1FB.9090106@FreeBSD.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <504CEAE0.704@FreeBSD.org> <504CF1FB.9090106@FreeBSD.org> Date: Tue, 11 Sep 2012 02:11:22 +0100 X-Google-Sender-Auth: MlTRqk5DrPKS5p-eX6RcYk9f3BM Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 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 Reply-To: attilio@FreeBSD.org List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Sep 2012 01:11:24 -0000 On Sun, Sep 9, 2012 at 8:46 PM, John Baldwin wrote: > 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. Speaking of which, I think it is time for curthread != NULL checks in the locking primitives to go, or there is a good reason I really don't understand to keep them? Attilio -- Peace can only be achieved by understanding - A. Einstein