Date: Thu, 2 Aug 2012 17:07:22 -0400 From: John Baldwin <jhb@freebsd.org> To: attilio@freebsd.org 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 Message-ID: <201208021707.22356.jhb@freebsd.org> In-Reply-To: <CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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_lock.c 2012-06-04
> > 18:27:32.000000000 0000
> > +++ //depot/user/jhb/lock/kern/kern_lock.c 2012-06-18 14:44:48.000000000
> > 0000
> > @@ -394,12 +394,12 @@
> > iflags |= LO_QUIET;
> > iflags |= flags & (LK_ADAPTIVE | LK_NOSHARE);
> >
> > + lock_init(&lk->lock_object, &lock_class_lockmgr, wmesg, NULL, iflags);
> > lk->lk_lock = LK_UNLOCKED;
> > lk->lk_recurse = 0;
> > lk->lk_exslpfail = 0;
> > lk->lk_timo = timo;
> > lk->lk_pri = pri;
> > - lock_init(&lk->lock_object, &lock_class_lockmgr, wmesg, NULL, iflags);
> > STACK_ZERO(lk);
> > }
>
> I'm not sure, why these reshuffling are needed?
This is related to another set of changes in the tree. I want to have the
panic for a duplicate initialization of a lock to panic right away so you can
see what the "old" state of the lock was.
> > --- //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.
> > --- //depot/projects/smpng/sys/kern/subr_sleepqueue.c 2012-06-04
> > 18:27:32.000000000 0000
> > +++ //depot/user/jhb/lock/kern/subr_sleepqueue.c 2012-06-05
> > 14:46:23.000000000 0000
> > @@ -296,7 +296,7 @@
> > MPASS((queue >= 0) && (queue < NR_SLEEPQS));
> >
> > /* If this thread is not allowed to sleep, die a horrible death. */
> > - KASSERT(!(td->td_pflags & TDP_NOSLEEPING),
> > + KASSERT(td->td_no_sleeping == 0,
> > ("Trying sleep, but thread marked as sleeping prohibited"));
> >
> > /* Look up the sleep queue associated with the wait channel 'wchan'. */
>
> Do we want to complete the TDP_NOSLEEPING support by also offering a
> macro for the check? Maybe as a separate patch.
Humm, we don't check it in many places, not sure we need a separate flag.
We don't have one for checking td_pinned for example.
> > --- //depot/projects/smpng/sys/kern/subr_syscall.c 2012-06-04
> > 18:27:32.000000000 0000
> > +++ //depot/user/jhb/lock/kern/subr_syscall.c 2012-06-05 14:46:23.000000000
> > 0000
> > @@ -185,9 +185,12 @@
> > KASSERT((td->td_pflags & TDP_NOFAULTING) == 0,
> > ("System call %s returning with pagefaults disabled",
> > syscallname(p, sa->code)));
> > - KASSERT((td->td_pflags & TDP_NOSLEEPING) == 0,
> > + KASSERT(td->td_no_sleeping == 0,
> > ("System call %s returning with sleep disabled",
> > syscallname(p, sa->code)));
> > + KASSERT(td->td_pinned == 0,
> > + ("System call %s returning with pinned thread",
> > + syscallname(p, sa->code)));
> >
> > /*
> > * Handle reschedule and other end-of-syscall issues
>
> Can you also add CRITICAL_ASSERT()?
It's earler in the file already as:
KASSERT(td->td_critnest == 0,
("System call %s returning in a critical section",
syscallname(p, sa->code)));
(More readable panic message than CRITICAL_ASSERT(), but I think in practice
this check just predated CRITICAL_ASSERT()).
> > --- //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.
Hmm, possibly.
> > --- //depot/projects/smpng/sys/sys/_rmlock.h 2011-06-20 00:58:40.000000000
> > 0000
> > +++ //depot/user/jhb/lock/sys/_rmlock.h 2012-06-05 01:54:51.000000000 0000
> > @@ -44,14 +44,17 @@
> > LIST_HEAD(rmpriolist,rm_priotracker);
> >
> > struct rmlock {
> > - struct lock_object lock_object;
> > + struct lock_object lock_object;
> > volatile cpuset_t rm_writecpus;
> > LIST_HEAD(,rm_priotracker) rm_activeReaders;
> > union {
> > + struct lock_object _rm_wlock_object;
> > struct mtx _rm_lock_mtx;
> > struct sx _rm_lock_sx;
> > } _rm_lock;
> > };
> > +
> > +#define rm_wlock_object _rm_lock._rm_wlock_object
> > #define rm_lock_mtx _rm_lock._rm_lock_mtx
> > #define rm_lock_sx _rm_lock._rm_lock_sx
>
> What is the point of keeping both the mtx and the rwlock?
There is no rwlock? There is a mutex (used for "normal" rmlocks) and
an sx lock (used for RM_SLEEPABLE rmlocks). For some of the the lock_class
methods, I want to forward a request on to the underlying write lock.
Having rm_wlock_object makes that a bit simpler removing the need for
a branch as I'm just going to invoke a lock_class method of the underlying
lock anyway.
> > --- //depot/projects/smpng/sys/sys/cpuset.h 2012-03-25 18:45:29.000000000
> > 0000
> > +++ //depot/user/jhb/lock/sys/cpuset.h 2012-06-18 21:20:58.000000000 0000
> > @@ -216,6 +216,9 @@
> > int cpusetobj_ffs(const cpuset_t *);
> > char *cpusetobj_strprint(char *, const cpuset_t *);
> > int cpusetobj_strscan(cpuset_t *, const char *);
> > +#ifdef DDB
> > +void ddb_display_cpuset(const cpuset_t *);
> > +#endif
> >
> > #else
> > __BEGIN_DECLS
>
> I'd prefer you offer a compat stub in order to avoid a KPI breakage
> rather than make an header dependent by DDB. If it was INVARIANTS
> dependant you may have user INVARIANT_SUPPORT for this, but I guess we
> just use that for INVARIANTS and not DDB (even if we could generalize
> it to, maybe under another name, to all the debugging mechanisms).
Humm. Do we do that anywhere else for DDB (provide empty shims)? I think
you already can't use a module that depends on DDB if the kernel doesn't have
DDB enabled (no db_printf() for example), so I don't think this introduces any
new KPI breakage.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201208021707.22356.jhb>