refcount_release_take_##lock

[Mateusz Guzik]

On Tue, Oct 28, 2014 at 11:54:54AM -0400, John Baldwin wrote:
> On Monday, October 27, 2014 3:27:21 pm Mateusz Guzik wrote:
> > On Mon, Oct 27, 2014 at 11:27:45AM -0400, John Baldwin wrote:
> > > Please keep the refcount_*() prefix so it matches the rest of the API.  I 
> > > would just declare the functions directly in refcount.h rather than requiring 
> > > a macro to be invoked in each C file.  We can also just implement the needed 
> > > lock types for now instead of all of them.
> > > 
> > > You could maybe replace 'take' with 'lock', but either name is fine.
> > > 
> > 
> > 
> > We need sx and rwlocks (and temporarily mutexes, but that is going away
> > in few days).
> 
> Ok.
> 
> > I ran into the following issue: opensolaris code has its own rwlock.h,
> > and their refcount.h eventually includes ours refcount.h (and it has to
> > since e.g. our file.h requires it).
> > 
> > I don't know any good solution.
> 
> Ugh.
> 
> > We could add locking funcs to a separate header (refcount_lock.h?) or use the
> > following hack:
> > 
> > +#ifdef _SYS_RWLOCK_H_
> > +REFCOUNT_RELEASE_LOCK_DEFINE(rwlock, struct rwlock, rw_wlock, rw_wunlock);
> > +#else
> 
> The problem here is that typically refcount.h would be included before rwlock.h
> (style(9) sorts headers alphabetically).
> 
> Given that you want to inline this anyway, you could perhaps implement it as
> a macro instead of an inline function?  That would result in it only being
> parsed when used which would side-step this.  It's not really ideal but might
> be less ugly than the other options.  Something like:
> 
> #define _refcount_release_lock(count, lock, LOCK_OP, UNLOCK_OP) \
> ...
> 
> #define	refcount_release_lock_mtx(count, lock)					\
> 	_refcount_release_lock((count), (lock), mtx_lock, mtx_unlock)
> 

diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c
index f8ae0e6..e94ccde 100644
--- a/sys/kern/kern_jail.c
+++ b/sys/kern/kern_jail.c
@@ -4466,15 +4466,12 @@ prison_racct_free_locked(struct prison_racct *prr)
 void
 prison_racct_free(struct prison_racct *prr)
 {
-	int old;
 
 	sx_assert(&allprison_lock, SA_UNLOCKED);
 
-	old = prr->prr_refcount;
-	if (old > 1 && atomic_cmpset_int(&prr->prr_refcount, old, old - 1))
+	if (!refcount_release_lock_sx(&prr->prr_refcount, &allprison_lock))
 		return;
 
-	sx_xlock(&allprison_lock);
 	prison_racct_free_locked(prr);
 	sx_xunlock(&allprison_lock);
 }
diff --git a/sys/kern/kern_loginclass.c b/sys/kern/kern_loginclass.c
index c0946ef..0771b38 100644
--- a/sys/kern/kern_loginclass.c
+++ b/sys/kern/kern_loginclass.c
@@ -81,18 +81,10 @@ loginclass_hold(struct loginclass *lc)
 void
 loginclass_free(struct loginclass *lc)
 {
-	int old;
 
-	old = lc->lc_refcount;
-	if (old > 1 && atomic_cmpset_int(&lc->lc_refcount, old, old - 1))
+	if (!refcount_release_lock_rwlock(&lc->lc_refcount, &loginclasses_lock))
 		return;
 
-	rw_wlock(&loginclasses_lock);
-	if (!refcount_release(&lc->lc_refcount)) {
-		rw_wunlock(&loginclasses_lock);
-		return;
-	}
-
 	racct_destroy(&lc->lc_racct);
 	LIST_REMOVE(lc, lc_next);
 	rw_wunlock(&loginclasses_lock);
diff --git a/sys/kern/kern_resource.c b/sys/kern/kern_resource.c
index 037a257..e1d5237 100644
--- a/sys/kern/kern_resource.c
+++ b/sys/kern/kern_resource.c
@@ -1303,20 +1303,10 @@ uihold(struct uidinfo *uip)
 void
 uifree(struct uidinfo *uip)
 {
-	int old;
 
-	/* Prepare for optimal case. */
-	old = uip->ui_ref;
-	if (old > 1 && atomic_cmpset_int(&uip->ui_ref, old, old - 1))
+	if (!refcount_release_lock_rwlock(&uip->ui_ref, &uihashtbl_lock))
 		return;
 
-	/* Prepare for suboptimal case. */
-	rw_wlock(&uihashtbl_lock);
-	if (refcount_release(&uip->ui_ref) == 0) {
-		rw_wunlock(&uihashtbl_lock);
-		return;
-	}
-
 	racct_destroy(&uip->ui_racct);
 	LIST_REMOVE(uip, ui_hash);
 	rw_wunlock(&uihashtbl_lock);
diff --git a/sys/sys/refcount.h b/sys/sys/refcount.h
index 4611664..343da6d 100644
--- a/sys/sys/refcount.h
+++ b/sys/sys/refcount.h
@@ -64,4 +64,34 @@ refcount_release(volatile u_int *count)
 	return (old == 1);
 }
 
+#define	_refcount_release_lock(count, lock, TYPE, LOCK_OP, UNLOCK_OP)		\
+({										\
+	TYPE *__lock;								\
+	volatile u_int *__cp;							\
+	u_int __old;								\
+	bool __ret;								\
+										\
+	__lock = (lock);							\
+	__cp = (count);								\
+	__old = *__cp;								\
+	__ret = 0;								\
+	if (!(__old > 1 && atomic_cmpset_int(__cp, __old, __old - 1))) {	\
+		LOCK_OP(__lock);						\
+		if (refcount_release(__cp) == 0)				\
+			UNLOCK_OP(__lock);					\
+		else 								\
+			__ret = 1;						\
+	}									\
+	__ret;									\
+})
+
+#define	refcount_release_lock_mtx(count, lock)		\
+	    _refcount_release_lock(count, lock, struct mtx, mtx_lock, mtx_unlock)
+#define	refcount_release_lock_rmlock(count, lock)	\
+	    _refcount_release_lock(count, lock, struct rmlock, rm_wlock, rm_wunlock)
+#define	refcount_release_lock_rwlock(count, lock)	\
+	    _refcount_release_lock(count, lock, struct rwlock, rw_wlock, rw_wunlock)
+#define	refcount_release_lock_sx(count, lock)		\
+	    _refcount_release_lock(count, lock, struct sx, sx_xlock, sx_xunlock)
+
 #endif	/* ! __SYS_REFCOUNT_H__ */

-- 
Mateusz Guzik <mjguzik gmail.com>