DragonFlyBSD

Why? When we release the lock in lock_sleep, we will obtain the status
anyways, right?

Looks nice this way, in case we need to pass the lkflags.

+1

No problem. We could keep msleep() around for some time.

cheers
simon

lockmgr_sleep(ident, lock, slpflags, wmesg, timeo)

lockmgr_sleep can also figure out what kind of lock is held internally
    and deal with it, or it can just assume an exclusive lock.  Either way
    lkflags do not have to be passed to it.

:If we're going to rename msleep() to spin_sleep() anyway, I suggest changing
:the prototype to
:
:spin_sleep(void *ident, int flags, const char *wmesg, int timo,
: struct spinlock *spin)

We want all the *sleep() procedures to use a similar prototype,
    and in this case being compatible with our own history as well
    as FreeBSD's will reduce confusion to a minimum.  That means putting
    the lock as the second argument.

spin_sleep(ident, spin, slpflags, wmesg, timeo)

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Assuming you mean lockmgr_sleep() here, no. lockmgr(LK_RELEASE) only returns
0. OTOH nobody in the tree actually checks for that, so we could change
lockmgr to return the lock status in that case. This seems to solve the
problem nicely. Any objections to that?

Maybe for a little bit (and with a big warning to prevent people from using
it; this is a good chance to bring in attribute(deprecated)) in order to
avoid breakage for out of tree code, but if we add spin_sleep(), msleep()
has to go. We don't want to end up like freebsd (which has mtx_sleep() AND
msleep()), right? ;)

Aggelos

lockmgr_sleep (or lock_sleep?) knows the internals of a lockmgr lock and
thus can optimize the access.

You mean msleep() has to go after a deprecation period, right? (period
being one release cycle = 6 months)

cheers
simon

Assuming an exclusive lock is not intuitive IMO and differs from the FreeBSD
semantics (even though it matches our msleep()/spin_sleep()). What do you
think of the lockmgr change suggested in another mail in this thread?

Nobody can disagree with the "compatible" part, but for a whatever_sleep()
that does a "drop, tsleep() and reacquire whatever lock" it seems more
natural to require the tsleep() args followed by the whatever args. It may
be unlikely, but imagine having to add another flag. Would you put it after
"spin" seperating the tsleep() args further?

Also, I don't think FreeBSD compatibility is an issue, unless the two choices
are otherwise on par.

Aggelos

Yes, but do you really want to split the code in

lockmgr()
case LK_RELEASE:

to a seperate function and call it from lock_sleep() (btw I agree that
lock_sleep() is a better name)? Changing lockmgr() just needs a few extra
lines.

Right.

Aggelos

[...]

lockmgr_sleep (or lock_sleep?) knows the internals of a lockmgr lock and
thus can optimize the access.

Yes, but do you really want to split the code in

lockmgr()
case LK_RELEASE:

to a seperate function and call it from lock_sleep()

Of course you don't. You mean that lock_sleep() can assume the lock is
already held and you could just implement a lockstatus_locked() and
call it there, or open code the check, right? I think the former is
infinitesimally better than returning the status from lockmgr().

Aggelos

So what's the verdict? Veto still on? Need to know in order to submit a patch :)

Aggelos

Ping. It would be nice if the msleep() deprecation made it in the release. Can
you take a five minute break to clarify? The patch in question follows.

Thanks,
Aggelos

