Subject: Re: lockmgr/lockinit and LK_SPIN - panic caused by legal flags?
To: Gary Thorpe <gathorpe79@yahoo.com>
From: Bill Studenmund <wrstuden@netbsd.org>
List: tech-kern
Date: 09/08/2003 15:18:54
On Sat, 6 Sep 2003, Gary Thorpe wrote:

> I am having a problem with passing LK_SPIN as a flag to lockmgr(). It
> raises a panic in the following code section, src/sys/kern/kern_lock.c,
> 1.72, in lockmgr() (current kernel is compiled with diagnostic):
>
> #ifdef DIAGNOSTIC /* { */
> 	/*
> 	 * Don't allow spins on sleep locks and don't allow sleeps
> 	 * on spin locks.
> 	 */
> 	if ((flags ^ lkp->lk_flags) & LK_SPIN)
> 		panic("lockmgr: sleep/spin mismatch");
> #endif /* } */
>
>
> The flag is LK_SPIN|LK_EXCLUSIVE|LK_RECURSEFAIL. lkp->lk_flags is equal
> to 0.
> (LK_SPIN|LK_...) ^ 0 = (LK_SPIN|LK_...)
> (LK_SPIN|LK_...) & LK_SPIN = nonzero
> And so the panic is called.

As well it should.

> I call lockinit using something similar to:
> lockinit(lkp, prio, wmesg, (1 * hz), 0);

If it's going to be a spin lock, you should initialize it as such.

> Which is why lkp->lk_flags is 0. I then call lockmgr with something
> similar to:
> lockmgr(lkp, LK_SPIN|LK_EXCLUSIVE|LK_RECURSEFAIL, NULL);
>
> It seems perfectly legal to call lockinit() with a 0 flag. I need to do
> this because the thread calling lockmgr() may need to sleep or spin
> depending on the situation (the spin case is in a network ioctl for a
> device: is sleeping safe?). Is this behaviour possible? I have not
> actually found code in the tree that uses LK_SPIN though, so could this
> be a genuine error that has not been found until now?

That's not what LK_SPIN is about, it seems. LK_SPIN is about making a spin
lock, not a sleeper lock. A lock should be made one way or the other, then
always used that same way. Thus the code is correct.

Why are you calling lockmgr() locks in case where sleeping is not
possible? That's the bug...

> Should the code be:
>
> #ifdef DIAGNOSTIC /* { */
> 	/*
> 	 * Don't allow spins on sleep locks and don't allow sleeps
> 	 * on spin locks.
> 	 */
> 	if (((flags|lkp->lk_flags) & LK_SPIN) &&
>                 ((flags|lkp->lk_flags) & LK_SLEEPFAIL)))
> 		panic("lockmgr: sleep/spin mismatch");
> #endif /* } */

Nope.

Take care,

Bill