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

[Aggelos Economopoulos <aoiko@xxxxxxxxxxxxxx>]

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