On Saturday 26 January 2008, Aggelos Economopoulos wrote:
> On Wednesday 16 January 2008, Aggelos Economopoulos wrote:
> > On Wednesday 16 January 2008, Matthew Dillon wrote:
> > > 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.
> >
> > 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?
> >
> > >
> > > :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)
> >
> > 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.
>
> So what's the verdict? Veto still on? Need to know in order to submit a patch
> :)
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);
