On Thu, May 23, 2002 at 12:11:03PM -0700, Konrad Schroder wrote:

 > What is the rule of thumb with regard to spl*() (in my case splbio()) in
 > conjunction with simple_lock()?  Must they be nested, and if so in what
 > order?

The use of spl*() together with simple_lock() generally indicates a place
where an "interrupt blocking mutex" would be used.  In Solaris (and my
"newlock" branch), mutex_enter()/mutex_exit() handle all of this for you.

In general, you spl*() when you acquire a mutex which can also be acquired
in interrupt context.  The logic goes something like this:

	* simple_lock() protects you from another CPU mucking with that
	  resource.

	* Since an interrupt on YOUR CPU could also cause the lock to be
	  acquired, you must block that interrupt before attempting to
	  acquire the resource, otherwise deadlock will occur.

For this reason, the order should ALWAYS be:

	s = splfoo();
	simple_lock(...);

	/* do stuff */

	simple_unlock(...);
	splx(s);

...in our current kernel.

 > For example, I have a pending change to vrele which (I think) should look
 > like this:
 > 
 >         s = splbio();
 >         simple_lock(&vp->v_interlock);
 >         vp->v_usecount--;
 >         if (vp->v_usecount > 0) {
 >                 simple_unlock(&vp->v_interlock);
 >                 splx(s);
 >                 return;
 >         }
 > 
 >         simple_lock(&vnode_free_list_slock);
 >         if (vp->v_holdcnt > 0)
 >                 TAILQ_INSERT_TAIL(&vnode_hold_list, vp, v_freelist);
 >         else
 >                 TAILQ_INSERT_TAIL(&vnode_free_list, vp, v_freelist);
 >         simple_unlock(&vnode_free_list_slock);
 >         splx(s);
 > 
 > The purpose of the change is that "v_usecount--" and the tailq insertion
 > need to be atomic vis-a-vis disk interrupts.  Given that the section under

Yes, your change is correct with regard to spl and simple_lock ordering.

But it looks to me like you're forgetting to release the v_interlock.

In a perfect world, things would work like this (this is how mutexes work
in the "newlock" branch):

	...
	mutex_init(&vp->v_interlock, MUTEX_SPIN, IPL_BIO);
	...
	mutex_init(&vnode_free_list_mutex, MUTEX_SPIN, IPL_BIO);
	...

	mutex_enter(&vp->v_interlock);
	vp->v_usecount--;
	if (vp->v_usecount > 0) {
		mutex_exit(&vp->v_interlock);
		return;
	}

	mutex_enter(&vnode_free_list_mutex);
	if (vp->v_holdcnt > 0)
		TAILQ_INSERT_TAIL(&vnode_hold_list, vp, v_freelist);
	else
		TAILQ_INSERT_TAIL(&vnode_free_list, vp, v_freelist);
	mutex_exit(&vnode_free_list_mutex);

	mutex_exit(&vp->v_interlock);

It's probably easiest to think about the current spl/simple_lock stuff
in this way.  And, it might not hurt to create macros for acquiring/releasing
these kinds of locks now (UVM currently has such a macro for dealing with
the fpageq lock, for example).

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>