From owner-svn-src-projects@FreeBSD.ORG Mon Sep 10 09:34:53 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id F2EBC1065670; Mon, 10 Sep 2012 09:34:52 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id A97CD8FC0A; Mon, 10 Sep 2012 09:34:51 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q8A9Yukl014946; Mon, 10 Sep 2012 12:34:56 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q8A9YhJ2007114; Mon, 10 Sep 2012 12:34:43 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q8A9YhqS007113; Mon, 10 Sep 2012 12:34:43 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Mon, 10 Sep 2012 12:34:43 +0300 From: Konstantin Belousov To: Attilio Rao Message-ID: <20120910093443.GT33100@deviant.kiev.zoral.com.ua> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <504CEAE0.704@FreeBSD.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="1sp61FhDAWPvkXjV" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.0 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: Davide Italiano , svn-src-projects@freebsd.org, src-committers@freebsd.org, John Baldwin Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Sep 2012 09:34:53 -0000 --1sp61FhDAWPvkXjV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 10, 2012 at 03:07:18AM +0100, Attilio Rao wrote: > On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin wrote: > > On 9/9/12 11:03 AM, Attilio Rao wrote: > >> On 8/2/12, Attilio Rao wrote: > >>> On 7/30/12, John Baldwin wrote: > >> > >> [ trimm ] > >> > >>>> --- //depot/projects/smpng/sys/kern/subr_turnstile.c 2012-06-= 04 > >>>> 18:27:32.000000000 0000 > >>>> +++ //depot/user/jhb/lock/kern/subr_turnstile.c 2012-06-05 > >>>> 00:27:57.000000000 0000 > >>>> @@ -684,6 +684,7 @@ > >>>> if (owner) > >>>> MPASS(owner->td_proc->p_magic =3D=3D P_MAGIC); > >>>> MPASS(queue =3D=3D TS_SHARED_QUEUE || queue =3D=3D TS_EXCLUSIVE_= QUEUE); > >>>> + KASSERT(!TD_IS_IDLETHREAD(td), ("idle threads cannot block on lo= cks")); > >>>> > >>>> /* > >>>> * If the lock does not already have a turnstile, use this threa= d's > >>> > >>> I'm wondering if we should also use similar checks in places doing > >>> adaptive spinning (including the TD_NO_SLEEPING check). Likely yes. > >> > >> So what do you think about this? > > > > This is isn't really good enough then. An idle thread should not > > acquire any lock that isn't a spin lock. Instead, you would be > > better off removing the assert I added above and adding an assert to > > mtx_lock(), rw_{rw}lock(), sx_{sx}lock(), lockmgr(), rm_{rw}lock() and > > all the try variants of those. >=20 > What do you think about these then? For me, it is definitely an improvement for the usefulness of the messages. >=20 > Attilio >=20 >=20 > Subject: [PATCH 1/3] Tweak the commit message in case of panic for sleepi= ng > from threads with TDP_NOSLEEPING on. >=20 > The current message has no informations on the thread and wchan > involed, which may prove to be useful in case where dumps have > mangled dwarf informations. >=20 > Reported by: kib > --- > sys/kern/subr_sleepqueue.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) >=20 > diff --git a/sys/kern/subr_sleepqueue.c b/sys/kern/subr_sleepqueue.c > index b868289..69e0d36 100644 > --- a/sys/kern/subr_sleepqueue.c > +++ b/sys/kern/subr_sleepqueue.c > @@ -297,7 +297,8 @@ sleepq_add(void *wchan, struct lock_object *lock, > const char *wmesg, int flags, >=20 > /* If this thread is not allowed to sleep, die a horrible death. = */ > KASSERT(!(td->td_pflags & TDP_NOSLEEPING), > - ("Trying sleep, but thread marked as sleeping prohibited")); > + ("sleepq_add: td(%p) to sleep on wchan(%p) with TDP_NOSLEEPIN= G on", > + td, wchan)); >=20 > /* Look up the sleep queue associated with the wait channel 'wcha= n'. */ > sq =3D sleepq_lookup(wchan); > --=20 > 1.7.7.4 >=20 > Subject: [PATCH 2/3] Tweak comments. >=20 > - Remove a sporious "with" > - Remove a comment which is no-longer true > --- > sys/kern/subr_trap.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) >=20 > diff --git a/sys/kern/subr_trap.c b/sys/kern/subr_trap.c > index 24960fd..13d273d1 100644 > --- a/sys/kern/subr_trap.c > +++ b/sys/kern/subr_trap.c > @@ -153,7 +153,7 @@ userret(struct thread *td, struct trapframe *frame) > KASSERT((td->td_pflags & TDP_NOSLEEPING) =3D=3D 0, > ("userret: Returning with sleep disabled")); > KASSERT(td->td_pinned =3D=3D 0, > - ("userret: Returning with with pinned thread")); > + ("userret: Returning with pinned thread")); > #ifdef VIMAGE > /* Unfortunately td_vnet_lpush needs VNET_DEBUG. */ > VNET_ASSERT(curvnet =3D=3D NULL, > @@ -166,7 +166,6 @@ userret(struct thread *td, struct trapframe *frame) > /* > * Process an asynchronous software trap. > * This is relatively easy. > - * This function will return with preemption disabled. > */ > void > ast(struct trapframe *framep) > --=20 > 1.7.7.4 >=20 > Subject: [PATCH 3/3] Improve check coverage about idle threads. >=20 > Idle threads are not allowed to acquire any lock but spinlocks. > Deny any attempt to do so by panicing at the locking operation > when INVARIANTS is on. >=20 > Discussed with: jhb >=20 > Signed-off-by: Attilio Rao > --- > sys/kern/kern_lock.c | 4 ++++ > sys/kern/kern_mutex.c | 6 ++++++ > sys/kern/kern_rmlock.c | 6 ++++++ > sys/kern/kern_rwlock.c | 13 +++++++++++++ > sys/kern/kern_sx.c | 13 +++++++++++++ > sys/kern/subr_turnstile.c | 1 - > 6 files changed, 42 insertions(+), 1 deletions(-) >=20 > diff --git a/sys/kern/kern_lock.c b/sys/kern/kern_lock.c > index 24526b0..46a7567 100644 > --- a/sys/kern/kern_lock.c > +++ b/sys/kern/kern_lock.c > @@ -478,6 +478,10 @@ __lockmgr_args(struct lock *lk, u_int flags, > struct lock_object *ilk, > ("%s: LK_INTERLOCK passed without valid interlock @ %s:%d", > __func__, file, line)); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("lockmgr() by idle thread %p on lockmgr %s @ %s:%d", > + curthread, lk->lock_object.lo_name, file, line)); > + > class =3D (flags & LK_INTERLOCK) ? LOCK_CLASS(ilk) : NULL; > if (panicstr !=3D NULL) { > if (flags & LK_INTERLOCK) > diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c > index f718ca0..9827a9f 100644 > --- a/sys/kern/kern_mutex.c > +++ b/sys/kern/kern_mutex.c > @@ -201,6 +201,9 @@ _mtx_lock_flags(struct mtx *m, int opts, const > char *file, int line) > if (SCHEDULER_STOPPED()) > return; > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("mtx_lock() by idle thread %p on sleep mutex %s @ %s:%d", > + curthread, m->lock_object.lo_name, file, line)); > KASSERT(m->mtx_lock !=3D MTX_DESTROYED, > ("mtx_lock() of destroyed mutex @ %s:%d", file, line)); > KASSERT(LOCK_CLASS(&m->lock_object) =3D=3D &lock_class_mtx_sleep, > @@ -301,6 +304,9 @@ mtx_trylock_flags_(struct mtx *m, int opts, const > char *file, int line) > return (1); >=20 > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("mtx_trylock() by idle thread %p on sleep mutex %s @ %s:%d", > + curthread, m->lock_object.lo_name, file, line)); > KASSERT(m->mtx_lock !=3D MTX_DESTROYED, > ("mtx_trylock() of destroyed mutex @ %s:%d", file, line)); > KASSERT(LOCK_CLASS(&m->lock_object) =3D=3D &lock_class_mtx_sleep, > diff --git a/sys/kern/kern_rmlock.c b/sys/kern/kern_rmlock.c > index 27d0462..ef1920b 100644 > --- a/sys/kern/kern_rmlock.c > +++ b/sys/kern/kern_rmlock.c > @@ -498,6 +498,9 @@ void _rm_wlock_debug(struct rmlock *rm, const char > *file, int line) > if (SCHEDULER_STOPPED()) > return; >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rm_wlock() by idle thread %p on rmlock %s @ %s:%d", > + curthread, rm->lock_object.lo_name, file, line)); > WITNESS_CHECKORDER(&rm->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE, > file, line, NULL); >=20 > @@ -540,6 +543,9 @@ _rm_rlock_debug(struct rmlock *rm, struct > rm_priotracker *tracker, > if (SCHEDULER_STOPPED()) > return (1); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rm_rlock() by idle thread %p on rmlock %s @ %s:%d", > + curthread, rm->lock_object.lo_name, file, line)); > if (!trylock && (rm->lock_object.lo_flags & RM_SLEEPABLE)) > WITNESS_CHECKORDER(&rm->rm_lock_sx.lock_object, LOP_NEWOR= DER, > file, line, NULL); > diff --git a/sys/kern/kern_rwlock.c b/sys/kern/kern_rwlock.c > index c337041..e0be154 100644 > --- a/sys/kern/kern_rwlock.c > +++ b/sys/kern/kern_rwlock.c > @@ -242,6 +242,9 @@ _rw_wlock(struct rwlock *rw, const char *file, int li= ne) > if (SCHEDULER_STOPPED()) > return; > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rw_wlock() by idle thread %p on rwlock %s @ %s:%d", > + curthread, rw->lock_object.lo_name, file, line)); > KASSERT(rw->rw_lock !=3D RW_DESTROYED, > ("rw_wlock() of destroyed rwlock @ %s:%d", file, line)); > WITNESS_CHECKORDER(&rw->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE= , file, > @@ -260,6 +263,9 @@ _rw_try_wlock(struct rwlock *rw, const char *file, in= t line) > if (SCHEDULER_STOPPED()) > return (1); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rw_try_wlock() by idle thread %p on rwlock %s @ %s:%d", > + curthread, rw->lock_object.lo_name, file, line)); > KASSERT(rw->rw_lock !=3D RW_DESTROYED, > ("rw_try_wlock() of destroyed rwlock @ %s:%d", file, line)); >=20 > @@ -333,6 +339,9 @@ _rw_rlock(struct rwlock *rw, const char *file, int li= ne) > if (SCHEDULER_STOPPED()) > return; >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rw_rlock() by idle thread %p on rwlock %s @ %s:%d", > + curthread, rw->lock_object.lo_name, file, line)); > KASSERT(rw->rw_lock !=3D RW_DESTROYED, > ("rw_rlock() of destroyed rwlock @ %s:%d", file, line)); > KASSERT(rw_wowner(rw) !=3D curthread, > @@ -521,6 +530,10 @@ _rw_try_rlock(struct rwlock *rw, const char > *file, int line) > if (SCHEDULER_STOPPED()) > return (1); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("rw_try_rlock() by idle thread %p on rwlock %s @ %s:%d", > + curthread, rw->lock_object.lo_name, file, line)); > + > for (;;) { > x =3D rw->rw_lock; > KASSERT(rw->rw_lock !=3D RW_DESTROYED, > diff --git a/sys/kern/kern_sx.c b/sys/kern/kern_sx.c > index bcd7acd..487a324 100644 > --- a/sys/kern/kern_sx.c > +++ b/sys/kern/kern_sx.c > @@ -250,6 +250,9 @@ _sx_slock(struct sx *sx, int opts, const char > *file, int line) > if (SCHEDULER_STOPPED()) > return (0); > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("sx_slock() by idle thread %p on sx %s @ %s:%d", > + curthread, sx->lock_object.lo_name, file, line)); > KASSERT(sx->sx_lock !=3D SX_LOCK_DESTROYED, > ("sx_slock() of destroyed sx @ %s:%d", file, line)); > WITNESS_CHECKORDER(&sx->lock_object, LOP_NEWORDER, file, line, NU= LL); > @@ -271,6 +274,10 @@ sx_try_slock_(struct sx *sx, const char *file, int l= ine) > if (SCHEDULER_STOPPED()) > return (1); >=20 > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("sx_try_slock() by idle thread %p on sx %s @ %s:%d", > + curthread, sx->lock_object.lo_name, file, line)); > + > for (;;) { > x =3D sx->sx_lock; > KASSERT(x !=3D SX_LOCK_DESTROYED, > @@ -297,6 +304,9 @@ _sx_xlock(struct sx *sx, int opts, const char > *file, int line) > if (SCHEDULER_STOPPED()) > return (0); > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("sx_xlock() by idle thread %p on sx %s @ %s:%d", > + curthread, sx->lock_object.lo_name, file, line)); > KASSERT(sx->sx_lock !=3D SX_LOCK_DESTROYED, > ("sx_xlock() of destroyed sx @ %s:%d", file, line)); > WITNESS_CHECKORDER(&sx->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE= , file, > @@ -321,6 +331,9 @@ sx_try_xlock_(struct sx *sx, const char *file, int li= ne) > return (1); >=20 > MPASS(curthread !=3D NULL); > + KASSERT(!TD_IS_IDLETHREAD(curthread), > + ("sx_try_xlock() by idle thread %p on sx %s @ %s:%d", > + curthread, sx->lock_object.lo_name, file, line)); > KASSERT(sx->sx_lock !=3D SX_LOCK_DESTROYED, > ("sx_try_xlock() of destroyed sx @ %s:%d", file, line)); >=20 > diff --git a/sys/kern/subr_turnstile.c b/sys/kern/subr_turnstile.c > index 31d16fe..76fb964 100644 > --- a/sys/kern/subr_turnstile.c > +++ b/sys/kern/subr_turnstile.c > @@ -684,7 +684,6 @@ turnstile_wait(struct turnstile *ts, struct thread > *owner, int queue) > if (owner) > MPASS(owner->td_proc->p_magic =3D=3D P_MAGIC); > MPASS(queue =3D=3D TS_SHARED_QUEUE || queue =3D=3D TS_EXCLUSIVE_Q= UEUE); > - KASSERT(!TD_IS_IDLETHREAD(td), ("idle threads cannot block on loc= ks")); >=20 > /* > * If the lock does not already have a turnstile, use this thread= 's > --=20 > 1.7.7.4 --1sp61FhDAWPvkXjV Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlBNtDMACgkQC3+MBN1Mb4gOugCg0tJCsDgcB1e687rhl2a2IdEA X2oAn3l/g9rAfUTZ2xVrrrAcLumGlOly =ZaJ2 -----END PGP SIGNATURE----- --1sp61FhDAWPvkXjV--