From owner-svn-src-projects@FreeBSD.ORG Thu Aug 2 20:56:06 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 BC92F1065677; Thu, 2 Aug 2012 20:56:06 +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 9F3478FC1D; Thu, 2 Aug 2012 20:56:05 +0000 (UTC) Received: by laai10 with SMTP id i10so6746574laa.13 for ; Thu, 02 Aug 2012 13:56:04 -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=UVSZuMJS4K9jGWKl2HucgMlQzwDQ4fadltCuyQCIRAw=; b=HKeOkbLoB6kmUlCmb9epcg6iABVJ3lj+R4VBfLU0FgaSYA9lxypl+7YaUpc3wfkf86 or5HsmaDXAoxCvFSXuxZb18o0oPKpwbt2RsPmSdfzgYI8+bUWRkye5OjGYRh+BpOTbtP ozpxwi5BLO3IoN6L5NEdxhnNX0JtrxWdQcOEZzrBspKtgpeWNmloGKwDMLRYbuiYkOHz SNwUb/p3htFeCkJLBDGsxNb9uR0uvrR/tUQYCzo+w8LmOesq2O6ThLF+arYOa3RPuHl2 mE4161rd0+2RopQYKFSq9wKRll1XvG9IrklhFw5L7gKKwxIXFzw1dWD0meZaWlgrij59 d9vg== MIME-Version: 1.0 Received: by 10.112.11.100 with SMTP id p4mr10223917lbb.35.1343940964156; Thu, 02 Aug 2012 13:56:04 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.27.65 with HTTP; Thu, 2 Aug 2012 13:56:03 -0700 (PDT) In-Reply-To: <201207301732.33474.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> Date: Thu, 2 Aug 2012 21:56:03 +0100 X-Google-Sender-Auth: y9HTdkK94zK2UiapVfasLJwy15c 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: Thu, 02 Aug 2012 20:56:06 -0000 On 7/30/12, John Baldwin 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(""); > +} > + > 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