On Tue, 9 Sep 2003, Gary Thorpe wrote:

>  --- Bill Studenmund <wrstuden@netbsd.org> wrote: > On Tue, 9 Sep 2003,
> >
> > I don't see where the problem is.
>
> When a process tries to acquire a spin lock, I don't think it would
> expect to be able to sleep on failure (which is what specifying
> LK_SLEEPFAIL i supposed to mean). If specifying sleeping AND spinning
> is wrong, why does the code _allow_ you to do just that, even if it
> effectively lets one over-ride the other? Because of the way the code
> is, I don't think it will sleep. Bad design.

That's not what LK_SLEEPFAIL does, according to the man page. LK_SLEEPFAIL
makes the locker sleep, then return failure. That is NOT the same as sleep
if there's a failure.  LK_SLEEPFAIL looks like the kind of thing you'd
want if you're decomissioning a lock.

> > > lockinit(lkp, prio, wmesg, 0, LK_SPIN);
> > > .
> > > .
> > > .
> > > lockmgr(lkp, LK_EXCLUSIVE, NULL);
> > > (LK_SPIN) ^ (LK_EXCLUSIVE) = LK_EXCLUSIVE|LK_SPIN;
> > > LK_EXCLUSIVE|LK_SPIN & LK_SPIN = LK_SPIN;
> > > panic;
> > >
> > > Here, I would have tried to initialize a spin lock, get the lock
> > > exclusively and it would still panic. The man page leaves this part
> > out
> > > and thats all I have to go by.
> >
> > Then the man page should be changed.
> >
> > However you initialized the lock as a spin lock, then tried to access
> > it
> > as if it wasn't. -> Boom!
>
> No, I initialized a spin lock and any lockmgr() operations on it should
> operate on a spin lock. _IF_ I try to specify a sleeping action
> (LK_SLEEPFAIL for example) in lockmgr(), then it should panic. It seems
> sleeping is the default state - that is not stated in the manual page.

That's not how this code is designed, and that's not what LK_SPIN is
doing.

[snip]

> Yes, but I would think that initializing the lock as a spin lock would
> mean that all operations with lockmgr() would default to spinning vs.
> sleeping. What is the point of endlessly specify the same flags over
> and over again - the flag for spinning vs. sleeping _should_ be
> permanent and required in lockinit(). Since you cannot mix the two,
> there is no point in letting the developer use LK_SPIN in every single
> action on the lock when it can be specified _once_ in lockinit().
> Doesn't it make sense to specify once what type of lock is being
> requested?
>
> The design was poorly thought out in this respect: panic should be
> raised when you try to do both _explicitly_ and the developer should be
> forced to _explicitly_ specify whether the lock is for
> sleeping/spinning when it is initialized.

Why? What's wrong with adding LK_SPIN, and ignoring flags that don't make
sense (LK_SLEEPFAIL)?

> The code stores the permanent flags passed to lockinit() and ORs them
> back with flags passed with lockmgr(): the place to specify ONCE
> whether the lock lets the caller sleep or spin would be lockinit(). The
> man page does not state clearly what flag makes a lock sleeping/spinng:
> is it LK_NOWAIT, LK_SLEEPFAIL, or LK_SPIN? The implied answer is that a
> missing LK_SPIN implies sleeping. This is not clear in the manual
> pages, but even if it where it is not what a "nice" lock API would do.

I agree the man pages need work.

Take care,

Bill