From owner-svn-src-projects@FreeBSD.ORG Thu Aug 2 21:07:46 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 7AA4C106566B; Thu, 2 Aug 2012 21:07:46 +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 2126E8FC08; Thu, 2 Aug 2012 21:07:46 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 8A92EB94B; Thu, 2 Aug 2012 17:07:45 -0400 (EDT) From: John Baldwin To: attilio@freebsd.org Date: Thu, 2 Aug 2012 17:07:22 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p17; KDE/4.5.5; amd64; ; ) References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201208021707.22356.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 02 Aug 2012 17:07:45 -0400 (EDT) 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 List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Aug 2012 21:07:46 -0000 On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: > On 7/30/12, John Baldwin 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