Date: Thu, 2 Aug 2012 21:56:03 +0100 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@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: <CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com> In-Reply-To: <201207301732.33474.jhb@freebsd.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On 7/30/12, John Baldwin <jhb@freebsd.org> wrote:
> On Monday, July 30, 2012 5:00:20 pm Attilio Rao wrote:
[ trimm ]
>> Right now some sort of similar check is enforced by WITNESS, but I can
>> see a value in cases where you want to test a kernel with INVARIANTS
>> but without WITNESS (it is not a matter of performance, it is just
>> that sometimes you cannot reproduce some specific races with WITNESS
>> on, while you can do it with WITNESS off, so it is funny to note how
>> sometimes WITNESS should be just dropped for some locking issues).
>
> No, it's to fix the constraint for RM_SLEEPABLE locks. The larger patch
> containing it is below. I still need to test it though.
>
> --- //depot/projects/smpng/sys/kern/kern_cpuset.c 2012-03-25
> 18:45:29.000000000 0000
> +++ //depot/user/jhb/lock/kern/kern_cpuset.c 2012-06-18 21:20:58.000000000
> 0000
> @@ -1147,25 +1147,34 @@
> }
>
> #ifdef DDB
> +void
> +ddb_display_cpuset(const cpuset_t *set)
> +{
> + int cpu, once;
> +
> + for (once = 0, cpu = 0; cpu < CPU_SETSIZE; cpu++) {
> + if (CPU_ISSET(cpu, set)) {
> + if (once == 0) {
> + db_printf("%d", cpu);
> + once = 1;
> + } else
> + db_printf(",%d", cpu);
> + }
> + }
> + if (once == 0)
> + db_printf("<none>");
> +}
> +
> DB_SHOW_COMMAND(cpusets, db_show_cpusets)
> {
> struct cpuset *set;
> - int cpu, once;
>
> LIST_FOREACH(set, &cpuset_ids, cs_link) {
> db_printf("set=%p id=%-6u ref=%-6d flags=0x%04x parent id=%d\n",
> set, set->cs_id, set->cs_ref, set->cs_flags,
> (set->cs_parent != NULL) ? set->cs_parent->cs_id : 0);
> db_printf(" mask=");
> - for (once = 0, cpu = 0; cpu < CPU_SETSIZE; cpu++) {
> - if (CPU_ISSET(cpu, &set->cs_mask)) {
> - if (once == 0) {
> - db_printf("%d", cpu);
> - once = 1;
> - } else
> - db_printf(",%d", cpu);
> - }
> - }
> + ddb_display_cpuset(&set->cs_mask);
> db_printf("\n");
> if (db_pager_quit)
> break;
I think this is a nice add-on, please commit it separately.
> --- //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?
> --- //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
> --- //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.
> --- //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()?
> --- //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.
> --- //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?
> --- //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).
The rest of the patch looks good.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA>