From owner-svn-src-projects@FreeBSD.ORG Mon Sep 10 02:07:21 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 82BE9106564A; Mon, 10 Sep 2012 02:07:21 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lpp01m010-f54.google.com (mail-lpp01m010-f54.google.com [209.85.215.54]) by mx1.freebsd.org (Postfix) with ESMTP id 5B9D68FC0A; Mon, 10 Sep 2012 02:07:20 +0000 (UTC) Received: by lage12 with SMTP id e12so1183113lag.13 for ; Sun, 09 Sep 2012 19:07:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=OXl6eQlHPF32Ca/51Syf3ubjD0hhmDYw+E51UXHhoFM=; b=AMS2R3HGKhe6tqIhzepJ3ceIq9/HizEVUixFzgoNx96OAsz5gXZWEGfirBmkeKSRL9 iHkyw8tPcKnLFcK/Lu9Yn2z/XqUMAf7uwWf8Z+2odCPMbQPB5FmVvxZVB3/wH8snS1Wq c4d0Zy5XUK4O7pmXkDKcokM9YuswA7nOeevPeoDFWZ+etJzeGJ/m7Ux1Ii6PGuNSrZeC 1WQ7lhy3wi4g6nu5h8uLqhcC0ubIeuntcucILzRarLXS/F+08i3LtP4CDqHjbLwVeG8a Xk6hTXrkclDVkGMeiFWMFlAtTp+TadgIygFcAdRATcmw0x+GtgwyBvxU545l4ig7I7kD vLfQ== MIME-Version: 1.0 Received: by 10.152.112.233 with SMTP id it9mr10923302lab.40.1347242838935; Sun, 09 Sep 2012 19:07:18 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Sun, 9 Sep 2012 19:07:18 -0700 (PDT) In-Reply-To: <504CEAE0.704@FreeBSD.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <504CEAE0.704@FreeBSD.org> Date: Mon, 10 Sep 2012 03:07:18 +0100 X-Google-Sender-Auth: wdDKbxzBEl-1ThevBYq-h3MtiEI Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Cc: Konstantin Belousov , Davide Italiano , src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Sep 2012 02:07:21 -0000 On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin wrote: > On 9/9/12 11:03 AM, Attilio Rao wrote: >> On 8/2/12, Attilio Rao wrote: >>> On 7/30/12, John Baldwin wrote: >> >> [ trimm ] >> >>>> --- //depot/projects/smpng/sys/kern/subr_turnstile.c 2012-06-04 >>>> 18:27:32.000000000 0000 >>>> +++ //depot/user/jhb/lock/kern/subr_turnstile.c 2012-06-05 >>>> 00:27:57.000000000 0000 >>>> @@ -684,6 +684,7 @@ >>>> if (owner) >>>> MPASS(owner->td_proc->p_magic == P_MAGIC); >>>> MPASS(queue == TS_SHARED_QUEUE || queue == TS_EXCLUSIVE_QUEUE); >>>> + KASSERT(!TD_IS_IDLETHREAD(td), ("idle threads cannot block on locks")); >>>> >>>> /* >>>> * If the lock does not already have a turnstile, use this thread's >>> >>> I'm wondering if we should also use similar checks in places doing >>> adaptive spinning (including the TD_NO_SLEEPING check). Likely yes. >> >> So what do you think about this? > > This is isn't really good enough then. An idle thread should not > acquire any lock that isn't a spin lock. Instead, you would be > better off removing the assert I added above and adding an assert to > mtx_lock(), rw_{rw}lock(), sx_{sx}lock(), lockmgr(), rm_{rw}lock() and > all the try variants of those. What do you think about these then? Attilio Subject: [PATCH 1/3] Tweak the commit message in case of panic for sleeping from threads with TDP_NOSLEEPING on. The current message has no informations on the thread and wchan involed, which may prove to be useful in case where dumps have mangled dwarf informations. Reported by: kib --- sys/kern/subr_sleepqueue.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/sys/kern/subr_sleepqueue.c b/sys/kern/subr_sleepqueue.c index b868289..69e0d36 100644 --- a/sys/kern/subr_sleepqueue.c +++ b/sys/kern/subr_sleepqueue.c @@ -297,7 +297,8 @@ sleepq_add(void *wchan, struct lock_object *lock, const char *wmesg, int flags, /* If this thread is not allowed to sleep, die a horrible death. */ KASSERT(!(td->td_pflags & TDP_NOSLEEPING), - ("Trying sleep, but thread marked as sleeping prohibited")); + ("sleepq_add: td(%p) to sleep on wchan(%p) with TDP_NOSLEEPING on", + td, wchan)); /* Look up the sleep queue associated with the wait channel 'wchan'. */ sq = sleepq_lookup(wchan); -- 1.7.7.4 Subject: [PATCH 2/3] Tweak comments. - Remove a sporious "with" - Remove a comment which is no-longer true --- sys/kern/subr_trap.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/sys/kern/subr_trap.c b/sys/kern/subr_trap.c index 24960fd..13d273d1 100644 --- a/sys/kern/subr_trap.c +++ b/sys/kern/subr_trap.c @@ -153,7 +153,7 @@ userret(struct thread *td, struct trapframe *frame) KASSERT((td->td_pflags & TDP_NOSLEEPING) == 0, ("userret: Returning with sleep disabled")); KASSERT(td->td_pinned == 0, - ("userret: Returning with with pinned thread")); + ("userret: Returning with pinned thread")); #ifdef VIMAGE /* Unfortunately td_vnet_lpush needs VNET_DEBUG. */ VNET_ASSERT(curvnet == NULL, @@ -166,7 +166,6 @@ userret(struct thread *td, struct trapframe *frame) /* * Process an asynchronous software trap. * This is relatively easy. - * This function will return with preemption disabled. */ void ast(struct trapframe *framep) -- 1.7.7.4 Subject: [PATCH 3/3] Improve check coverage about idle threads. Idle threads are not allowed to acquire any lock but spinlocks. Deny any attempt to do so by panicing at the locking operation when INVARIANTS is on. Discussed with: jhb Signed-off-by: Attilio Rao --- sys/kern/kern_lock.c | 4 ++++ sys/kern/kern_mutex.c | 6 ++++++ sys/kern/kern_rmlock.c | 6 ++++++ sys/kern/kern_rwlock.c | 13 +++++++++++++ sys/kern/kern_sx.c | 13 +++++++++++++ sys/kern/subr_turnstile.c | 1 - 6 files changed, 42 insertions(+), 1 deletions(-) diff --git a/sys/kern/kern_lock.c b/sys/kern/kern_lock.c index 24526b0..46a7567 100644 --- a/sys/kern/kern_lock.c +++ b/sys/kern/kern_lock.c @@ -478,6 +478,10 @@ __lockmgr_args(struct lock *lk, u_int flags, struct lock_object *ilk, ("%s: LK_INTERLOCK passed without valid interlock @ %s:%d", __func__, file, line)); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("lockmgr() by idle thread %p on lockmgr %s @ %s:%d", + curthread, lk->lock_object.lo_name, file, line)); + class = (flags & LK_INTERLOCK) ? LOCK_CLASS(ilk) : NULL; if (panicstr != NULL) { if (flags & LK_INTERLOCK) diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c index f718ca0..9827a9f 100644 --- a/sys/kern/kern_mutex.c +++ b/sys/kern/kern_mutex.c @@ -201,6 +201,9 @@ _mtx_lock_flags(struct mtx *m, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return; MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("mtx_lock() by idle thread %p on sleep mutex %s @ %s:%d", + curthread, m->lock_object.lo_name, file, line)); KASSERT(m->mtx_lock != MTX_DESTROYED, ("mtx_lock() of destroyed mutex @ %s:%d", file, line)); KASSERT(LOCK_CLASS(&m->lock_object) == &lock_class_mtx_sleep, @@ -301,6 +304,9 @@ mtx_trylock_flags_(struct mtx *m, int opts, const char *file, int line) return (1); MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("mtx_trylock() by idle thread %p on sleep mutex %s @ %s:%d", + curthread, m->lock_object.lo_name, file, line)); KASSERT(m->mtx_lock != MTX_DESTROYED, ("mtx_trylock() of destroyed mutex @ %s:%d", file, line)); KASSERT(LOCK_CLASS(&m->lock_object) == &lock_class_mtx_sleep, diff --git a/sys/kern/kern_rmlock.c b/sys/kern/kern_rmlock.c index 27d0462..ef1920b 100644 --- a/sys/kern/kern_rmlock.c +++ b/sys/kern/kern_rmlock.c @@ -498,6 +498,9 @@ void _rm_wlock_debug(struct rmlock *rm, const char *file, int line) if (SCHEDULER_STOPPED()) return; + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rm_wlock() by idle thread %p on rmlock %s @ %s:%d", + curthread, rm->lock_object.lo_name, file, line)); WITNESS_CHECKORDER(&rm->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE, file, line, NULL); @@ -540,6 +543,9 @@ _rm_rlock_debug(struct rmlock *rm, struct rm_priotracker *tracker, if (SCHEDULER_STOPPED()) return (1); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rm_rlock() by idle thread %p on rmlock %s @ %s:%d", + curthread, rm->lock_object.lo_name, file, line)); if (!trylock && (rm->lock_object.lo_flags & RM_SLEEPABLE)) WITNESS_CHECKORDER(&rm->rm_lock_sx.lock_object, LOP_NEWORDER, file, line, NULL); diff --git a/sys/kern/kern_rwlock.c b/sys/kern/kern_rwlock.c index c337041..e0be154 100644 --- a/sys/kern/kern_rwlock.c +++ b/sys/kern/kern_rwlock.c @@ -242,6 +242,9 @@ _rw_wlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return; MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rw_wlock() by idle thread %p on rwlock %s @ %s:%d", + curthread, rw->lock_object.lo_name, file, line)); KASSERT(rw->rw_lock != RW_DESTROYED, ("rw_wlock() of destroyed rwlock @ %s:%d", file, line)); WITNESS_CHECKORDER(&rw->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE, file, @@ -260,6 +263,9 @@ _rw_try_wlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return (1); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rw_try_wlock() by idle thread %p on rwlock %s @ %s:%d", + curthread, rw->lock_object.lo_name, file, line)); KASSERT(rw->rw_lock != RW_DESTROYED, ("rw_try_wlock() of destroyed rwlock @ %s:%d", file, line)); @@ -333,6 +339,9 @@ _rw_rlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return; + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rw_rlock() by idle thread %p on rwlock %s @ %s:%d", + curthread, rw->lock_object.lo_name, file, line)); KASSERT(rw->rw_lock != RW_DESTROYED, ("rw_rlock() of destroyed rwlock @ %s:%d", file, line)); KASSERT(rw_wowner(rw) != curthread, @@ -521,6 +530,10 @@ _rw_try_rlock(struct rwlock *rw, const char *file, int line) if (SCHEDULER_STOPPED()) return (1); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("rw_try_rlock() by idle thread %p on rwlock %s @ %s:%d", + curthread, rw->lock_object.lo_name, file, line)); + for (;;) { x = rw->rw_lock; KASSERT(rw->rw_lock != RW_DESTROYED, diff --git a/sys/kern/kern_sx.c b/sys/kern/kern_sx.c index bcd7acd..487a324 100644 --- a/sys/kern/kern_sx.c +++ b/sys/kern/kern_sx.c @@ -250,6 +250,9 @@ _sx_slock(struct sx *sx, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return (0); MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("sx_slock() by idle thread %p on sx %s @ %s:%d", + curthread, sx->lock_object.lo_name, file, line)); KASSERT(sx->sx_lock != SX_LOCK_DESTROYED, ("sx_slock() of destroyed sx @ %s:%d", file, line)); WITNESS_CHECKORDER(&sx->lock_object, LOP_NEWORDER, file, line, NULL); @@ -271,6 +274,10 @@ sx_try_slock_(struct sx *sx, const char *file, int line) if (SCHEDULER_STOPPED()) return (1); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("sx_try_slock() by idle thread %p on sx %s @ %s:%d", + curthread, sx->lock_object.lo_name, file, line)); + for (;;) { x = sx->sx_lock; KASSERT(x != SX_LOCK_DESTROYED, @@ -297,6 +304,9 @@ _sx_xlock(struct sx *sx, int opts, const char *file, int line) if (SCHEDULER_STOPPED()) return (0); MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("sx_xlock() by idle thread %p on sx %s @ %s:%d", + curthread, sx->lock_object.lo_name, file, line)); KASSERT(sx->sx_lock != SX_LOCK_DESTROYED, ("sx_xlock() of destroyed sx @ %s:%d", file, line)); WITNESS_CHECKORDER(&sx->lock_object, LOP_NEWORDER | LOP_EXCLUSIVE, file, @@ -321,6 +331,9 @@ sx_try_xlock_(struct sx *sx, const char *file, int line) return (1); MPASS(curthread != NULL); + KASSERT(!TD_IS_IDLETHREAD(curthread), + ("sx_try_xlock() by idle thread %p on sx %s @ %s:%d", + curthread, sx->lock_object.lo_name, file, line)); KASSERT(sx->sx_lock != SX_LOCK_DESTROYED, ("sx_try_xlock() of destroyed sx @ %s:%d", file, line)); diff --git a/sys/kern/subr_turnstile.c b/sys/kern/subr_turnstile.c index 31d16fe..76fb964 100644 --- a/sys/kern/subr_turnstile.c +++ b/sys/kern/subr_turnstile.c @@ -684,7 +684,6 @@ turnstile_wait(struct turnstile *ts, struct thread *owner, int queue) if (owner) MPASS(owner->td_proc->p_magic == P_MAGIC); MPASS(queue == TS_SHARED_QUEUE || queue == TS_EXCLUSIVE_QUEUE); - KASSERT(!TD_IS_IDLETHREAD(td), ("idle threads cannot block on locks")); /* * If the lock does not already have a turnstile, use this thread's -- 1.7.7.4