--- Bill Studenmund <wrstuden@netbsd.org> wrote: > On Tue, 9 Sep 2003,
Gary Thorpe wrote:
> 
> > Ok, so basically, the only way the code won't panic is if both
> > lockinit() and lockmgr() specify LK_SPIN with each invocation for
> the
> > same lock. However, looking at the code it is possible to do this:
> >
> > lockinit(lkp, prio, wmesg, 0, LK_SPIN|LK_SLEEPFAIL);
> > .
> > .
> > .
> > lockmgr(lkp, LK_SPIN|LK_SLEEPFAIL, NULL);
> >
> > (LK_SPIN|LK_SLEEPFAIL) ^ (LK_SPIN|LK_SLEEPFAIL) = 0;
> > 0 & LK_SPIN = 0;
> > no panic;
> >
> > Does this make sense? While the code requires that LK_SPIN must
> always
> > be specified (unlike all the other flags - bad documentation), it
> > doesn't prevent this mix/matching. How about:
> 
> 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.

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

> What the LK_SPIN panic is trying to do is make it so that we don't
> silently mix up lock pointers. Everything that is supposed to have a
> spin
> lock will have LK_SPIN, and stuff that is supposed to be a sleeper
> lock
> won't. Think of it as a way of hiding both struct lockmgr_spin and
> struct
> lockmgr_sleep within struct lockmgr.

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.

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.

> 
> Take care,
> 
> Bill
>  

______________________________________________________________________ 
Post your free ad now! http://personals.yahoo.ca