From owner-svn-src-projects@FreeBSD.ORG Sun Sep 9 15:03:17 2012
Return-Path: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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: <CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
References: <201207301350.q6UDobCI099069@svn.freebsd.org>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
Date: Sun, 9 Sep 2012 16:03:14 +0100
X-Google-Sender-Auth: zJLgs-gyubSS6i04OEBpXDHJhUY
Message-ID: <CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Konstantin Belousov <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sun, 09 Sep 2012 15:03:17 -0000
On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <kostikbel@gmail.com>
To: Attilio Rao <attilio@freebsd.org>
Message-ID: <20120909190944.GO33100@deviant.kiev.zoral.com.ua>
References: <201207301350.q6UDobCI099069@svn.freebsd.org>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
Mime-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
protocol="application/pgp-signature"; boundary="boFd84b/DoLkbats"
Content-Disposition: inline
In-Reply-To: <CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
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 <davide@freebsd.org>, svn-src-projects@freebsd.org,
src-committers@freebsd.org, John Baldwin <jhb@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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <attilio@freebsd.org> wrote:
> > On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <jhb@FreeBSD.org>
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>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
In-Reply-To: <CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
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 <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <attilio@freebsd.org> wrote:
>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
<504CEAE0.704@FreeBSD.org>
Date: Sun, 9 Sep 2012 20:23:18 +0100
X-Google-Sender-Auth: NbGfNhgDyTm3_b5Fg-E0wohsRN0
Message-ID: <CAJ-FndCuQz8mJwLMUM3j9rAfvkH3848U6t7wv-c=8YerTKUdOw@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Konstantin Belousov <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sun, 09 Sep 2012 19:23:27 -0000
On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin <jhb@freebsd.org> wrote:
> On 9/9/12 11:03 AM, Attilio Rao wrote:
>> On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
>>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
<504CEAE0.704@FreeBSD.org>
Date: Sun, 9 Sep 2012 20:34:24 +0100
X-Google-Sender-Auth: _mQY3cublmA8Elsbx_Usw9a-wgo
Message-ID: <CAJ-FndBg3C0pohwfxr_RAAAai5DK1tk62jr2ua-rkSVttWTsWQ@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Konstantin Belousov <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sun, 09 Sep 2012 19:34:27 -0000
On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin <jhb@freebsd.org> wrote:
> On 9/9/12 11:03 AM, Attilio Rao wrote:
>> On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
>>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <jhb@FreeBSD.org>
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>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
<504CEAE0.704@FreeBSD.org>
<CAJ-FndCuQz8mJwLMUM3j9rAfvkH3848U6t7wv-c=8YerTKUdOw@mail.gmail.com>
In-Reply-To: <CAJ-FndCuQz8mJwLMUM3j9rAfvkH3848U6t7wv-c=8YerTKUdOw@mail.gmail.com>
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 <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <jhb@freebsd.org> wrote:
>> On 9/9/12 11:03 AM, Attilio Rao wrote:
>>> On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
>>>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
<504CEAE0.704@FreeBSD.org>
Date: Mon, 10 Sep 2012 03:07:18 +0100
X-Google-Sender-Auth: wdDKbxzBEl-1ThevBYq-h3MtiEI
Message-ID: <CAJ-FndBNC-ZheSzJaujCSijXpVZAUEO8F6ZWHEzhFM9C=XvNgA@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Konstantin Belousov <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Mon, 10 Sep 2012 02:07:21 -0000
On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin <jhb@freebsd.org> wrote:
> On 9/9/12 11:03 AM, Attilio Rao wrote:
>> On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
>>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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 <attilio@pcbsd-2488.(none)>
---
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: <owner-svn-src-projects@FreeBSD.ORG>
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 <kostikbel@gmail.com>
To: Attilio Rao <attilio@freebsd.org>
Message-ID: <20120910093443.GT33100@deviant.kiev.zoral.com.ua>
References: <201207301350.q6UDobCI099069@svn.freebsd.org>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
<504CEAE0.704@FreeBSD.org>
<CAJ-FndBNC-ZheSzJaujCSijXpVZAUEO8F6ZWHEzhFM9C=XvNgA@mail.gmail.com>
Mime-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
protocol="application/pgp-signature"; boundary="1sp61FhDAWPvkXjV"
Content-Disposition: inline
In-Reply-To: <CAJ-FndBNC-ZheSzJaujCSijXpVZAUEO8F6ZWHEzhFM9C=XvNgA@mail.gmail.com>
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 <davide@freebsd.org>, svn-src-projects@freebsd.org,
src-committers@freebsd.org, John Baldwin <jhb@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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <jhb@freebsd.org> wrote:
> > On 9/9/12 11:03 AM, Attilio Rao wrote:
> >> On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
> >>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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 <attilio@pcbsd-2488.(none)>
> ---
> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <jhb@freebsd.org>
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>
<CAJ-FndBNC-ZheSzJaujCSijXpVZAUEO8F6ZWHEzhFM9C=XvNgA@mail.gmail.com>
In-Reply-To: <CAJ-FndBNC-ZheSzJaujCSijXpVZAUEO8F6ZWHEzhFM9C=XvNgA@mail.gmail.com>
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 <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <jhb@freebsd.org> wrote:
> > On 9/9/12 11:03 AM, Attilio Rao wrote:
> >> On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
> >>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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>
<CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com>
<CAJ-FndDnO7wjnWPV0tTu+UGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com>
<201207301732.33474.jhb@freebsd.org>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<CAJ-FndARWZGwdiLeaQcnM+M+m8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com>
<504CEAE0.704@FreeBSD.org>
<CAJ-FndCuQz8mJwLMUM3j9rAfvkH3848U6t7wv-c=8YerTKUdOw@mail.gmail.com>
<504CF1FB.9090106@FreeBSD.org>
Date: Tue, 11 Sep 2012 02:11:22 +0100
X-Google-Sender-Auth: MlTRqk5DrPKS5p-eX6RcYk9f3BM
Message-ID: <CAJ-FndCW91Y=SB3GFXDxtXGQdQzUzyF5KzKqjtuFYGs0W0-w6g@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Konstantin Belousov <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Tue, 11 Sep 2012 01:11:24 -0000
On Sun, Sep 9, 2012 at 8:46 PM, John Baldwin <jhb@freebsd.org> wrote:
> On 9/9/12 3:23 PM, Attilio Rao wrote:
>> On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin <jhb@freebsd.org> wrote:
>>> On 9/9/12 11:03 AM, Attilio Rao wrote:
>>>> On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
>>>>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <avg@FreeBSD.org>
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>
<CAJ-FndBJNNBNDUEDsDBUvwoVExZpnXmoJmpY58gE3QQbw3hRGA@mail.gmail.com>
<CACYV=-HmOwZ=E8Pw3-mUw0994SbvZaA3eMfcwM0fDTu_zykBJg@mail.gmail.com>
<CAJ-FndBmXkyJJ=fCkEpVm84E56A2_EoM6kbch03e4RMEM6WCGQ@mail.gmail.com>
<20120730143943.GY2676@deviant.kiev.zoral.com.ua>
<CAJ-FndByYcZ+UhnkFT_n2=W=UheqUCi0+UAX+F07EqbVU=6iDQ@mail.gmail.com>
<CAJ-FndCQ6HGAfFdjofNfJ+HeNaE7uqoNhJB9GH4pGFxyZ_1yLg@mail.gmail.com>
<5016A21B.6090409@FreeBSD.org>
<CAJ-FndCFjZP=0ThpMxy6WSDQAZOm0TRkyu0bWfxVBwtT-h+1cA@mail.gmail.com>
<5016A8E4.7070405@FreeBSD.org>
<CAJ-FndB0zc-TC0E=H4p1qcOB4ngEWtwXoyhScf68G8i0p5UErw@mail.gmail.com>
<CAJ-FndDAFmrEBW3d29cj-CZoPT+D5UPFadzL6i9BNH9ztzsJ+Q@mail.gmail.com>
In-Reply-To: <CAJ-FndDAFmrEBW3d29cj-CZoPT+D5UPFadzL6i9BNH9ztzsJ+Q@mail.gmail.com>
X-Enigmail-Version: 1.4.3
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Cc: Konstantin Belousov <kostikbel@gmail.com>,
Davide Italiano <davide@FreeBSD.org>,
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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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: <owner-svn-src-projects@FreeBSD.ORG>
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 <jhb@freebsd.org>
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>
<CAJ-FndCW91Y=SB3GFXDxtXGQdQzUzyF5KzKqjtuFYGs0W0-w6g@mail.gmail.com>
In-Reply-To: <CAJ-FndCW91Y=SB3GFXDxtXGQdQzUzyF5KzKqjtuFYGs0W0-w6g@mail.gmail.com>
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 <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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>
<CAJ-FndCW91Y=SB3GFXDxtXGQdQzUzyF5KzKqjtuFYGs0W0-w6g@mail.gmail.com>
<201209121511.42296.jhb@freebsd.org>
Date: Thu, 13 Sep 2012 01:07:11 +0100
X-Google-Sender-Auth: sd0s38_-7pamlOMnrfqgngWRC34
Message-ID: <CAJ-FndDnYE3CKNRajGn5YV4TBKDRCnX3=cgyT2yA9mxbnkangQ@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Konstantin Belousov <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 13 Sep 2012 00:07:13 -0000
On Wed, Sep 12, 2012 at 8:11 PM, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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>
<CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
<201208021707.22356.jhb@freebsd.org>
Date: Thu, 13 Sep 2012 02:36:58 +0100
X-Google-Sender-Auth: GBXXp1umUGoAAJzZB4SrVfIFetY
Message-ID: <CAJ-FndA8Yende_=-hgOMjfUkQVhaSdSjAb0W8xthqN1ThwT=Vg@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>, mlaier@freebsd.org,
Stephan Uphoff <ups@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Konstantin Belousov <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 13 Sep 2012 01:37:00 -0000
On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote:
> On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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 <attilio@pcbsd-2488.(none)>
---
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 <sys/pcpu.h>
/*
* 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <jhb@freebsd.org>
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>
<CAJ-FndA8Yende_=-hgOMjfUkQVhaSdSjAb0W8xthqN1ThwT=Vg@mail.gmail.com>
In-Reply-To: <CAJ-FndA8Yende_=-hgOMjfUkQVhaSdSjAb0W8xthqN1ThwT=Vg@mail.gmail.com>
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 <davide@freebsd.org>, mlaier@freebsd.org,
svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>,
src-committers@freebsd.org, Stephan Uphoff <ups@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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <jhb@freebsd.org> wrote:
> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
> >> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <jhb@freebsd.org>
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>
<CAJ-FndDnYE3CKNRajGn5YV4TBKDRCnX3=cgyT2yA9mxbnkangQ@mail.gmail.com>
In-Reply-To: <CAJ-FndDnYE3CKNRajGn5YV4TBKDRCnX3=cgyT2yA9mxbnkangQ@mail.gmail.com>
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 <kostikbel@gmail.com>,
Davide Italiano <davide@freebsd.org>, 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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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>
<CAJ-FndA8Yende_=-hgOMjfUkQVhaSdSjAb0W8xthqN1ThwT=Vg@mail.gmail.com>
<201209130910.50876.jhb@freebsd.org>
Date: Thu, 13 Sep 2012 15:38:54 +0100
X-Google-Sender-Auth: BAi4PyoHE3Kzhcgl9AuwIj1Q-co
Message-ID: <CAJ-FndASH1=i4ozwP=YepF58iC_5+nf4L4MCu3+2-xB9FVzyvg@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Davide Italiano <davide@freebsd.org>, mlaier@freebsd.org,
svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>,
src-committers@freebsd.org, Stephan Uphoff <ups@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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 13 Sep 2012 14:38:58 -0000
On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin <jhb@freebsd.org> wrote:
> On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote:
>> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote:
>> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>> >> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <jhb@freebsd.org>
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>
<CAJ-FndASH1=i4ozwP=YepF58iC_5+nf4L4MCu3+2-xB9FVzyvg@mail.gmail.com>
In-Reply-To: <CAJ-FndASH1=i4ozwP=YepF58iC_5+nf4L4MCu3+2-xB9FVzyvg@mail.gmail.com>
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 <davide@freebsd.org>, mlaier@freebsd.org,
svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>,
src-committers@freebsd.org, Stephan Uphoff <ups@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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <jhb@freebsd.org> wrote:
> > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote:
> >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote:
> >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
> >> >> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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>
<CAJ-FndASH1=i4ozwP=YepF58iC_5+nf4L4MCu3+2-xB9FVzyvg@mail.gmail.com>
<201209131132.21103.jhb@freebsd.org>
Date: Thu, 13 Sep 2012 17:20:54 +0100
X-Google-Sender-Auth: ZcL04eqF14HRlR4cERA2elyKGU0
Message-ID: <CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Davide Italiano <davide@freebsd.org>, mlaier@freebsd.org,
svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>,
src-committers@freebsd.org, Stephan Uphoff <ups@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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 13 Sep 2012 16:20:58 -0000
On 9/13/12, John Baldwin <jhb@freebsd.org> wrote:
> On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote:
>> On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin <jhb@freebsd.org> wrote:
>> > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote:
>> >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote:
>> >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>> >> >> On 7/30/12, John Baldwin <jhb@freebsd.org> 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <multiple recipients>; 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: <CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g@mail.gmail.com>
References: <201207301350.q6UDobCI099069@svn.freebsd.org>
<201209130910.50876.jhb@freebsd.org>
<CAJ-FndASH1=i4ozwP=YepF58iC_5+nf4L4MCu3+2-xB9FVzyvg@mail.gmail.com>
<201209131132.21103.jhb@freebsd.org>
<CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g@mail.gmail.com>
Date: Fri, 14 Sep 2012 23:32:35 +0100
X-Google-Sender-Auth: mx4oo8WVkKm-09_CQAOD5N_uyQ8
Message-ID: <CAJ-FndBvs1F+bXfvL-U2yTi313mebuZ6KidtDqh_CfchxX7dAg@mail.gmail.com>
From: Attilio Rao <attilio@freebsd.org>
To: John Baldwin <jhb@freebsd.org>
Content-Type: text/plain; charset=UTF-8
Cc: Davide Italiano <davide@freebsd.org>, mlaier@freebsd.org,
svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>,
src-committers@freebsd.org, Stephan Uphoff <ups@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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Fri, 14 Sep 2012 22:32:40 -0000
On Thu, Sep 13, 2012 at 5:20 PM, Attilio Rao <attilio@freebsd.org> wrote:
> On 9/13/12, John Baldwin <jhb@freebsd.org> wrote:
>> On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote:
>>> On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin <jhb@freebsd.org> wrote:
>>> > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote:
>>> >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote:
>>> >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>>> >> >> On 7/30/12, John Baldwin <jhb@freebsd.org> 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 <attilio@pcbsd-2488.(none)>
---
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 <sys/pcpu.h>
/*
* 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: <owner-svn-src-projects@FreeBSD.ORG>
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 <jhb@FreeBSD.org>
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>
<CAJ-FndASH1=i4ozwP=YepF58iC_5+nf4L4MCu3+2-xB9FVzyvg@mail.gmail.com>
<201209131132.21103.jhb@freebsd.org>
<CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g@mail.gmail.com>
<CAJ-FndBvs1F+bXfvL-U2yTi313mebuZ6KidtDqh_CfchxX7dAg@mail.gmail.com>
In-Reply-To: <CAJ-FndBvs1F+bXfvL-U2yTi313mebuZ6KidtDqh_CfchxX7dAg@mail.gmail.com>
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 <davide@freebsd.org>, mlaier@freebsd.org,
svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>,
src-committers@freebsd.org, Stephan Uphoff <ups@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" <svn-src-projects.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-projects>
List-Post: <mailto:svn-src-projects@freebsd.org>
List-Help: <mailto:svn-src-projects-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-projects>,
<mailto:svn-src-projects-request@freebsd.org?subject=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 <attilio@freebsd.org> wrote:
>> On 9/13/12, John Baldwin <jhb@freebsd.org> wrote:
>>> On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote:
>>>> On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin <jhb@freebsd.org> wrote:
>>>>> On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote:
>>>>>> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote:
>>>>>>> On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>>>>>>>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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