Bug #919

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

Added by aoiko over 6 years ago. Updated over 4 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

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

History

#1 Updated by corecode over 6 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

#2 Updated by dillon over 6 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
<>

#3 Updated by aoiko over 6 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

#4 Updated by corecode over 6 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

#5 Updated by aoiko over 6 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

#6 Updated by aoiko over 6 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

#7 Updated by aoiko over 6 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

#8 Updated by aoiko over 6 years ago

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

Aggelos

#9 Updated by aoiko over 6 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)
}

/*
+ * Atomically drop a lockmgr lock and go to sleep. The lock is reacquired
+ * before returning from this function. Passes on the value returned by
+ * tsleep().
+ */
+int
+lock_sleep(void *ident, int flags, const char *wmesg, int timo,
+ struct lock *lk)
+{
+ int err, mode;
+
+ mode = lockstatus_owned(lk, curthread);
+ KKASSERT((mode == LK_EXCLUSIVE) || (mode == LK_SHARED));
+
+ crit_enter();
+ tsleep_interlock(ident);
+ lockmgr(lk, LK_RELEASE);
+ err = tsleep(ident, flags, wmesg, timo);
+ crit_exit();
+ lockmgr(lk, mode);
+ return err;
+}
+
+/*
* Interlocked spinlock sleep. An exclusively held spinlock must
* be passed to msleep(). The function will atomically release the
* spinlock and tsleep on the ident, then reacquire the spinlock and
@@ -596,8 +619,8 @@ tsleep_interlock(void *ident)
* heavily.
*/
int
-msleep(void *ident, struct spinlock *spin, int flags,
- const char *wmesg, int timo)
+spin_sleep(void *ident, int flags, const char *wmesg, int timo,
+ struct spinlock *spin)
{
globaldata_t gd = mycpu;
int error;
Index: sys/sys/cdefs.h
===================================================================
retrieving revision 1.19
diff -u -p -r1.19 cdefs.h
--- sys/sys/cdefs.h
+++ sys/sys/cdefs.h
@@ -174,8 +174,10 @@

#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
===================================================================
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);

#10 Updated by corecode over 6 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

#11 Updated by aoiko over 6 years ago

No problem with that.

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

Aggelos

#12 Updated by pavalos over 6 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

#13 Updated by dillon over 6 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

#14 Updated by dillon over 6 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
<>

#15 Updated by aoiko over 6 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

#16 Updated by aoiko over 6 years ago

Yet another ping.

Aggelos

#17 Updated by corecode over 6 years ago

Pick one and put it in :)

cheers
simon

#18 Updated by aoiko over 6 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

#19 Updated by dillon over 6 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
<>

#20 Updated by alexh over 4 years ago

lksleep has been introduced some time ago.

Also available in: Atom PDF