From owner-svn-src-projects@FreeBSD.ORG Sun Sep 9 15:03:17 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B8E1C106566B; Sun, 9 Sep 2012 15:03:17 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 9750C8FC08; Sun, 9 Sep 2012 15:03:16 +0000 (UTC) Received: by lbbgg13 with SMTP id gg13so1103337lbb.13 for ; Sun, 09 Sep 2012 08:03:15 -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=7H7FOaca9Ru46USaNFLVzF2S9htKOf9wWfXqrUFR40w=; b=aELQUbHJ+3r28zNXxwezxMz3C+5aetwrIC3zVbtqnpMhSqLZjnFpRphGaOG2/BBcdY xD48pIyAJJ42TOwOvMOiHlvoZdN3arxmT6pQ0/2GSf9TgjvWQ6KuFyCHlAZHks7Cp040 6sXYYbFnOFkmy+stmQRoLJq0nyKy+1lCSCD3+Irdd4PQpbhYTNMO+xfXFNkzGk1GXWns mQpex3VexPcWguyBMEg8jmWCMtpzzPryefu+3yqX+I4AOcM3Itn7rXmrFwbo19iQzzYL m5bWFxoCaRVbQHmrWC+sI7UCNUIBNnz20HfvXfekjYiRUdnT/SNx6oIOmeXcXJe55tn0 cItw== MIME-Version: 1.0 Received: by 10.112.86.41 with SMTP id m9mr4035122lbz.108.1347202995200; Sun, 09 Sep 2012 08:03:15 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Sun, 9 Sep 2012 08:03:14 -0700 (PDT) In-Reply-To: References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> Date: Sun, 9 Sep 2012 16:03:14 +0100 X-Google-Sender-Auth: zJLgs-gyubSS6i04OEBpXDHJhUY 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: Sun, 09 Sep 2012 15:03:17 -0000 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? Thanks, Attilio In r239585 some further sanity checks to idlethreads not sleeping or blocking have been added. However, adaptive spinning on locks is not covered by this and it really should be, as it is operationally equivalent to blocking or sleeping on the primitive. --- sys/kern/kern_lock.c | 4 ++++ sys/kern/kern_mutex.c | 2 ++ sys/kern/kern_rwlock.c | 4 ++++ sys/kern/kern_sx.c | 4 ++++ 4 files changed, 14 insertions(+), 0 deletions(-) diff --git a/sys/kern/kern_lock.c b/sys/kern/kern_lock.c index 24526b0..a771daa 100644 --- a/sys/kern/kern_lock.c +++ b/sys/kern/kern_lock.c @@ -565,6 +565,8 @@ __lockmgr_args(struct lock *lk, u_int flags, struct lock_object *ilk, CTR3(KTR_LOCK, "%s: spinning on %p held by %p", __func__, lk, owner); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("idle threads cannot sleep on locks")); /* * If we are holding also an interlock drop it @@ -784,6 +786,8 @@ __lockmgr_args(struct lock *lk, u_int flags, struct lock_object *ilk, CTR3(KTR_LOCK, "%s: spinning on %p held by %p", __func__, lk, owner); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("idle threads cannot sleep on locks")); /* * If we are holding also an interlock drop it diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c index f718ca0..5a665c0 100644 --- a/sys/kern/kern_mutex.c +++ b/sys/kern/kern_mutex.c @@ -397,6 +397,8 @@ _mtx_lock_sleep(struct mtx *m, uintptr_t tid, int opts, const char *file, CTR3(KTR_LOCK, "%s: spinning on %p held by %p", __func__, m, owner); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("idle threads cannot block on locks")); while (mtx_owner(m) == owner && TD_IS_RUNNING(owner)) { cpu_spinwait(); diff --git a/sys/kern/kern_rwlock.c b/sys/kern/kern_rwlock.c index c337041..3ef14d6 100644 --- a/sys/kern/kern_rwlock.c +++ b/sys/kern/kern_rwlock.c @@ -391,6 +391,8 @@ _rw_rlock(struct rwlock *rw, const char *file, int line) CTR3(KTR_LOCK, "%s: spinning on %p held by %p", __func__, rw, owner); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("idle threads cannot block on locks")); while ((struct thread*)RW_OWNER(rw->rw_lock) == owner && TD_IS_RUNNING(owner)) { cpu_spinwait(); @@ -713,6 +715,8 @@ _rw_wlock_hard(struct rwlock *rw, uintptr_t tid, const char *file, int line) if (LOCK_LOG_TEST(&rw->lock_object, 0)) CTR3(KTR_LOCK, "%s: spinning on %p held by %p", __func__, rw, owner); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("idle threads cannot block on locks")); while ((struct thread*)RW_OWNER(rw->rw_lock) == owner && TD_IS_RUNNING(owner)) { cpu_spinwait(); diff --git a/sys/kern/kern_sx.c b/sys/kern/kern_sx.c index bcd7acd..176ff9c 100644 --- a/sys/kern/kern_sx.c +++ b/sys/kern/kern_sx.c @@ -551,6 +551,8 @@ _sx_xlock_hard(struct sx *sx, uintptr_t tid, int opts, const char *file, CTR3(KTR_LOCK, "%s: spinning on %p held by %p", __func__, sx, owner); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("idle threads cannot sleep on locks")); GIANT_SAVE(); while (SX_OWNER(sx->sx_lock) == x && TD_IS_RUNNING(owner)) { @@ -840,6 +842,8 @@ _sx_slock_hard(struct sx *sx, int opts, const char *file, int line) CTR3(KTR_LOCK, "%s: spinning on %p held by %p", __func__, sx, owner); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("idle threads cannot sleep on locks")); GIANT_SAVE(); while (SX_OWNER(sx->sx_lock) == x && TD_IS_RUNNING(owner)) { From owner-svn-src-projects@FreeBSD.ORG Sun Sep 9 19:09:48 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 7299F1065673; Sun, 9 Sep 2012 19:09:48 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 7FBD78FC0A; Sun, 9 Sep 2012 19:09:47 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q89J9ujl089962; Sun, 9 Sep 2012 22:09:56 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q89J9iIe098591; Sun, 9 Sep 2012 22:09:44 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q89J9iNw098590; Sun, 9 Sep 2012 22:09:44 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 9 Sep 2012 22:09:44 +0300 From: Konstantin Belousov To: Attilio Rao Message-ID: <20120909190944.GO33100@deviant.kiev.zoral.com.ua> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="boFd84b/DoLkbats" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.0 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: Davide Italiano , svn-src-projects@freebsd.org, src-committers@freebsd.org, John Baldwin 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:09:48 -0000 --boFd84b/DoLkbats Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Sep 09, 2012 at 04:03:14PM +0100, Attilio Rao wrote: > On 8/2/12, Attilio Rao wrote: > > On 7/30/12, John Baldwin wrote: >=20 > [ trimm ] >=20 > >> --- //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 =3D=3D P_MAGIC); > >> MPASS(queue =3D=3D TS_SHARED_QUEUE || queue =3D=3D TS_EXCLUSIVE_QUEU= E); > >> + 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. >=20 > So what do you think about this? My 2 cents are that is would be useful both to show the kind of lock (mutex/rw/sx/lockmgr etc) as well as the lock name. Ideally, the offending lock address would be also printed, because our kgdb dwarf parser does not work much more often then it works. --boFd84b/DoLkbats Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlBM6XcACgkQC3+MBN1Mb4im8ACfU5EM6WbGjSXx6EqZruJzKKqo ViUAn10ERi8PRJ50KMeZicIXVnkO/F1a =MmP2 -----END PGP SIGNATURE----- --boFd84b/DoLkbats-- From owner-svn-src-projects@FreeBSD.ORG Sun Sep 9 19:15:41 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BD952106564A; Sun, 9 Sep 2012 19:15:41 +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 894388FC15; Sun, 9 Sep 2012 19:15:41 +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 916E9B96F; Sun, 9 Sep 2012 15:15:40 -0400 (EDT) Message-ID: <504CEAE0.704@FreeBSD.org> Date: Sun, 09 Sep 2012 15:15:44 -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> 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:15:40 -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:15:41 -0000 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. -- John Baldwin From owner-svn-src-projects@FreeBSD.ORG Sun Sep 9 19:23:27 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 29E43106564A; Sun, 9 Sep 2012 19:23:27 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lpp01m010-f54.google.com (mail-lpp01m010-f54.google.com [209.85.215.54]) by mx1.freebsd.org (Postfix) with ESMTP id 0EFD28FC08; Sun, 9 Sep 2012 19:23:25 +0000 (UTC) Received: by lage12 with SMTP id e12so1060213lag.13 for ; Sun, 09 Sep 2012 12:23:19 -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=Q6nWUZEwqZICqzqtjXhfXPSQ+GZ66bNtzTUIaUlr7SM=; b=qnFfZwbN/+156BkJzC9UZjqHRJsbvFmWdxFba+NZI+vpBVBdvCwe8h+FPjQ0+Kkms/ a+AOPhAWMQPeg4WsbMUAZeGu49XiQVGgq4H4LPUCAUQd5eGSSZFEMLqo0kBXhD/X8ngk eLqL3R5L4TzhcC25KMGFlyIKAVZrVCCal3WJcnvAefvjNaG0PEvOOWO6d2GIgWuMXztW e/f0IDE2aGcq+xyDbQb4OYiTLuFsUT5rNDQncKKevkXV+3NOTAx7y8b/skldf6mrZDyt tkys6BV/JhMpJyiYD/MpZiUbK1hPsxOoeiBCAjAIBLiuIVMaHmlLnRIcVkndib6n5LEs ClIA== MIME-Version: 1.0 Received: by 10.112.103.71 with SMTP id fu7mr4000988lbb.21.1347218599070; Sun, 09 Sep 2012 12:23:19 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Sun, 9 Sep 2012 12:23:18 -0700 (PDT) In-Reply-To: <504CEAE0.704@FreeBSD.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <504CEAE0.704@FreeBSD.org> Date: Sun, 9 Sep 2012 20:23:18 +0100 X-Google-Sender-Auth: NbGfNhgDyTm3_b5Fg-E0wohsRN0 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: Sun, 09 Sep 2012 19:23:27 -0000 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. Attilio -- Peace can only be achieved by understanding - A. Einstein From owner-svn-src-projects@FreeBSD.ORG Sun Sep 9 19:34:27 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 2678A106566B; Sun, 9 Sep 2012 19:34:27 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 072A98FC08; Sun, 9 Sep 2012 19:34:25 +0000 (UTC) Received: by lbbgg13 with SMTP id gg13so1193317lbb.13 for ; Sun, 09 Sep 2012 12:34:24 -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=R64Aj2IEzpaY48Qkcz64j7LFBd74CGkUF4rIsQ5gkm8=; b=DQENqEezc4JTDkdStov28N4OmLdTdyh9U7u7AMghoOtZZok/Uk6aNj7UWKFvKNIz9v WcL5VfolxJxuOdjeqACo+aZ9dr11Nb1hxPWbwzI2rbh6QvXkWmbaVV2osGrH2+NgFUFo YRvV8QdaXmu9bzGc//E7oierc4oMOZBuxVu8quRV6oRYtokK4p0c7MOIRPmZpyiSJiH1 F2d6UHWqWeALNWxrdvKiqex1bYabA7UctQkrm9+V0rtrdU/lmnnNK60c8AK9N9Gw5ubS U3LM8wr+BPkUAGJREJo7JEzyZUoG9scrJd8T91PJ0xWnlpUpu5prUL2YsmY7PoFuIayo ltIw== MIME-Version: 1.0 Received: by 10.152.48.70 with SMTP id j6mr858878lan.57.1347219264766; Sun, 09 Sep 2012 12:34:24 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Sun, 9 Sep 2012 12:34:24 -0700 (PDT) In-Reply-To: <504CEAE0.704@FreeBSD.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <504CEAE0.704@FreeBSD.org> Date: Sun, 9 Sep 2012 20:34:24 +0100 X-Google-Sender-Auth: _mQY3cublmA8Elsbx_Usw9a-wgo 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: Sun, 09 Sep 2012 19:34:27 -0000 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. On a second thought, maybe this is not too bad, confining it in the hard version of functions eventually. Also, I think the checks may be good to go only on locking acquisition operations, not all of them. However, I would leave the check on sleepqueues as it also protects idlethreads from sleep(9). As a side node, also, I would change the output of the sleepqueues panic to also print the wmesg, as Kostik pointed out. Attilio -- Peace can only be achieved by understanding - A. Einstein 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 From owner-svn-src-projects@FreeBSD.ORG Mon Sep 10 02:07:21 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 82BE9106564A; Mon, 10 Sep 2012 02:07:21 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lpp01m010-f54.google.com (mail-lpp01m010-f54.google.com [209.85.215.54]) by mx1.freebsd.org (Postfix) with ESMTP id 5B9D68FC0A; Mon, 10 Sep 2012 02:07:20 +0000 (UTC) Received: by lage12 with SMTP id e12so1183113lag.13 for ; Sun, 09 Sep 2012 19:07:19 -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=OXl6eQlHPF32Ca/51Syf3ubjD0hhmDYw+E51UXHhoFM=; b=AMS2R3HGKhe6tqIhzepJ3ceIq9/HizEVUixFzgoNx96OAsz5gXZWEGfirBmkeKSRL9 iHkyw8tPcKnLFcK/Lu9Yn2z/XqUMAf7uwWf8Z+2odCPMbQPB5FmVvxZVB3/wH8snS1Wq c4d0Zy5XUK4O7pmXkDKcokM9YuswA7nOeevPeoDFWZ+etJzeGJ/m7Ux1Ii6PGuNSrZeC 1WQ7lhy3wi4g6nu5h8uLqhcC0ubIeuntcucILzRarLXS/F+08i3LtP4CDqHjbLwVeG8a Xk6hTXrkclDVkGMeiFWMFlAtTp+TadgIygFcAdRATcmw0x+GtgwyBvxU545l4ig7I7kD vLfQ== MIME-Version: 1.0 Received: by 10.152.112.233 with SMTP id it9mr10923302lab.40.1347242838935; Sun, 09 Sep 2012 19:07:18 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Sun, 9 Sep 2012 19:07:18 -0700 (PDT) In-Reply-To: <504CEAE0.704@FreeBSD.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <504CEAE0.704@FreeBSD.org> Date: Mon, 10 Sep 2012 03:07:18 +0100 X-Google-Sender-Auth: wdDKbxzBEl-1ThevBYq-h3MtiEI 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: Mon, 10 Sep 2012 02:07:21 -0000 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. What do you think about these then? Attilio Subject: [PATCH 1/3] Tweak the commit message in case of panic for sleeping from threads with TDP_NOSLEEPING on. The current message has no informations on the thread and wchan involed, which may prove to be useful in case where dumps have mangled dwarf informations. Reported by: kib --- sys/kern/subr_sleepqueue.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/sys/kern/subr_sleepqueue.c b/sys/kern/subr_sleepqueue.c index b868289..69e0d36 100644 --- a/sys/kern/subr_sleepqueue.c +++ b/sys/kern/subr_sleepqueue.c @@ -297,7 +297,8 @@ sleepq_add(void *wchan, struct lock_object *lock, const char *wmesg, int flags, /* If this thread is not allowed to sleep, die a horrible death. */ KASSERT(!(td->td_pflags & TDP_NOSLEEPING), - ("Trying sleep, but thread marked as sleeping prohibited")); + ("sleepq_add: td(%p) to sleep on wchan(%p) with TDP_NOSLEEPING on", + td, wchan)); /* Look up the sleep queue associated with the wait channel 'wchan'. */ sq = sleepq_lookup(wchan); -- 1.7.7.4 Subject: [PATCH 2/3] Tweak comments. - Remove a sporious "with" - Remove a comment which is no-longer true --- sys/kern/subr_trap.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/sys/kern/subr_trap.c b/sys/kern/subr_trap.c index 24960fd..13d273d1 100644 --- a/sys/kern/subr_trap.c +++ b/sys/kern/subr_trap.c @@ -153,7 +153,7 @@ userret(struct thread *td, struct trapframe *frame) KASSERT((td->td_pflags & TDP_NOSLEEPING) == 0, ("userret: Returning with sleep disabled")); KASSERT(td->td_pinned == 0, - ("userret: Returning with with pinned thread")); + ("userret: Returning with pinned thread")); #ifdef VIMAGE /* Unfortunately td_vnet_lpush needs VNET_DEBUG. */ VNET_ASSERT(curvnet == NULL, @@ -166,7 +166,6 @@ userret(struct thread *td, struct trapframe *frame) /* * Process an asynchronous software trap. * This is relatively easy. - * This function will return with preemption disabled. */ void ast(struct trapframe *framep) -- 1.7.7.4 Subject: [PATCH 3/3] Improve check coverage about idle threads. Idle threads are not allowed to acquire any lock but spinlocks. Deny any attempt to do so by panicing at the locking operation when INVARIANTS is on. Discussed with: jhb Signed-off-by: Attilio Rao --- sys/kern/kern_lock.c | 4 ++++ sys/kern/kern_mutex.c | 6 ++++++ sys/kern/kern_rmlock.c | 6 ++++++ sys/kern/kern_rwlock.c | 13 +++++++++++++ sys/kern/kern_sx.c | 13 +++++++++++++ sys/kern/subr_turnstile.c | 1 - 6 files changed, 42 insertions(+), 1 deletions(-) diff --git a/sys/kern/kern_lock.c b/sys/kern/kern_lock.c index 24526b0..46a7567 100644 --- a/sys/kern/kern_lock.c +++ b/sys/kern/kern_lock.c @@ -478,6 +478,10 @@ __lockmgr_args(struct lock *lk, u_int flags, struct lock_object *ilk, ("%s: LK_INTERLOCK passed without valid interlock @ %s:%d", __func__, file, line)); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("lockmgr() by idle thread %p on lockmgr %s @ %s:%d", + curthread, lk->lock_object.lo_name, file, line)); + class = (flags & LK_INTERLOCK) ? LOCK_CLASS(ilk) : NULL; if (panicstr != NULL) { if (flags & LK_INTERLOCK) diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c index f718ca0..9827a9f 100644 --- a/sys/kern/kern_mutex.c +++ b/sys/kern/kern_mutex.c @@ -201,6 +201,9 @@ _mtx_lock_flags(struct mtx *m, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return; MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("mtx_lock() by idle thread %p on sleep mutex %s @ %s:%d", + curthread, m->lock_object.lo_name, file, line)); KASSERT(m->mtx_lock != MTX_DESTROYED, ("mtx_lock() of destroyed mutex @ %s:%d", file, line)); KASSERT(LOCK_CLASS(&m->lock_object) == &lock_class_mtx_sleep, @@ -301,6 +304,9 @@ mtx_trylock_flags_(struct mtx *m, int opts, const char *file, int line) return (1); MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("mtx_trylock() by idle thread %p on sleep mutex %s @ %s:%d", + curthread, m->lock_object.lo_name, file, line)); KASSERT(m->mtx_lock != MTX_DESTROYED, ("mtx_trylock() of destroyed mutex @ %s:%d", file, line)); KASSERT(LOCK_CLASS(&m->lock_object) == &lock_class_mtx_sleep, diff --git a/sys/kern/kern_rmlock.c b/sys/kern/kern_rmlock.c index 27d0462..ef1920b 100644 --- a/sys/kern/kern_rmlock.c +++ b/sys/kern/kern_rmlock.c @@ -498,6 +498,9 @@ void _rm_wlock_debug(struct rmlock *rm, const char *file, int line) if (SCHEDULER_STOPPED()) return; + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rm_wlock() by idle thread %p on rmlock %s @ %s:%d", + curthread, rm->lock_object.lo_name, file, line)); WITNESS_CHECKORDER(&rm->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE, file, line, NULL); @@ -540,6 +543,9 @@ _rm_rlock_debug(struct rmlock *rm, struct rm_priotracker *tracker, if (SCHEDULER_STOPPED()) return (1); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rm_rlock() by idle thread %p on rmlock %s @ %s:%d", + curthread, rm->lock_object.lo_name, file, line)); if (!trylock && (rm->lock_object.lo_flags & RM_SLEEPABLE)) WITNESS_CHECKORDER(&rm->rm_lock_sx.lock_object, LOP_NEWORDER, file, line, NULL); diff --git a/sys/kern/kern_rwlock.c b/sys/kern/kern_rwlock.c index c337041..e0be154 100644 --- a/sys/kern/kern_rwlock.c +++ b/sys/kern/kern_rwlock.c @@ -242,6 +242,9 @@ _rw_wlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return; MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rw_wlock() by idle thread %p on rwlock %s @ %s:%d", + curthread, rw->lock_object.lo_name, file, line)); KASSERT(rw->rw_lock != RW_DESTROYED, ("rw_wlock() of destroyed rwlock @ %s:%d", file, line)); WITNESS_CHECKORDER(&rw->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE, file, @@ -260,6 +263,9 @@ _rw_try_wlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return (1); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rw_try_wlock() by idle thread %p on rwlock %s @ %s:%d", + curthread, rw->lock_object.lo_name, file, line)); KASSERT(rw->rw_lock != RW_DESTROYED, ("rw_try_wlock() of destroyed rwlock @ %s:%d", file, line)); @@ -333,6 +339,9 @@ _rw_rlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return; + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rw_rlock() by idle thread %p on rwlock %s @ %s:%d", + curthread, rw->lock_object.lo_name, file, line)); KASSERT(rw->rw_lock != RW_DESTROYED, ("rw_rlock() of destroyed rwlock @ %s:%d", file, line)); KASSERT(rw_wowner(rw) != curthread, @@ -521,6 +530,10 @@ _rw_try_rlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return (1); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rw_try_rlock() by idle thread %p on rwlock %s @ %s:%d", + curthread, rw->lock_object.lo_name, file, line)); + for (;;) { x = rw->rw_lock; KASSERT(rw->rw_lock != RW_DESTROYED, diff --git a/sys/kern/kern_sx.c b/sys/kern/kern_sx.c index bcd7acd..487a324 100644 --- a/sys/kern/kern_sx.c +++ b/sys/kern/kern_sx.c @@ -250,6 +250,9 @@ _sx_slock(struct sx *sx, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return (0); MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("sx_slock() by idle thread %p on sx %s @ %s:%d", + curthread, sx->lock_object.lo_name, file, line)); KASSERT(sx->sx_lock != SX_LOCK_DESTROYED, ("sx_slock() of destroyed sx @ %s:%d", file, line)); WITNESS_CHECKORDER(&sx->lock_object, LOP_NEWORDER, file, line, NULL); @@ -271,6 +274,10 @@ sx_try_slock_(struct sx *sx, const char *file, int line) if (SCHEDULER_STOPPED()) return (1); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("sx_try_slock() by idle thread %p on sx %s @ %s:%d", + curthread, sx->lock_object.lo_name, file, line)); + for (;;) { x = sx->sx_lock; KASSERT(x != SX_LOCK_DESTROYED, @@ -297,6 +304,9 @@ _sx_xlock(struct sx *sx, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return (0); MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("sx_xlock() by idle thread %p on sx %s @ %s:%d", + curthread, sx->lock_object.lo_name, file, line)); KASSERT(sx->sx_lock != SX_LOCK_DESTROYED, ("sx_xlock() of destroyed sx @ %s:%d", file, line)); WITNESS_CHECKORDER(&sx->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE, file, @@ -321,6 +331,9 @@ sx_try_xlock_(struct sx *sx, const char *file, int line) return (1); MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("sx_try_xlock() by idle thread %p on sx %s @ %s:%d", + curthread, sx->lock_object.lo_name, file, line)); KASSERT(sx->sx_lock != SX_LOCK_DESTROYED, ("sx_try_xlock() of destroyed sx @ %s:%d", file, line)); diff --git a/sys/kern/subr_turnstile.c b/sys/kern/subr_turnstile.c index 31d16fe..76fb964 100644 --- a/sys/kern/subr_turnstile.c +++ b/sys/kern/subr_turnstile.c @@ -684,7 +684,6 @@ turnstile_wait(struct turnstile *ts, struct thread *owner, int queue) 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 -- 1.7.7.4 From owner-svn-src-projects@FreeBSD.ORG Mon Sep 10 09:34:53 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id F2EBC1065670; Mon, 10 Sep 2012 09:34:52 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id A97CD8FC0A; Mon, 10 Sep 2012 09:34:51 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q8A9Yukl014946; Mon, 10 Sep 2012 12:34:56 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q8A9YhJ2007114; Mon, 10 Sep 2012 12:34:43 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q8A9YhqS007113; Mon, 10 Sep 2012 12:34:43 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Mon, 10 Sep 2012 12:34:43 +0300 From: Konstantin Belousov To: Attilio Rao Message-ID: <20120910093443.GT33100@deviant.kiev.zoral.com.ua> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <504CEAE0.704@FreeBSD.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="1sp61FhDAWPvkXjV" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.0 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: Davide Italiano , svn-src-projects@freebsd.org, src-committers@freebsd.org, John Baldwin 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: Mon, 10 Sep 2012 09:34:53 -0000 --1sp61FhDAWPvkXjV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 10, 2012 at 03:07:18AM +0100, 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 =3D=3D P_MAGIC); > >>>> MPASS(queue =3D=3D TS_SHARED_QUEUE || queue =3D=3D TS_EXCLUSIVE_= QUEUE); > >>>> + KASSERT(!TD_IS_IDLETHREAD(td), ("idle threads cannot block on lo= cks")); > >>>> > >>>> /* > >>>> * If the lock does not already have a turnstile, use this threa= d'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. >=20 > What do you think about these then? For me, it is definitely an improvement for the usefulness of the messages. >=20 > Attilio >=20 >=20 > Subject: [PATCH 1/3] Tweak the commit message in case of panic for sleepi= ng > from threads with TDP_NOSLEEPING on. >=20 > The current message has no informations on the thread and wchan > involed, which may prove to be useful in case where dumps have > mangled dwarf informations. >=20 > Reported by: kib > --- > sys/kern/subr_sleepqueue.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) >=20 > diff --git a/sys/kern/subr_sleepqueue.c b/sys/kern/subr_sleepqueue.c > index b868289..69e0d36 100644 > --- a/sys/kern/subr_sleepqueue.c > +++ b/sys/kern/subr_sleepqueue.c > @@ -297,7 +297,8 @@ sleepq_add(void *wchan, struct lock_object *lock, > const char *wmesg, int flags, >=20 > /* If this thread is not allowed to sleep, die a horrible death. = */ > KASSERT(!(td->td_pflags & TDP_NOSLEEPING), > - ("Trying sleep, but thread marked as sleeping prohibited")); > + ("sleepq_add: td(%p) to sleep on wchan(%p) with TDP_NOSLEEPIN= G on", > + td, wchan)); >=20 > /* Look up the sleep queue associated with the wait channel 'wcha= n'. */ > sq =3D sleepq_lookup(wchan); > --=20 > 1.7.7.4 >=20 > Subject: [PATCH 2/3] Tweak comments. >=20 > - Remove a sporious "with" > - Remove a comment which is no-longer true > --- > sys/kern/subr_trap.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) >=20 > diff --git a/sys/kern/subr_trap.c b/sys/kern/subr_trap.c > index 24960fd..13d273d1 100644 > --- a/sys/kern/subr_trap.c > +++ b/sys/kern/subr_trap.c > @@ -153,7 +153,7 @@ userret(struct thread *td, struct trapframe *frame) > KASSERT((td->td_pflags & TDP_NOSLEEPING) =3D=3D 0, > ("userret: Returning with sleep disabled")); > KASSERT(td->td_pinned =3D=3D 0, > - ("userret: Returning with with pinned thread")); > + ("userret: Returning with pinned thread")); > #ifdef VIMAGE > /* Unfortunately td_vnet_lpush needs VNET_DEBUG. */ > VNET_ASSERT(curvnet =3D=3D NULL, > @@ -166,7 +166,6 @@ userret(struct thread *td, struct trapframe *frame) > /* > * Process an asynchronous software trap. > * This is relatively easy. > - * This function will return with preemption disabled. > */ > void > ast(struct trapframe *framep) > --=20 > 1.7.7.4 >=20 > Subject: [PATCH 3/3] Improve check coverage about idle threads. >=20 > Idle threads are not allowed to acquire any lock but spinlocks. > Deny any attempt to do so by panicing at the locking operation > when INVARIANTS is on. >=20 > Discussed with: jhb >=20 > Signed-off-by: Attilio Rao > --- > sys/kern/kern_lock.c | 4 ++++ > sys/kern/kern_mutex.c | 6 ++++++ > sys/kern/kern_rmlock.c | 6 ++++++ > sys/kern/kern_rwlock.c | 13 +++++++++++++ > sys/kern/kern_sx.c | 13 +++++++++++++ > sys/kern/subr_turnstile.c | 1 - > 6 files changed, 42 insertions(+), 1 deletions(-) >=20 > diff --git a/sys/kern/kern_lock.c b/sys/kern/kern_lock.c > index 24526b0..46a7567 100644 > --- a/sys/kern/kern_lock.c > +++ b/sys/kern/kern_lock.c > @@ -478,6 +478,10 @@ __lockmgr_args(struct lock *lk, u_int flags, > struct lock_object *ilk, > ("%s: LK_INTERLOCK passed without valid interlock @ %s:%d", > __func__, file, line)); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("lockmgr() by idle thread %p on lockmgr %s @ %s:%d", > + curthread, lk->lock_object.lo_name, file, line)); > + > class =3D (flags & LK_INTERLOCK) ? LOCK_CLASS(ilk) : NULL; > if (panicstr !=3D NULL) { > if (flags & LK_INTERLOCK) > diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c > index f718ca0..9827a9f 100644 > --- a/sys/kern/kern_mutex.c > +++ b/sys/kern/kern_mutex.c > @@ -201,6 +201,9 @@ _mtx_lock_flags(struct mtx *m, int opts, const > char *file, int line) > if (SCHEDULER_STOPPED()) > return; > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("mtx_lock() by idle thread %p on sleep mutex %s @ %s:%d", > + curthread, m->lock_object.lo_name, file, line)); > KASSERT(m->mtx_lock !=3D MTX_DESTROYED, > ("mtx_lock() of destroyed mutex @ %s:%d", file, line)); > KASSERT(LOCK_CLASS(&m->lock_object) =3D=3D &lock_class_mtx_sleep, > @@ -301,6 +304,9 @@ mtx_trylock_flags_(struct mtx *m, int opts, const > char *file, int line) > return (1); >=20 > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("mtx_trylock() by idle thread %p on sleep mutex %s @ %s:%d", > + curthread, m->lock_object.lo_name, file, line)); > KASSERT(m->mtx_lock !=3D MTX_DESTROYED, > ("mtx_trylock() of destroyed mutex @ %s:%d", file, line)); > KASSERT(LOCK_CLASS(&m->lock_object) =3D=3D &lock_class_mtx_sleep, > diff --git a/sys/kern/kern_rmlock.c b/sys/kern/kern_rmlock.c > index 27d0462..ef1920b 100644 > --- a/sys/kern/kern_rmlock.c > +++ b/sys/kern/kern_rmlock.c > @@ -498,6 +498,9 @@ void _rm_wlock_debug(struct rmlock *rm, const char > *file, int line) > if (SCHEDULER_STOPPED()) > return; >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rm_wlock() by idle thread %p on rmlock %s @ %s:%d", > + curthread, rm->lock_object.lo_name, file, line)); > WITNESS_CHECKORDER(&rm->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE, > file, line, NULL); >=20 > @@ -540,6 +543,9 @@ _rm_rlock_debug(struct rmlock *rm, struct > rm_priotracker *tracker, > if (SCHEDULER_STOPPED()) > return (1); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rm_rlock() by idle thread %p on rmlock %s @ %s:%d", > + curthread, rm->lock_object.lo_name, file, line)); > if (!trylock && (rm->lock_object.lo_flags & RM_SLEEPABLE)) > WITNESS_CHECKORDER(&rm->rm_lock_sx.lock_object, LOP_NEWOR= DER, > file, line, NULL); > diff --git a/sys/kern/kern_rwlock.c b/sys/kern/kern_rwlock.c > index c337041..e0be154 100644 > --- a/sys/kern/kern_rwlock.c > +++ b/sys/kern/kern_rwlock.c > @@ -242,6 +242,9 @@ _rw_wlock(struct rwlock *rw, const char *file, int li= ne) > if (SCHEDULER_STOPPED()) > return; > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rw_wlock() by idle thread %p on rwlock %s @ %s:%d", > + curthread, rw->lock_object.lo_name, file, line)); > KASSERT(rw->rw_lock !=3D RW_DESTROYED, > ("rw_wlock() of destroyed rwlock @ %s:%d", file, line)); > WITNESS_CHECKORDER(&rw->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE= , file, > @@ -260,6 +263,9 @@ _rw_try_wlock(struct rwlock *rw, const char *file, in= t line) > if (SCHEDULER_STOPPED()) > return (1); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rw_try_wlock() by idle thread %p on rwlock %s @ %s:%d", > + curthread, rw->lock_object.lo_name, file, line)); > KASSERT(rw->rw_lock !=3D RW_DESTROYED, > ("rw_try_wlock() of destroyed rwlock @ %s:%d", file, line)); >=20 > @@ -333,6 +339,9 @@ _rw_rlock(struct rwlock *rw, const char *file, int li= ne) > if (SCHEDULER_STOPPED()) > return; >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rw_rlock() by idle thread %p on rwlock %s @ %s:%d", > + curthread, rw->lock_object.lo_name, file, line)); > KASSERT(rw->rw_lock !=3D RW_DESTROYED, > ("rw_rlock() of destroyed rwlock @ %s:%d", file, line)); > KASSERT(rw_wowner(rw) !=3D curthread, > @@ -521,6 +530,10 @@ _rw_try_rlock(struct rwlock *rw, const char > *file, int line) > if (SCHEDULER_STOPPED()) > return (1); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rw_try_rlock() by idle thread %p on rwlock %s @ %s:%d", > + curthread, rw->lock_object.lo_name, file, line)); > + > for (;;) { > x =3D rw->rw_lock; > KASSERT(rw->rw_lock !=3D RW_DESTROYED, > diff --git a/sys/kern/kern_sx.c b/sys/kern/kern_sx.c > index bcd7acd..487a324 100644 > --- a/sys/kern/kern_sx.c > +++ b/sys/kern/kern_sx.c > @@ -250,6 +250,9 @@ _sx_slock(struct sx *sx, int opts, const char > *file, int line) > if (SCHEDULER_STOPPED()) > return (0); > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("sx_slock() by idle thread %p on sx %s @ %s:%d", > + curthread, sx->lock_object.lo_name, file, line)); > KASSERT(sx->sx_lock !=3D SX_LOCK_DESTROYED, > ("sx_slock() of destroyed sx @ %s:%d", file, line)); > WITNESS_CHECKORDER(&sx->lock_object, LOP_NEWORDER, file, line, NU= LL); > @@ -271,6 +274,10 @@ sx_try_slock_(struct sx *sx, const char *file, int l= ine) > if (SCHEDULER_STOPPED()) > return (1); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("sx_try_slock() by idle thread %p on sx %s @ %s:%d", > + curthread, sx->lock_object.lo_name, file, line)); > + > for (;;) { > x =3D sx->sx_lock; > KASSERT(x !=3D SX_LOCK_DESTROYED, > @@ -297,6 +304,9 @@ _sx_xlock(struct sx *sx, int opts, const char > *file, int line) > if (SCHEDULER_STOPPED()) > return (0); > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("sx_xlock() by idle thread %p on sx %s @ %s:%d", > + curthread, sx->lock_object.lo_name, file, line)); > KASSERT(sx->sx_lock !=3D SX_LOCK_DESTROYED, > ("sx_xlock() of destroyed sx @ %s:%d", file, line)); > WITNESS_CHECKORDER(&sx->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE= , file, > @@ -321,6 +331,9 @@ sx_try_xlock_(struct sx *sx, const char *file, int li= ne) > return (1); >=20 > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("sx_try_xlock() by idle thread %p on sx %s @ %s:%d", > + curthread, sx->lock_object.lo_name, file, line)); > KASSERT(sx->sx_lock !=3D SX_LOCK_DESTROYED, > ("sx_try_xlock() of destroyed sx @ %s:%d", file, line)); >=20 > diff --git a/sys/kern/subr_turnstile.c b/sys/kern/subr_turnstile.c > index 31d16fe..76fb964 100644 > --- a/sys/kern/subr_turnstile.c > +++ b/sys/kern/subr_turnstile.c > @@ -684,7 +684,6 @@ turnstile_wait(struct turnstile *ts, struct thread > *owner, int queue) > if (owner) > MPASS(owner->td_proc->p_magic =3D=3D P_MAGIC); > MPASS(queue =3D=3D TS_SHARED_QUEUE || queue =3D=3D TS_EXCLUSIVE_Q= UEUE); > - KASSERT(!TD_IS_IDLETHREAD(td), ("idle threads cannot block on loc= ks")); >=20 > /* > * If the lock does not already have a turnstile, use this thread= 's > --=20 > 1.7.7.4 --1sp61FhDAWPvkXjV Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlBNtDMACgkQC3+MBN1Mb4gOugCg0tJCsDgcB1e687rhl2a2IdEA X2oAn3l/g9rAfUTZ2xVrrrAcLumGlOly =ZaJ2 -----END PGP SIGNATURE----- --1sp61FhDAWPvkXjV-- From owner-svn-src-projects@FreeBSD.ORG Mon Sep 10 18:43:33 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 4DA681065675; Mon, 10 Sep 2012 18:43:33 +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 1FCF68FC14; Mon, 10 Sep 2012 18:43:33 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 75FA3B990; Mon, 10 Sep 2012 14:43:32 -0400 (EDT) From: John Baldwin To: attilio@freebsd.org Date: Mon, 10 Sep 2012 14:43:31 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p17; KDE/4.5.5; amd64; ; ) References: <201207301350.q6UDobCI099069@svn.freebsd.org> <504CEAE0.704@FreeBSD.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201209101443.31207.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 10 Sep 2012 14:43:32 -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: Mon, 10 Sep 2012 18:43:33 -0000 On Sunday, September 09, 2012 10:07:18 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. > > What do you think about these then? These look good, thanks! -- John Baldwin 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 From owner-svn-src-projects@FreeBSD.ORG Tue Sep 11 07:31:16 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 B68401065674; Tue, 11 Sep 2012 07:31:16 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 6BE7F8FC0A; Tue, 11 Sep 2012 07:31:14 +0000 (UTC) Received: from porto.starpoint.kiev.ua (porto-e.starpoint.kiev.ua [212.40.38.100]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id KAA19449; Tue, 11 Sep 2012 10:31:13 +0300 (EEST) (envelope-from avg@FreeBSD.org) Received: from localhost ([127.0.0.1]) by porto.starpoint.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1TBKwD-00025d-8b; Tue, 11 Sep 2012 10:31:13 +0300 Message-ID: <504EE8C0.9000203@FreeBSD.org> Date: Tue, 11 Sep 2012 10:31:12 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:15.0) Gecko/20120901 Thunderbird/15.0 MIME-Version: 1.0 To: attilio@FreeBSD.org References: <201207301350.q6UDobCI099069@svn.freebsd.org> <20120730143943.GY2676@deviant.kiev.zoral.com.ua> <5016A21B.6090409@FreeBSD.org> <5016A8E4.7070405@FreeBSD.org> In-Reply-To: X-Enigmail-Version: 1.4.3 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Konstantin Belousov , Davide Italiano , svn-src-projects@FreeBSD.org, src-committers@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: Tue, 11 Sep 2012 07:31:16 -0000 on 08/09/2012 01:35 Attilio Rao said the following: > More specifically, what do you think about this patch?: > http://www.freebsd.org/~attilio/userret_diag.patch > > Of course I moved the XEN par too before the checks. > The patch survived to few consecutive and parallel buildworlds, FWIW. Sorry for the late reply. The patch looks very good. I haven't been following commit emails for some days - perhaps you've already committed this? :) Thank you. -- Andriy Gapon From owner-svn-src-projects@FreeBSD.ORG Wed Sep 12 20:33:35 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 6A482106564A; Wed, 12 Sep 2012 20:33:35 +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 EAD728FC14; Wed, 12 Sep 2012 20:33:34 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 5F354B911; Wed, 12 Sep 2012 16:33:31 -0400 (EDT) From: John Baldwin To: attilio@freebsd.org Date: Wed, 12 Sep 2012 15:11:42 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p17; KDE/4.5.5; amd64; ; ) References: <201207301350.q6UDobCI099069@svn.freebsd.org> <504CF1FB.9090106@FreeBSD.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201209121511.42296.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 12 Sep 2012 16:33:31 -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: Wed, 12 Sep 2012 20:33:35 -0000 On Monday, September 10, 2012 9:11:22 pm Attilio Rao wrote: > 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? They can probably be axed. -- John Baldwin From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 00:07:13 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 681CC106566B; Thu, 13 Sep 2012 00:07:13 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-vb0-f54.google.com (mail-vb0-f54.google.com [209.85.212.54]) by mx1.freebsd.org (Postfix) with ESMTP id D28578FC0A; Thu, 13 Sep 2012 00:07:12 +0000 (UTC) Received: by vbmv11 with SMTP id v11so3608953vbm.13 for ; Wed, 12 Sep 2012 17:07:11 -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=xmbjbGiF1xDVCEk8F+x2fvfcxI0Z6y/ws7eOZRBk+84=; b=BKwO5mrM+zsA8g392JprbX9Tfzkg3eG1D1ThiahnPm0R2OV4CieTHqLxy6ZyiBhNP1 pRtFb5M+pFl/VQApFn79Q9/QbndcPRa2Poi73Sq7Wf2MSHGswYCWk2yq/P6gy0ToXxRe vbSb6llAAjz5Geidw8xqc5jQbg2aOIM0NXedgcQL3yjR2dfRe/LS92F6xiGdKm7aygJH 0wfw20xmzTk7pkyuD+JyOgwii70flLw4MwMj65qTi2Ipe77b9vBLJy1pCrUDyu2pPuiy WSyJEvgdBJNHxIhwBZhs/qG0D4pRftQ/Xkv8pELH2Ikz6+SwiOmhgmnThOQX783utaTa TJQA== MIME-Version: 1.0 Received: by 10.52.26.137 with SMTP id l9mr99727vdg.62.1347494831642; Wed, 12 Sep 2012 17:07:11 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.220.106.6 with HTTP; Wed, 12 Sep 2012 17:07:11 -0700 (PDT) In-Reply-To: <201209121511.42296.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <504CF1FB.9090106@FreeBSD.org> <201209121511.42296.jhb@freebsd.org> Date: Thu, 13 Sep 2012 01:07:11 +0100 X-Google-Sender-Auth: sd0s38_-7pamlOMnrfqgngWRC34 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: Thu, 13 Sep 2012 00:07:13 -0000 On Wed, Sep 12, 2012 at 8:11 PM, John Baldwin wrote: > On Monday, September 10, 2012 9:11:22 pm Attilio Rao wrote: >> 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? > > They can probably be axed. What do you think about this? Please note that I would also axe the check in printtrap() on several arches, but maybe there is a valid reason to keep it I'm not thinking right now, so I left it out in this patch. Thanks, Attilio Subject: [PATCH 6/6] Remove all the checks on curthread != NULL with the exception of MD in printtrap() functions on several !x86 arches. Generally this check is not needed anymore, as there is not a legitimate case where curthread != NULL. Discussed with: bde, jhb --- sys/dev/hwpmc/hwpmc_arm.c | 6 ++---- sys/dev/hwpmc/hwpmc_x86.c | 3 +-- sys/kern/kern_condvar.c | 2 +- sys/kern/kern_mutex.c | 5 ----- sys/kern/kern_rwlock.c | 2 -- sys/kern/kern_sx.c | 5 ----- sys/kern/kern_thread.c | 1 - sys/kern/vfs_subr.c | 1 - 8 files changed, 4 insertions(+), 21 deletions(-) diff --git a/sys/dev/hwpmc/hwpmc_arm.c b/sys/dev/hwpmc/hwpmc_arm.c index c2c24c2..79613f7 100644 --- a/sys/dev/hwpmc/hwpmc_arm.c +++ b/sys/dev/hwpmc/hwpmc_arm.c @@ -78,8 +78,7 @@ pmc_save_kernel_callchain(uintptr_t *cc, int maxsamples, pc = PMC_TRAPFRAME_TO_PC(tf); *cc++ = pc; - if ((td = curthread) == NULL) - return (1); + td = curthread; if (maxsamples <= 1) return (1); @@ -129,8 +128,7 @@ pmc_save_user_callchain(uintptr_t *cc, int maxsamples, pc = PMC_TRAPFRAME_TO_PC(tf); *cc++ = pc; - if ((td = curthread) == NULL) - return (1); + td = curthread; if (maxsamples <= 1) return (1); diff --git a/sys/dev/hwpmc/hwpmc_x86.c b/sys/dev/hwpmc/hwpmc_x86.c index e7485a4..cd9e4f6 100644 --- a/sys/dev/hwpmc/hwpmc_x86.c +++ b/sys/dev/hwpmc/hwpmc_x86.c @@ -168,8 +168,7 @@ pmc_save_kernel_callchain(uintptr_t *cc, int nframes, struct trapframe *tf) *cc++ = pc; r = fp + sizeof(uintptr_t); /* points to return address */ - if ((td = curthread) == NULL) - return (1); + td = curthread; if (nframes <= 1) return (1); diff --git a/sys/kern/kern_condvar.c b/sys/kern/kern_condvar.c index 9355819..630075b 100644 --- a/sys/kern/kern_condvar.c +++ b/sys/kern/kern_condvar.c @@ -50,7 +50,7 @@ __FBSDID("$FreeBSD$"); * Common sanity checks for cv_wait* functions. */ #define CV_ASSERT(cvp, lock, td) do { \ - KASSERT((td) != NULL, ("%s: curthread NULL", __func__)); \ + KASSERT((td) != NULL, ("%s: passed td is NULL", __func__)); \ KASSERT(TD_IS_RUNNING(td), ("%s: not TDS_RUNNING", __func__)); \ KASSERT((cvp) != NULL, ("%s: cvp NULL", __func__)); \ KASSERT((lock) != NULL, ("%s: lock NULL", __func__)); \ diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c index 9827a9f..2ab7a09 100644 --- a/sys/kern/kern_mutex.c +++ b/sys/kern/kern_mutex.c @@ -200,7 +200,6 @@ _mtx_lock_flags(struct mtx *m, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return; - MPASS(curthread != NULL); KASSERT(!TD_IS_IDLETHREAD(curthread), ("mtx_lock() by idle thread %p on sleep mutex %s @ %s:%d", curthread, m->lock_object.lo_name, file, line)); @@ -225,7 +224,6 @@ _mtx_unlock_flags(struct mtx *m, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return; - MPASS(curthread != NULL); KASSERT(m->mtx_lock != MTX_DESTROYED, ("mtx_unlock() of destroyed mutex @ %s:%d", file, line)); KASSERT(LOCK_CLASS(&m->lock_object) == &lock_class_mtx_sleep, @@ -248,7 +246,6 @@ _mtx_lock_spin_flags(struct mtx *m, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return; - MPASS(curthread != NULL); KASSERT(m->mtx_lock != MTX_DESTROYED, ("mtx_lock_spin() of destroyed mutex @ %s:%d", file, line)); KASSERT(LOCK_CLASS(&m->lock_object) == &lock_class_mtx_spin, @@ -272,7 +269,6 @@ _mtx_unlock_spin_flags(struct mtx *m, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return; - MPASS(curthread != NULL); KASSERT(m->mtx_lock != MTX_DESTROYED, ("mtx_unlock_spin() of destroyed mutex @ %s:%d", file, line)); KASSERT(LOCK_CLASS(&m->lock_object) == &lock_class_mtx_spin, @@ -303,7 +299,6 @@ mtx_trylock_flags_(struct mtx *m, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return (1); - MPASS(curthread != NULL); KASSERT(!TD_IS_IDLETHREAD(curthread), ("mtx_trylock() by idle thread %p on sleep mutex %s @ %s:%d", curthread, m->lock_object.lo_name, file, line)); diff --git a/sys/kern/kern_rwlock.c b/sys/kern/kern_rwlock.c index e0be154..3a51874 100644 --- a/sys/kern/kern_rwlock.c +++ b/sys/kern/kern_rwlock.c @@ -241,7 +241,6 @@ _rw_wlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return; - MPASS(curthread != NULL); KASSERT(!TD_IS_IDLETHREAD(curthread), ("rw_wlock() by idle thread %p on rwlock %s @ %s:%d", curthread, rw->lock_object.lo_name, file, line)); @@ -292,7 +291,6 @@ _rw_wunlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return; - MPASS(curthread != NULL); KASSERT(rw->rw_lock != RW_DESTROYED, ("rw_wunlock() of destroyed rwlock @ %s:%d", file, line)); _rw_assert(rw, RA_WLOCKED, file, line); diff --git a/sys/kern/kern_sx.c b/sys/kern/kern_sx.c index 487a324..af2391f 100644 --- a/sys/kern/kern_sx.c +++ b/sys/kern/kern_sx.c @@ -249,7 +249,6 @@ _sx_slock(struct sx *sx, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return (0); - MPASS(curthread != NULL); KASSERT(!TD_IS_IDLETHREAD(curthread), ("sx_slock() by idle thread %p on sx %s @ %s:%d", curthread, sx->lock_object.lo_name, file, line)); @@ -303,7 +302,6 @@ _sx_xlock(struct sx *sx, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return (0); - MPASS(curthread != NULL); KASSERT(!TD_IS_IDLETHREAD(curthread), ("sx_xlock() by idle thread %p on sx %s @ %s:%d", curthread, sx->lock_object.lo_name, file, line)); @@ -330,7 +328,6 @@ sx_try_xlock_(struct sx *sx, const char *file, int line) if (SCHEDULER_STOPPED()) return (1); - MPASS(curthread != NULL); KASSERT(!TD_IS_IDLETHREAD(curthread), ("sx_try_xlock() by idle thread %p on sx %s @ %s:%d", curthread, sx->lock_object.lo_name, file, line)); @@ -361,7 +358,6 @@ _sx_sunlock(struct sx *sx, const char *file, int line) if (SCHEDULER_STOPPED()) return; - MPASS(curthread != NULL); KASSERT(sx->sx_lock != SX_LOCK_DESTROYED, ("sx_sunlock() of destroyed sx @ %s:%d", file, line)); _sx_assert(sx, SA_SLOCKED, file, line); @@ -378,7 +374,6 @@ _sx_xunlock(struct sx *sx, const char *file, int line) if (SCHEDULER_STOPPED()) return; - MPASS(curthread != NULL); KASSERT(sx->sx_lock != SX_LOCK_DESTROYED, ("sx_xunlock() of destroyed sx @ %s:%d", file, line)); _sx_assert(sx, SA_XLOCKED, file, line); diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c index c4ad7b8..7c21644 100644 --- a/sys/kern/kern_thread.c +++ b/sys/kern/kern_thread.c @@ -622,7 +622,6 @@ thread_single(int mode) p = td->td_proc; mtx_assert(&Giant, MA_NOTOWNED); PROC_LOCK_ASSERT(p, MA_OWNED); - KASSERT((td != NULL), ("curthread is NULL")); if ((p->p_flag & P_HADTHREADS) == 0) return (0); diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c index 06d16c0..5c781c2 100644 --- a/sys/kern/vfs_subr.c +++ b/sys/kern/vfs_subr.c @@ -3416,7 +3416,6 @@ vfs_unmountall(void) struct thread *td; int error; - KASSERT(curthread != NULL, ("vfs_unmountall: NULL curthread")); CTR1(KTR_VFS, "%s: unmounting all filesystems", __func__); td = curthread; -- 1.7.7.4 From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 01:37:00 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 799131065670; Thu, 13 Sep 2012 01:37:00 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-vb0-f54.google.com (mail-vb0-f54.google.com [209.85.212.54]) by mx1.freebsd.org (Postfix) with ESMTP id AAEE08FC08; Thu, 13 Sep 2012 01:36:59 +0000 (UTC) Received: by vbmv11 with SMTP id v11so3691636vbm.13 for ; Wed, 12 Sep 2012 18:36:58 -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=rYB5LR1d39aHJTMY2SvC5MiTXcXm/wjTV+w3q6n+OFQ=; b=hOxHbAY0K3pC1cijdZZdBEMi+KgEktAwhZL0/Y3/DWePJhGEIGke72jeXMfhT2zdp+ czdiVSOkb+YPNIZaM7zUfWb6Xt2vN9uU3FXnR7B3nYZH6IPhizTOOenyge1QZAd4bLsT j2FBbl9aCadUNLInazgRl2Epw0X2Enj62/oU8dNo4U7c+VbGkQNqEezJ3sp8cr1KJwI1 YN/fCrW47vW1jJXd5ldZPf7MkeBLAoCahW+mu0Dm3oUG97LgjE+iqEm0qdn9CnKUsprR pkK+3qDizAOK+K8Lw+lIhVp3XOm9MVZO9eiVV5PY6TmzHaI9l8bfm822JiBBbHuv8JAc lmvg== MIME-Version: 1.0 Received: by 10.52.26.137 with SMTP id l9mr194945vdg.62.1347500218792; Wed, 12 Sep 2012 18:36:58 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.220.106.6 with HTTP; Wed, 12 Sep 2012 18:36:58 -0700 (PDT) In-Reply-To: <201208021707.22356.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <201208021707.22356.jhb@freebsd.org> Date: Thu, 13 Sep 2012 02:36:58 +0100 X-Google-Sender-Auth: GBXXp1umUGoAAJzZB4SrVfIFetY Message-ID: From: Attilio Rao To: John Baldwin , mlaier@freebsd.org, Stephan Uphoff 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: Thu, 13 Sep 2012 01:37:00 -0000 On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: >> On 7/30/12, John Baldwin wrote: >> > --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 >> > 18:45:29.000000000 0000 >> > +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 21:20:58.000000000 >> > 0000 >> > @@ -70,6 +70,9 @@ >> > } >> > >> > static void assert_rm(const struct lock_object *lock, int what); >> > +#ifdef DDB >> > +static void db_show_rm(const struct lock_object *lock); >> > +#endif >> > static void lock_rm(struct lock_object *lock, int how); >> > #ifdef KDTRACE_HOOKS >> > static int owner_rm(const struct lock_object *lock, struct thread >> > **owner); >> >> While here, did you consider also: >> - Abstracting compiler_memory_barrier() into a MI, compiler dependent function? >> - Fix rm_queue with DCPU possibly > > Mostly I just wanted to fill in missing functionality and fixup the > RM_SLEEPABLE bits a bit. So what do you think about the following patch? If you agree I will send to pho@ for testing in a batch with other patches. Thanks, Attilio Subject: [PATCH 7/7] Reimplement pc_rm_queue as a dynamic entry for per-cpu members. Sparse notes: o rm_tracker_remove() doesn't seem to need the pc argument also in the current code o The stop ptr value changes from &pc->pc_rm_queue to NULL when scanning the cpus list Signed-off-by: Attilio Rao --- sys/kern/kern_rmlock.c | 42 ++++++++++++++++++++++-------------------- sys/kern/subr_pcpu.c | 2 -- sys/sys/_rmlock.h | 10 +++++----- sys/sys/pcpu.h | 18 ------------------ 4 files changed, 27 insertions(+), 45 deletions(-) diff --git a/sys/kern/kern_rmlock.c b/sys/kern/kern_rmlock.c index ef1920b..c6ba65a 100644 --- a/sys/kern/kern_rmlock.c +++ b/sys/kern/kern_rmlock.c @@ -76,6 +76,8 @@ static int owner_rm(const struct lock_object *lock, struct thread **owner); #endif static int unlock_rm(struct lock_object *lock); +static DPCPU_DEFINE(struct rm_queue, rm_queue); + struct lock_class lock_class_rm = { .lc_name = "rm", .lc_flags = LC_SLEEPLOCK | LC_RECURSABLE, @@ -133,24 +135,24 @@ MTX_SYSINIT(rm_spinlock, &rm_spinlock, "rm_spinlock", MTX_SPIN); * interrupt on the *local* cpu. */ static void inline -rm_tracker_add(struct pcpu *pc, struct rm_priotracker *tracker) +rm_tracker_add(struct rm_queue *pcpu_rm_queue, struct rm_priotracker *tracker) { struct rm_queue *next; /* Initialize all tracker pointers */ - tracker->rmp_cpuQueue.rmq_prev = &pc->pc_rm_queue; - next = pc->pc_rm_queue.rmq_next; + tracker->rmp_cpuQueue.rmq_prev = NULL; + next = pcpu_rm_queue->rmq_next; tracker->rmp_cpuQueue.rmq_next = next; /* rmq_prev is not used during froward traversal. */ next->rmq_prev = &tracker->rmp_cpuQueue; /* Update pointer to first element. */ - pc->pc_rm_queue.rmq_next = &tracker->rmp_cpuQueue; + pcpu_rm_queue->rmq_next = &tracker->rmp_cpuQueue; } static void inline -rm_tracker_remove(struct pcpu *pc, struct rm_priotracker *tracker) +rm_tracker_remove(struct rm_priotracker *tracker) { struct rm_queue *next, *prev; @@ -167,13 +169,12 @@ rm_tracker_remove(struct pcpu *pc, struct rm_priotracker *tracker) static void rm_cleanIPI(void *arg) { - struct pcpu *pc; struct rmlock *rm = arg; struct rm_priotracker *tracker; - struct rm_queue *queue; - pc = pcpu_find(curcpu); + struct rm_queue *queue, *pcpu_rm_queue; + pcpu_rm_queue = DPCPU_PTR(rm_queue); - for (queue = pc->pc_rm_queue.rmq_next; queue != &pc->pc_rm_queue; + for (queue = pcpu_rm_queue->rmq_next; queue != NULL; queue = queue->rmq_next) { tracker = (struct rm_priotracker *)queue; if (tracker->rmp_rmlock == rm && tracker->rmp_flags == 0) { @@ -256,11 +257,12 @@ static int _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker, int trylock) { struct pcpu *pc; - struct rm_queue *queue; + struct rm_queue *queue, *pcpu_rm_queue; struct rm_priotracker *atracker; critical_enter(); pc = pcpu_find(curcpu); + pcpu_rm_queue = DPCPU_PTR(rm_queue); /* Check if we just need to do a proper critical_exit. */ if (!CPU_ISSET(pc->pc_cpuid, &rm->rm_writecpus)) { @@ -269,12 +271,12 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker, int trylock) } /* Remove our tracker from the per-cpu list. */ - rm_tracker_remove(pc, tracker); + rm_tracker_remove(tracker); /* Check to see if the IPI granted us the lock after all. */ if (tracker->rmp_flags) { /* Just add back tracker - we hold the lock. */ - rm_tracker_add(pc, tracker); + rm_tracker_add(pcpu_rm_queue, tracker); critical_exit(); return (1); } @@ -288,8 +290,8 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker, int trylock) * Just grant the lock if this thread already has a tracker * for this lock on the per-cpu queue. */ - for (queue = pc->pc_rm_queue.rmq_next; - queue != &pc->pc_rm_queue; queue = queue->rmq_next) { + for (queue = pcpu_rm_queue->rmq_next; queue != NULL; + queue = queue->rmq_next) { atracker = (struct rm_priotracker *)queue; if ((atracker->rmp_rmlock == rm) && (atracker->rmp_thread == tracker->rmp_thread)) { @@ -298,7 +300,7 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker, int trylock) tracker, rmp_qentry); tracker->rmp_flags = RMPF_ONQUEUE; mtx_unlock_spin(&rm_spinlock); - rm_tracker_add(pc, tracker); + rm_tracker_add(pcpu_rm_queue, tracker); critical_exit(); return (1); } @@ -326,7 +328,7 @@ _rm_rlock_hard(struct rmlock *rm, struct rm_priotracker *tracker, int trylock) critical_enter(); pc = pcpu_find(curcpu); CPU_CLR(pc->pc_cpuid, &rm->rm_writecpus); - rm_tracker_add(pc, tracker); + rm_tracker_add(pcpu_rm_queue, tracker); sched_pin(); critical_exit(); @@ -342,6 +344,7 @@ int _rm_rlock(struct rmlock *rm, struct rm_priotracker *tracker, int trylock) { struct thread *td = curthread; + struct rm_queue *pcpu_rm_queue; struct pcpu *pc; if (SCHEDULER_STOPPED()) @@ -356,8 +359,9 @@ _rm_rlock(struct rmlock *rm, struct rm_priotracker *tracker, int trylock) compiler_memory_barrier(); pc = cpuid_to_pcpu[td->td_oncpu]; /* pcpu_find(td->td_oncpu); */ + pcpu_rm_queue = DPCPU_ID_PTR(td->td_oncpu, rm_queue); - rm_tracker_add(pc, tracker); + rm_tracker_add(pcpu_rm_queue, tracker); sched_pin(); @@ -413,15 +417,13 @@ _rm_unlock_hard(struct thread *td,struct rm_priotracker *tracker) void _rm_runlock(struct rmlock *rm, struct rm_priotracker *tracker) { - struct pcpu *pc; struct thread *td = tracker->rmp_thread; if (SCHEDULER_STOPPED()) return; td->td_critnest++; /* critical_enter(); */ - pc = cpuid_to_pcpu[td->td_oncpu]; /* pcpu_find(td->td_oncpu); */ - rm_tracker_remove(pc, tracker); + rm_tracker_remove(tracker); td->td_critnest--; sched_unpin(); diff --git a/sys/kern/subr_pcpu.c b/sys/kern/subr_pcpu.c index 505a4df..279295e 100644 --- a/sys/kern/subr_pcpu.c +++ b/sys/kern/subr_pcpu.c @@ -90,8 +90,6 @@ pcpu_init(struct pcpu *pcpu, int cpuid, size_t size) cpuid_to_pcpu[cpuid] = pcpu; STAILQ_INSERT_TAIL(&cpuhead, pcpu, pc_allcpu); cpu_pcpu_init(pcpu, cpuid, size); - pcpu->pc_rm_queue.rmq_next = &pcpu->pc_rm_queue; - pcpu->pc_rm_queue.rmq_prev = &pcpu->pc_rm_queue; } void diff --git a/sys/sys/_rmlock.h b/sys/sys/_rmlock.h index 15d6c49..51bb03e 100644 --- a/sys/sys/_rmlock.h +++ b/sys/sys/_rmlock.h @@ -32,11 +32,6 @@ #ifndef _SYS__RMLOCK_H_ #define _SYS__RMLOCK_H_ -/* - * XXXUPS remove as soon as we have per cpu variable - * linker sets and can define rm_queue in _rm_lock.h -*/ -#include /* * Mostly reader/occasional writer lock. */ @@ -55,6 +50,11 @@ struct rmlock { #define rm_lock_mtx _rm_lock._rm_lock_mtx #define rm_lock_sx _rm_lock._rm_lock_sx +struct rm_queue { + struct rm_queue *volatile rmq_next; + struct rm_queue *volatile rmq_prev; +}; + struct rm_priotracker { struct rm_queue rmp_cpuQueue; /* Must be first */ struct rmlock *rmp_rmlock; diff --git a/sys/sys/pcpu.h b/sys/sys/pcpu.h index 4a4ec00..78ba04a 100644 --- a/sys/sys/pcpu.h +++ b/sys/sys/pcpu.h @@ -137,15 +137,6 @@ extern uintptr_t dpcpu_off[]; #endif /* _KERNEL */ -/* - * XXXUPS remove as soon as we have per cpu variable - * linker sets and can define rm_queue in _rm_lock.h - */ -struct rm_queue { - struct rm_queue* volatile rmq_next; - struct rm_queue* volatile rmq_prev; -}; - /* * This structure maps out the global data that needs to be kept on a * per-cpu basis. The members are accessed via the PCPU_GET/SET/PTR @@ -169,15 +160,6 @@ struct pcpu { void *pc_netisr; /* netisr SWI cookie */ int pc_dnweight; /* vm_page_dontneed() */ int pc_domain; /* Memory domain. */ - - /* - * Stuff for read mostly lock - * - * XXXUPS remove as soon as we have per cpu variable - * linker sets. - */ - struct rm_queue pc_rm_queue; - uintptr_t pc_dynamic; /* Dynamic per-cpu data area */ /* -- 1.7.7.4 From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 14:27:55 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A28181065708; Thu, 13 Sep 2012 14:27:55 +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 722808FC18; Thu, 13 Sep 2012 14:27:55 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id BB785B983; Thu, 13 Sep 2012 10:27:54 -0400 (EDT) From: John Baldwin To: attilio@freebsd.org Date: Thu, 13 Sep 2012 09:10:50 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p17; KDE/4.5.5; amd64; ; ) References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201208021707.22356.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201209130910.50876.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 13 Sep 2012 10:27:54 -0400 (EDT) Cc: Davide Italiano , mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov , src-committers@freebsd.org, Stephan Uphoff 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: Thu, 13 Sep 2012 14:27:55 -0000 On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: > On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: > > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: > >> On 7/30/12, John Baldwin wrote: > >> > --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 > >> > 18:45:29.000000000 0000 > >> > +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 21:20:58.000000000 > >> > 0000 > >> > @@ -70,6 +70,9 @@ > >> > } > >> > > >> > static void assert_rm(const struct lock_object *lock, int what); > >> > +#ifdef DDB > >> > +static void db_show_rm(const struct lock_object *lock); > >> > +#endif > >> > static void lock_rm(struct lock_object *lock, int how); > >> > #ifdef KDTRACE_HOOKS > >> > static int owner_rm(const struct lock_object *lock, struct thread > >> > **owner); > >> > >> While here, did you consider also: > >> - Abstracting compiler_memory_barrier() into a MI, compiler dependent function? > >> - Fix rm_queue with DCPU possibly > > > > Mostly I just wanted to fill in missing functionality and fixup the > > RM_SLEEPABLE bits a bit. > > So what do you think about the following patch? If you agree I will > send to pho@ for testing in a batch with other patches. It's not super clear to me that having it be static vs dynamic is all that big of a deal. However, your approach in general is better, and it certainly should have been using PCPU_GET() for the curcpu case all along rather than inlining pcpu_find(). > --- a/sys/kern/kern_rmlock.c > +++ b/sys/kern/kern_rmlock.c > @@ -167,13 +169,12 @@ rm_tracker_remove(struct pcpu *pc, struct > rm_priotracker *tracker) > static void > rm_cleanIPI(void *arg) > { > - struct pcpu *pc; > struct rmlock *rm = arg; > struct rm_priotracker *tracker; > - struct rm_queue *queue; > - pc = pcpu_find(curcpu); > + struct rm_queue *queue, *pcpu_rm_queue; > + pcpu_rm_queue = DPCPU_PTR(rm_queue); Can you fix the old style bug of not having a blank line after the variable declarations? > - for (queue = pc->pc_rm_queue.rmq_next; queue != &pc->pc_rm_queue; > + for (queue = pcpu_rm_queue->rmq_next; queue != NULL; > queue = queue->rmq_next) { It would be nice to use one of the queue macros rather than doing the list management by hand, but perhaps that isn't possible (and that should be a separate change even if it possible). -- John Baldwin From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 14:27:56 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 748091065701; Thu, 13 Sep 2012 14:27:56 +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 477178FC19; Thu, 13 Sep 2012 14:27:56 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id AB86EB988; Thu, 13 Sep 2012 10:27:55 -0400 (EDT) From: John Baldwin To: attilio@freebsd.org Date: Thu, 13 Sep 2012 09:12:38 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p17; KDE/4.5.5; amd64; ; ) References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209121511.42296.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Message-Id: <201209130912.39029.jhb@freebsd.org> 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); Thu, 13 Sep 2012 10:27:55 -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: Thu, 13 Sep 2012 14:27:56 -0000 On Wednesday, September 12, 2012 8:07:11 pm Attilio Rao wrote: > On Wed, Sep 12, 2012 at 8:11 PM, John Baldwin wrote: > > On Monday, September 10, 2012 9:11:22 pm Attilio Rao wrote: > >> 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? > > > > They can probably be axed. > > What do you think about this? > Please note that I would also axe the check in printtrap() on several > arches, but maybe there is a valid reason to keep it I'm not thinking > right now, so I left it out in this patch. There can be a window where curthread is NULL during early boot (e.g. if you got a trap / fault during initi386() or the equivalent). I think the patch is fine. -- John Baldwin From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 14:38:58 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 0678E1065670; Thu, 13 Sep 2012 14:38:58 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 7A8F78FC15; Thu, 13 Sep 2012 14:38:56 +0000 (UTC) Received: by lbbgg13 with SMTP id gg13so2446899lbb.13 for ; Thu, 13 Sep 2012 07:38:55 -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=11Tad2H4YfDJc7Zp4xWH1TbWvkdGF+V+xRh5yN9b2sY=; b=WwSDAcoP+1kziIaIeN4nbiyIJYQOIaBSIrJzOo97kkDRExNYk9+vk85vWLEIEnzJgJ C51euZMU6LvUcDZvp2mA1fmxJpMNnmvzXRd0/lqpGyc5SeU3KC7ywwZOqfWCAtC2E5m8 zDaQK48NQMILtheyfOhCT6cDymMIlZZzK+bzVGPkOvig5G9bcbLAvstI2RvsVTBA2H7m z9m6NpcU2qiA2qTXJ7sAtNuhx1Y0+wfONxJRkfoJOhc9RKdwZdA+vw7olRz4LNgmZU1G WufiWOyWHb89J6gw2xA5c5YHmbaKapEaiLm8yZocgJHqlDOeIm79Q74y/eQ3wwx4NCHh GhaQ== MIME-Version: 1.0 Received: by 10.112.86.41 with SMTP id m9mr996634lbz.108.1347547135147; Thu, 13 Sep 2012 07:38:55 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Thu, 13 Sep 2012 07:38:54 -0700 (PDT) In-Reply-To: <201209130910.50876.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201208021707.22356.jhb@freebsd.org> <201209130910.50876.jhb@freebsd.org> Date: Thu, 13 Sep 2012 15:38:54 +0100 X-Google-Sender-Auth: BAi4PyoHE3Kzhcgl9AuwIj1Q-co Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Cc: Davide Italiano , mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov , src-committers@freebsd.org, Stephan Uphoff 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: Thu, 13 Sep 2012 14:38:58 -0000 On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin wrote: > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: >> >> On 7/30/12, John Baldwin wrote: >> >> > --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 >> >> > 18:45:29.000000000 0000 >> >> > +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 21:20:58.000000000 >> >> > 0000 >> >> > @@ -70,6 +70,9 @@ >> >> > } >> >> > >> >> > static void assert_rm(const struct lock_object *lock, int what); >> >> > +#ifdef DDB >> >> > +static void db_show_rm(const struct lock_object *lock); >> >> > +#endif >> >> > static void lock_rm(struct lock_object *lock, int how); >> >> > #ifdef KDTRACE_HOOKS >> >> > static int owner_rm(const struct lock_object *lock, struct thread >> >> > **owner); >> >> >> >> While here, did you consider also: >> >> - Abstracting compiler_memory_barrier() into a MI, compiler dependent function? >> >> - Fix rm_queue with DCPU possibly >> > >> > Mostly I just wanted to fill in missing functionality and fixup the >> > RM_SLEEPABLE bits a bit. >> >> So what do you think about the following patch? If you agree I will >> send to pho@ for testing in a batch with other patches. > > It's not super clear to me that having it be static vs dynamic is all that > big of a deal. However, your approach in general is better, and it certainly > should have been using PCPU_GET() for the curcpu case all along rather than > inlining pcpu_find(). You mean what is the performance difference between static vs dynamic? Or you mean, why we want such patch at all? In the former question there is a further indirection (pc_dynamic access), for the latter question the patched code avoids namespace pollution at all and makes the code more readable. >> --- a/sys/kern/kern_rmlock.c >> +++ b/sys/kern/kern_rmlock.c >> @@ -167,13 +169,12 @@ rm_tracker_remove(struct pcpu *pc, struct >> rm_priotracker *tracker) >> static void >> rm_cleanIPI(void *arg) >> { >> - struct pcpu *pc; >> struct rmlock *rm = arg; >> struct rm_priotracker *tracker; >> - struct rm_queue *queue; >> - pc = pcpu_find(curcpu); >> + struct rm_queue *queue, *pcpu_rm_queue; >> + pcpu_rm_queue = DPCPU_PTR(rm_queue); > > Can you fix the old style bug of not having a blank line after the > variable declarations? bde@ has sent me a lot of notes on how the files I'm touching are style-broken already. I'm trying to aligning to existing style right now, and will fix all the style bugs pointed out by Bruce (and you) in separate commits. Thus, for the moment, I would leave these chunks like they are now. >> - for (queue = pc->pc_rm_queue.rmq_next; queue != &pc->pc_rm_queue; >> + for (queue = pcpu_rm_queue->rmq_next; queue != NULL; >> queue = queue->rmq_next) { > > It would be nice to use one of the queue macros rather than doing the > list management by hand, but perhaps that isn't possible (and that > should be a separate change even if it possible). I agree on the separate change, not 100% sure if this is feasible but I will check once this goes in. So is the patch approved by you? Attilio -- Peace can only be achieved by understanding - A. Einstein From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 15:45:24 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 350EF106564A; Thu, 13 Sep 2012 15:45:24 +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 0619D8FC08; Thu, 13 Sep 2012 15:45:24 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 45091B983; Thu, 13 Sep 2012 11:45:23 -0400 (EDT) From: John Baldwin To: attilio@freebsd.org Date: Thu, 13 Sep 2012 11:32:20 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p17; KDE/4.5.5; amd64; ; ) References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201209131132.21103.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 13 Sep 2012 11:45:23 -0400 (EDT) Cc: Davide Italiano , mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov , src-committers@freebsd.org, Stephan Uphoff 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: Thu, 13 Sep 2012 15:45:24 -0000 On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote: > On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin wrote: > > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: > >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: > >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: > >> >> On 7/30/12, John Baldwin wrote: > >> >> > --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 > >> >> > 18:45:29.000000000 0000 > >> >> > +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 21:20:58.000000000 > >> >> > 0000 > >> >> > @@ -70,6 +70,9 @@ > >> >> > } > >> >> > > >> >> > static void assert_rm(const struct lock_object *lock, int what); > >> >> > +#ifdef DDB > >> >> > +static void db_show_rm(const struct lock_object *lock); > >> >> > +#endif > >> >> > static void lock_rm(struct lock_object *lock, int how); > >> >> > #ifdef KDTRACE_HOOKS > >> >> > static int owner_rm(const struct lock_object *lock, struct thread > >> >> > **owner); > >> >> > >> >> While here, did you consider also: > >> >> - Abstracting compiler_memory_barrier() into a MI, compiler dependent function? > >> >> - Fix rm_queue with DCPU possibly > >> > > >> > Mostly I just wanted to fill in missing functionality and fixup the > >> > RM_SLEEPABLE bits a bit. > >> > >> So what do you think about the following patch? If you agree I will > >> send to pho@ for testing in a batch with other patches. > > > > It's not super clear to me that having it be static vs dynamic is all that > > big of a deal. However, your approach in general is better, and it certainly > > should have been using PCPU_GET() for the curcpu case all along rather than > > inlining pcpu_find(). > > You mean what is the performance difference between static vs dynamic? > Or you mean, why we want such patch at all? > In the former question there is a further indirection (pc_dynamic > access), for the latter question the patched code avoids namespace > pollution at all and makes the code more readable. More why we want it. I think most of your readability fixes would work just as well if it remained static and we used PCPU_GET(). However, I think your changes are fine. FYI, much of subr_rmlock.c goes out of its way to optimize for performance (such as inlining critical_enter(), critical_exit(), and pcpu_find()), so adding the new indirection goes against the grain of that. -- John Baldwin From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 16:20:58 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 09EEF106564A; Thu, 13 Sep 2012 16:20:58 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 7C6D28FC0C; Thu, 13 Sep 2012 16:20:56 +0000 (UTC) Received: by lbbgg13 with SMTP id gg13so2549592lbb.13 for ; Thu, 13 Sep 2012 09:20:55 -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=W5d7rOjlV+fsas4SGbQgRnviWEJAOh657PvcJYzDpcI=; b=Sd0S0iBqL3Tou4kDfBqdJQ1tAqAz+ziSEo7qlUqV6++fWDtyFlrr3CJeVmIfuLfwvN Jz3A4giL/ILi8NzcX7BSZWVR3wtIKSzu5ZpY7Eheyc1CL+DpxSfIPs7w187LF764JEXV UJrqSWPcP+UIP2b++quDe6Li5TyKhN2bmx0M3rT7+otKSZfLDCDJVjvCTPNUPdw1wBXh uGdJB/UXzfj9zYlQV/xmsGec5OQ62xb7QhGU1p01ui2XJFuYd2QHSTluAsxMve4VVI2K hyRmWmbQn3MFNeOj2EYYHWod1PJRFTJkMQJro0MWkENHBP2HOcosltlBs0RgndBSp+d6 S/tA== MIME-Version: 1.0 Received: by 10.112.17.163 with SMTP id p3mr71378lbd.83.1347553255247; Thu, 13 Sep 2012 09:20:55 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Thu, 13 Sep 2012 09:20:54 -0700 (PDT) In-Reply-To: <201209131132.21103.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <201209131132.21103.jhb@freebsd.org> Date: Thu, 13 Sep 2012 17:20:54 +0100 X-Google-Sender-Auth: ZcL04eqF14HRlR4cERA2elyKGU0 Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Cc: Davide Italiano , mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov , src-committers@freebsd.org, Stephan Uphoff 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: Thu, 13 Sep 2012 16:20:58 -0000 On 9/13/12, John Baldwin wrote: > On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote: >> On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin wrote: >> > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: >> >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: >> >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: >> >> >> On 7/30/12, John Baldwin wrote: >> >> >> > --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 >> >> >> > 18:45:29.000000000 0000 >> >> >> > +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 >> >> >> > 21:20:58.000000000 >> >> >> > 0000 >> >> >> > @@ -70,6 +70,9 @@ >> >> >> > } >> >> >> > >> >> >> > static void assert_rm(const struct lock_object *lock, int >> >> >> > what); >> >> >> > +#ifdef DDB >> >> >> > +static void db_show_rm(const struct lock_object *lock); >> >> >> > +#endif >> >> >> > static void lock_rm(struct lock_object *lock, int how); >> >> >> > #ifdef KDTRACE_HOOKS >> >> >> > static int owner_rm(const struct lock_object *lock, struct >> >> >> > thread >> >> >> > **owner); >> >> >> >> >> >> While here, did you consider also: >> >> >> - Abstracting compiler_memory_barrier() into a MI, compiler >> >> >> dependent function? >> >> >> - Fix rm_queue with DCPU possibly >> >> > >> >> > Mostly I just wanted to fill in missing functionality and fixup the >> >> > RM_SLEEPABLE bits a bit. >> >> >> >> So what do you think about the following patch? If you agree I will >> >> send to pho@ for testing in a batch with other patches. >> > >> > It's not super clear to me that having it be static vs dynamic is all >> > that >> > big of a deal. However, your approach in general is better, and it >> > certainly >> > should have been using PCPU_GET() for the curcpu case all along rather >> > than >> > inlining pcpu_find(). >> >> You mean what is the performance difference between static vs dynamic? >> Or you mean, why we want such patch at all? >> In the former question there is a further indirection (pc_dynamic >> access), for the latter question the patched code avoids namespace >> pollution at all and makes the code more readable. > > More why we want it. I think most of your readability fixes would work > just > as well if it remained static and we used PCPU_GET(). However, I think > your > changes are fine. Well, the namespace pollution cannot be avoided without using the dynamic approach, and that is the important part of the patch. > FYI, much of subr_rmlock.c goes out of its way to optimize for performance > (such as inlining critical_enter(), critical_exit(), and pcpu_find()), so > adding the new indirection goes against the grain of that. That one of the reasons why I'm asking for advices here actually. I would like to understand if we prefer a cleaner approach or avoid one further indirection with a super-optimized path. Attilio -- Peace can only be achieved by understanding - A. Einstein From owner-svn-src-projects@FreeBSD.ORG Fri Sep 14 22:32:40 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 1F026106566C; Fri, 14 Sep 2012 22:32:40 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lpp01m010-f54.google.com (mail-lpp01m010-f54.google.com [209.85.215.54]) by mx1.freebsd.org (Postfix) with ESMTP id 8CA158FC0A; Fri, 14 Sep 2012 22:32:37 +0000 (UTC) Received: by lage12 with SMTP id e12so3718141lag.13 for ; Fri, 14 Sep 2012 15:32:36 -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=vV9KhfTokeTtPnj5wNiUOgBCih5T2t5gmoeMSyMU3p8=; b=qaW5MovH6XX81eyekUhUQCU1ILJX270pSNQuMoDetlSNKPWJENlxFCypfBW/A80in/ XnyfOASwtyYGRj/WjZW/IRuhha81XPrILDwzPXLzliogebOlBkVIUHJDvmO8TICfezNE k2yFiA/+GIaERN/HBTzO+gUTG4fAZMQKlBmcirtKdxqMHFNpMaZMfaRsn0cN3uXxOKNq fv7lsui0FWgStpHy0vCXk0P1WHkPvHKw9T9C3BGGAtuIOn6NDX0E1tkhTUSqdXA1MJdZ 61RJ11+lN9aK6PelHJyQO99oiTODzpBLqWWjiFjnWqus5qpqI0ZGJN74iV9JGZj9ZLP4 1m0w== MIME-Version: 1.0 Received: by 10.152.131.37 with SMTP id oj5mr3795243lab.14.1347661956528; Fri, 14 Sep 2012 15:32:36 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Fri, 14 Sep 2012 15:32:35 -0700 (PDT) In-Reply-To: References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <201209131132.21103.jhb@freebsd.org> Date: Fri, 14 Sep 2012 23:32:35 +0100 X-Google-Sender-Auth: mx4oo8WVkKm-09_CQAOD5N_uyQ8 Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Cc: Davide Italiano , mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov , src-committers@freebsd.org, Stephan Uphoff 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: Fri, 14 Sep 2012 22:32:40 -0000 On Thu, Sep 13, 2012 at 5:20 PM, Attilio Rao wrote: > On 9/13/12, John Baldwin wrote: >> On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote: >>> On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin wrote: >>> > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: >>> >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: >>> >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: >>> >> >> On 7/30/12, John Baldwin wrote: >>> >> >> > --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 >>> >> >> > 18:45:29.000000000 0000 >>> >> >> > +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 >>> >> >> > 21:20:58.000000000 >>> >> >> > 0000 >>> >> >> > @@ -70,6 +70,9 @@ >>> >> >> > } >>> >> >> > >>> >> >> > static void assert_rm(const struct lock_object *lock, int >>> >> >> > what); >>> >> >> > +#ifdef DDB >>> >> >> > +static void db_show_rm(const struct lock_object *lock); >>> >> >> > +#endif >>> >> >> > static void lock_rm(struct lock_object *lock, int how); >>> >> >> > #ifdef KDTRACE_HOOKS >>> >> >> > static int owner_rm(const struct lock_object *lock, struct >>> >> >> > thread >>> >> >> > **owner); >>> >> >> >>> >> >> While here, did you consider also: >>> >> >> - Abstracting compiler_memory_barrier() into a MI, compiler >>> >> >> dependent function? >>> >> >> - Fix rm_queue with DCPU possibly >>> >> > >>> >> > Mostly I just wanted to fill in missing functionality and fixup the >>> >> > RM_SLEEPABLE bits a bit. >>> >> >>> >> So what do you think about the following patch? If you agree I will >>> >> send to pho@ for testing in a batch with other patches. >>> > >>> > It's not super clear to me that having it be static vs dynamic is all >>> > that >>> > big of a deal. However, your approach in general is better, and it >>> > certainly >>> > should have been using PCPU_GET() for the curcpu case all along rather >>> > than >>> > inlining pcpu_find(). >>> >>> You mean what is the performance difference between static vs dynamic? >>> Or you mean, why we want such patch at all? >>> In the former question there is a further indirection (pc_dynamic >>> access), for the latter question the patched code avoids namespace >>> pollution at all and makes the code more readable. >> >> More why we want it. I think most of your readability fixes would work >> just >> as well if it remained static and we used PCPU_GET(). However, I think >> your >> changes are fine. > > Well, the namespace pollution cannot be avoided without using the > dynamic approach, and that is the important part of the patch. > >> FYI, much of subr_rmlock.c goes out of its way to optimize for performance >> (such as inlining critical_enter(), critical_exit(), and pcpu_find()), so >> adding the new indirection goes against the grain of that. > I've thought about it and I think that avoiding the indirection is sensitive in that codepath. I've then came up with this patch which should avoid namespace pollution and the indirection. What do you think about it? Thanks, Attilio Subject: [PATCH 9/9] Avoid namespace pollution of sys/sys/pcpu.h in sys/sys/_rmlock.h. pc_rm_queue should really be implemented as a dynamic per-cpu object. but, the read-mode operation should be kept as fast as possible, so in order to avoid the extra-indirection from accessing pc_dynamic, just make it a static part of the per-cpu area. Avoid further the namespace pollution of pcpu.h in _rmlock.h by defining a verbatim copy of struct rm_queue as it is needed. There could be a CTASSERT() in the headers in the case the struct is already defined in order to see if the size matches, but generally this should not be too important as the verbatim can be easilly found by grepping. Also, note that an inclusion of _rmlock.h into pcpu.h would be problematic because _rmlock.h also requires _sx.h which in turn requires _lock.h (and this may not be the end) which results in another namespace pollution. Signed-off-by: Attilio Rao --- sys/sys/_rmlock.h | 15 ++++++++++----- sys/sys/pcpu.h | 25 +++++++++++++------------ 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/sys/sys/_rmlock.h b/sys/sys/_rmlock.h index 15d6c49..f2e713f 100644 --- a/sys/sys/_rmlock.h +++ b/sys/sys/_rmlock.h @@ -32,15 +32,20 @@ #ifndef _SYS__RMLOCK_H_ #define _SYS__RMLOCK_H_ -/* - * XXXUPS remove as soon as we have per cpu variable - * linker sets and can define rm_queue in _rm_lock.h -*/ -#include /* * Mostly reader/occasional writer lock. */ +/* Check the comment present in sys/pcpu.h. */ +#ifndef RMLOCK_DEFINE_RM_QUEUE +#define RMLOCK_DEFINE_RM_QUEUE + +struct rm_queue { + struct rm_queue* volatile rmq_next; + struct rm_queue* volatile rmq_prev; +}; +#endif + LIST_HEAD(rmpriolist,rm_priotracker); struct rmlock { diff --git a/sys/sys/pcpu.h b/sys/sys/pcpu.h index 4a4ec00..6536255 100644 --- a/sys/sys/pcpu.h +++ b/sys/sys/pcpu.h @@ -137,14 +137,23 @@ extern uintptr_t dpcpu_off[]; #endif /* _KERNEL */ -/* - * XXXUPS remove as soon as we have per cpu variable - * linker sets and can define rm_queue in _rm_lock.h +#ifndef RMLOCK_DEFINE_RM_QUEUE +#define RMLOCK_DEFINE_RM_QUEUE + +/* + * pc_rm_queue should really be implemented as a dynamic per-cpu object. + * Anyway, the read-mode operation should be kept as fast as possible, + * so in order to avoid the extra-indirection from accessing pc_dynamic, + * just make it a static part of the per-cpu area. + * Also, the definition of the struct rm_queue must be present in both + * sys/sys/_rmlock.h and sys/sys/pcpu.h. In order to avoid namespace + * pollutions, however, in a way and the other, use a verbatim definition. */ struct rm_queue { struct rm_queue* volatile rmq_next; struct rm_queue* volatile rmq_prev; }; +#endif /* * This structure maps out the global data that needs to be kept on a @@ -169,15 +178,7 @@ struct pcpu { void *pc_netisr; /* netisr SWI cookie */ int pc_dnweight; /* vm_page_dontneed() */ int pc_domain; /* Memory domain. */ - - /* - * Stuff for read mostly lock - * - * XXXUPS remove as soon as we have per cpu variable - * linker sets. - */ - struct rm_queue pc_rm_queue; - + struct rm_queue pc_rm_queue; /* rmlock list of trackers. */ uintptr_t pc_dynamic; /* Dynamic per-cpu data area */ /* From owner-svn-src-projects@FreeBSD.ORG Sat Sep 15 23:52:55 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 B02CF106564A; Sat, 15 Sep 2012 23:52:55 +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 643048FC12; Sat, 15 Sep 2012 23:52:55 +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 362E1B911; Sat, 15 Sep 2012 19:52:54 -0400 (EDT) Message-ID: <505514D5.90800@FreeBSD.org> Date: Sat, 15 Sep 2012 19:52:53 -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> <201209130910.50876.jhb@freebsd.org> <201209131132.21103.jhb@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); Sat, 15 Sep 2012 19:52:54 -0400 (EDT) Cc: Davide Italiano , mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov , src-committers@freebsd.org, Stephan Uphoff 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: Sat, 15 Sep 2012 23:52:55 -0000 On 9/14/12 6:32 PM, Attilio Rao wrote: > On Thu, Sep 13, 2012 at 5:20 PM, Attilio Rao wrote: >> On 9/13/12, John Baldwin wrote: >>> On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote: >>>> On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin wrote: >>>>> On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: >>>>>> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: >>>>>>> On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: >>>>>>>> On 7/30/12, John Baldwin wrote: >>>>>>>>> --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 >>>>>>>>> 18:45:29.000000000 0000 >>>>>>>>> +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 >>>>>>>>> 21:20:58.000000000 >>>>>>>>> 0000 >>>>>>>>> @@ -70,6 +70,9 @@ >>>>>>>>> } >>>>>>>>> >>>>>>>>> static void assert_rm(const struct lock_object *lock, int >>>>>>>>> what); >>>>>>>>> +#ifdef DDB >>>>>>>>> +static void db_show_rm(const struct lock_object *lock); >>>>>>>>> +#endif >>>>>>>>> static void lock_rm(struct lock_object *lock, int how); >>>>>>>>> #ifdef KDTRACE_HOOKS >>>>>>>>> static int owner_rm(const struct lock_object *lock, struct >>>>>>>>> thread >>>>>>>>> **owner); >>>>>>>> >>>>>>>> While here, did you consider also: >>>>>>>> - Abstracting compiler_memory_barrier() into a MI, compiler >>>>>>>> dependent function? >>>>>>>> - Fix rm_queue with DCPU possibly >>>>>>> >>>>>>> Mostly I just wanted to fill in missing functionality and fixup the >>>>>>> RM_SLEEPABLE bits a bit. >>>>>> >>>>>> So what do you think about the following patch? If you agree I will >>>>>> send to pho@ for testing in a batch with other patches. >>>>> >>>>> It's not super clear to me that having it be static vs dynamic is all >>>>> that >>>>> big of a deal. However, your approach in general is better, and it >>>>> certainly >>>>> should have been using PCPU_GET() for the curcpu case all along rather >>>>> than >>>>> inlining pcpu_find(). >>>> >>>> You mean what is the performance difference between static vs dynamic? >>>> Or you mean, why we want such patch at all? >>>> In the former question there is a further indirection (pc_dynamic >>>> access), for the latter question the patched code avoids namespace >>>> pollution at all and makes the code more readable. >>> >>> More why we want it. I think most of your readability fixes would work >>> just >>> as well if it remained static and we used PCPU_GET(). However, I think >>> your >>> changes are fine. >> >> Well, the namespace pollution cannot be avoided without using the >> dynamic approach, and that is the important part of the patch. >> >>> FYI, much of subr_rmlock.c goes out of its way to optimize for performance >>> (such as inlining critical_enter(), critical_exit(), and pcpu_find()), so >>> adding the new indirection goes against the grain of that. >> > > I've thought about it and I think that avoiding the indirection is > sensitive in that codepath. I've then came up with this patch which > should avoid namespace pollution and the indirection. > > What do you think about it? Why not just move rm_queue to _rmlock.h and make pcpu.h include that? Barring that, make a _rmlock_queue.h and have both headers include that. However, I think that having _rmlock.h in pcpu.h is fine. -- John Baldwin