svn commit: r238907 - projects/calloutng/sys/kern

[John Baldwin]

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.

> > --- //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