Index: sys/kern/kern_lock.c ===================================================================
retrieving revision 1.27
diff u -p -r1.27 kern_lock.c
-- sys/kern/kern_lock.c
+++ sys/kern/kern_lock.c
@ -537,11 +537,11 @ lockuninit(struct lock *l) * Determine the status of a lock.
*/
int
-lockstatus(struct lock *lkp, struct thread *td)
+lockstatus_owned(struct lock *lkp, struct thread *td) {
int lock_type = 0;

- spin_lock_wr(&lkp->lk_spinlock);
+ /* XXX: assert owned */
if (lkp->lk_exclusivecount != 0) {
if (td NULL || lkp->lk_lockholder td)
lock_type = LK_EXCLUSIVE;
@ -550,10 +550,19 @ lockstatus(struct lock *lkp, struct thre
} else if (lkp->lk_sharecount != 0) {
lock_type = LK_SHARED;
}
- spin_unlock_wr(&lkp->lk_spinlock);
return (lock_type);
}

int lockstatus(struct lock lkp, struct thread *td)
{
+ int lock_type;

spin_lock_wr(&lkp->lk_spinlock);
+ lock_type = lockstatus_owned(lkp, td);
+ spin_unlock_wr(&lkp->lk_spinlock);
+ return lock_type;
}

/ * Determine the number of holders of a lock. *
Index: sys/kern/kern_synch.c ===================================================================
retrieving revision 1.88
diff u -p -r1.88 kern_synch.c
-- sys/kern/kern_synch.c
+++ sys/kern/kern_synch.c
@ -587,6 +587,29 @ tsleep_interlock(void *ident)
} ===================================================================
retrieving revision 1.19
diff u -p -r1.19 cdefs.h
-- sys/sys/cdefs.h
+++ sys/sys/cdefs.h
@ -174,8 +174,10 @ ===================================================================
retrieving revision 1.19
diff u -p -r1.19 lock.h
-- sys/sys/lock.h
+++ sys/sys/lock.h
@ -205,6 +205,7 @ void lockmgr_setexclusive_interlocked(st
void lockmgr_clrexclusive_interlocked(struct lock *);
void lockmgr_kernproc (struct lock *);
void lockmgr_printinfo (struct lock *);
+int lockstatus_owned (struct lock *, struct thread *);
int lockstatus (struct lock *, struct thread *);
int lockcount (struct lock *);
int lockcountnb (struct lock *);
Index: sys/sys/systm.h ===================================================================
retrieving revision 1.77
diff u -p -r1.77 systm.h
-- sys/sys/systm.h
+++ sys/sys/systm.h
@ -324,7 +324,19 @ extern watchdog_tickle_fn wdog_tickler; * less often.
/
int tsleep (void *, int, const char *, int);
-int msleep (void *, struct spinlock *, int, const char *, int);
int spin_sleep(void *, int, const char *, int, struct spinlock *);
+int lock_sleep(void *, int, const char *, int, struct lock *);
/
+ * msleep() has been renamed to spin_sleep() and is scheduled for removal in
+ * the next (2.2) release. XXX remove msleep().
+ */
static inline __deprecated int
+msleep(void *ident, struct spinlock *spin, int flags,
const char *wmesg, int timo)
{
return spin_sleep(ident, flags, wmesg, timo, spin);
}

void tsleep_interlock (void *chan);
int lwkt_sleep (const char *, int);
void tstop (void);

#if GNUC_PREREQ(3, 1)
 #define always_inline __attribute((always_inline))
#define deprecated    __attribute((deprecated))
 #else
 #define __always_inline
#define __deprecated
 #endif

#if GNUC_PREREQ(3, 3)
Index: sys/sys/lock.h

I like it, but I'd put lock_sleep in kern_lock.c and wouldn't make
lockstatus_owned public.

+10, should go in now. However then we'd ship a release with a lot of
deprecation warnings. Maybe we should follow up with a msleep->spin_sleep
commit.

cheers
simon

No problem with that.

That was my intention. find + multiline regular expression should help.

Aggelos

I like it too. Having the lock as the first or last arg makes much more
sense than having it in the middle. It won't be confusing, especially
when we document it in a manual page and in porting_drivers.txt.

+1

--Peter

Please keep in mind that we are branching the code-base today :-)
Maybe save the changes until after we branch.

With regards to argument order, I think it is being overthought and
     what we really need to do is to use a similar argument order to
     what FreeBSD uses just to keep things sane for those of us who
     regularly look at multiple project's sources.

That means make it the second argument, just like we've always had
     and just like FreeBSD has.

-Matt

:
:So what's the verdict? Veto still on? Need to know in order to submit a patch :)
:
:Aggelos

I'm woefully behind on list mail.

Lets do it after we branch.  It doesn't need to be in this release.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Sure. Since my exams period is about to start, I can't take care of it before
mid-March, so if any of the people who are porting freebsd code want the
*sleep() changes, please feel free to commit them yourselves.

Aggelos

Yet another ping.

Aggelos

Pick one and put it in :)

cheers
simon

[...]

So what's the verdict? Veto still on? Need to know in order to submit a

patch :)

Yet another ping.

Pick one and put it in :)

Well, I have picked one months ago and explained why. Matt disagreed, I
offered counter-arguments (see quoted discussion). If he's got something more
convincing to respond with, I want to know. After all, we're (supposed to be)
changing the interface to make it better not to make it the way I like it :)

That said, I am going to time out before my ping counter wraps around :)

Aggelos

:Well, I have picked one months ago and explained why. Matt disagreed, I
:offered counter-arguments (see quoted discussion). If he's got something more
:convincing to respond with, I want to know. After all, we're (supposed to be)
:changing the interface to make it better not to make it the way I like it :)
:
:That said, I am going to time out before my ping counter wraps around :)
:
:Aggelos

Oh, its ok.  Minor semantic disagreements shouldn't prevent useful code
    from going in.  Choose something and throw it in.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

lksleep has been introduced some time ago.