svn commit: r238907 - projects/calloutng/sys/kern
Attilio Rao
attilio at freebsd.org
Thu Aug 2 21:15:22 UTC 2012
On 8/2/12, John Baldwin <jhb at freebsd.org> wrote:
> On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>> On 7/30/12, John Baldwin <jhb at 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.
If you are going to commit this, please make sure to add right
comments that explain the reasons. Likely you may want to add that on
top of implementation of lock_init().
>> > --- //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.
I just meant a macro for checking that as we have special macro for
dealing with bumping/unbumping. But this is up to you, it is not
really important.
>> > --- //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()).
I think we should just use one form or another. I propose at some
point we just adopt the CRITICAL_ASSERT(), so I hope we will change
all the occurence of the dumby check in proper CRITICAL_ASSERT().
>> > --- //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.
Yes, sorry, misread that part of the patch.
>
>> > --- //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.
I think it is sensitive we don't add further KPI breakage. I think it
is a good time we start discussing expansion of INVARIANT_SUPPORT to
something more general that offers compat shims for all the debugging
mechanisms.
Attilio
--
Peace can only be achieved by understanding - A. Einstein
More information about the svn-src-projects
mailing list