Project

General

Profile

Actions

Bug #919

closed

lockmgr_sleep() (was Re: ssleep() (was Re: mention msleep() in porting_drivers.txt))

Added by aoiko about 16 years ago. Updated almost 14 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:

Description

On Monday 07 January 2008, Matthew Dillon wrote:
[...]

If we're gonna start throwing around different types of sleeps maybe
we should make the names more verbose. spin_sleep(), lockmgr_sleep(),
etc. [...]

So, about adding a lockmgr_sleep(): I think it would be useful; open-coding
the whole thing makes it easy to slip in hard to notice bugs. But what should
the prototype be? If we follow the freebsd semantics, the lock should be re-
acquired on return. This means that we need to do a lockstatus() (which implies
a spin_lock_wr()) to get information that the caller probably has available or
add an extra "lock flags" parameter which complicates the interface. The
options seem to be:

  • lockmgr_sleep(void *ident, struct lock *lk, int flags, const char *wmesg,
    int timeo)

forces spin_lock_wr(), but is consistent with our (and freebsd's) msleep()
prototype.

  • lockmgr_sleep(void *ident, struct lock *lk, int lkflags, int flags,
    const char *wmesg, int timeo)

this sucks. If you accidentally exchange the flags arguments you'll only find
out after one or more debugging sessions.

  • lockmgr_sleep(void *ident, int lkflags, struct lock *lk, int flags,
    const char *wmesg, int timeo)

this is ugly and inconsistent.

  • lockmgr_sleep(void *ident, int flags, const char *wmesg, int timeo,
    struct lock *lk, int lkflags)

this is more promising but inconsistent with our msleep(). OTOH, I the
original msleep() prototype seems a bit backward to me, so I'd consider
this an improvement.

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)

The down side of course is having to change a couple of dozen call sites. But
as long as we're breaking people's patches with the name change, we might as
well fix the interface, assuming everyone agrees that's an improvement.

Any thoughts and/or better ideas?

Aggelos

Actions #1

Updated by corecode about 16 years ago

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

Actions #2

Updated by dillon about 16 years ago

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
<>
Actions #3

Updated by aoiko about 16 years ago

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

Actions #4

Updated by corecode about 16 years ago

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

Actions #5

Updated by aoiko about 16 years ago

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

Actions #6

Updated by aoiko about 16 years ago

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

Actions #7

Updated by aoiko about 16 years ago

[...]

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

Actions #8

Updated by aoiko about 16 years ago

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

Aggelos

Actions #9

Updated by aoiko about 16 years ago

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
Actions #10

Updated by corecode about 16 years ago

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

Actions #11

Updated by aoiko about 16 years ago

No problem with that.

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

Aggelos

Actions #12

Updated by pavalos about 16 years ago

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

Actions #13

Updated by dillon about 16 years ago

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
Actions #14

Updated by dillon about 16 years ago

:
: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
<>
Actions #15

Updated by aoiko about 16 years ago

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

Actions #16

Updated by aoiko almost 16 years ago

Yet another ping.

Aggelos

Actions #17

Updated by corecode almost 16 years ago

Pick one and put it in :)

cheers
simon

Actions #18

Updated by aoiko almost 16 years ago

[...]

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

Actions #19

Updated by dillon almost 16 years ago

: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
<>
Actions #20

Updated by alexh almost 14 years ago

lksleep has been introduced some time ago.

Actions

Also available in: Atom PDF