Date: Mon, 10 Sep 2012 12:34:43 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Attilio Rao <attilio@freebsd.org> 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 Message-ID: <20120910093443.GT33100@deviant.kiev.zoral.com.ua> In-Reply-To: <CAJ-FndBNC-ZheSzJaujCSijXpVZAUEO8F6ZWHEzhFM9C=XvNgA@mail.gmail.com> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com> <CAJ-FndDnO7wjnWPV0tTu%2BUGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com> <201207301732.33474.jhb@freebsd.org> <CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com> <CAJ-FndARWZGwdiLeaQcnM%2BM%2Bm8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com> <504CEAE0.704@FreeBSD.org> <CAJ-FndBNC-ZheSzJaujCSijXpVZAUEO8F6ZWHEzhFM9C=XvNgA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120910093443.GT33100>