[PATCH] Finish the task 'Replace loginclass mutex with rwlock'

[Tiwei Bie]

On Mon, Oct 27, 2014 at 10:15:53PM +0100, Mateusz Guzik wrote:
> On Mon, Oct 27, 2014 at 08:48:28AM +0800, Tiwei Bie wrote:
> > > In general I see the change mirrors uifind & friends and seems correct.
> > > 
> > > However, I think we can alter the code so that it looks nicer.
> > 
> > Yeah! I have made some modifications to this patch. ;-)
> > 
> 
> Well, I made some annotations to the patch in my previous mail, I guess
> you missed them.
> 

Sorry, I misunderstood your annotations. ;-(

> > #1. in loginclass_free():
> > 
> > I removed an unnecessary rw_wunlock(&loginclasses_lock) call;
> > 
> 
> This means more work done is with the lock held. Whether that's good,
> bad or acceptable is arguable, I personally don't like it.
> 

I don't like it either. In this case, I thought the free() operation
could be finished quickly, so the overhead could be trivial and removing
this call could make the codes shorter.

> > #2. in loginclass_find():
> > 
> > The newly allocated 'lc' could be freed if someone else has created the
> > loginclass. But I couldn't find a simple way to avoid this. Because we
> > could not call racct_create() which could sleep while holding the wlock[1].
> > And this is the code that could sleep in racct_create():
> > 
> > 	*racctp = uma_zalloc(racct_zone, M_WAITOK | M_ZERO);
> 
> That's a totally standard situation.
> 
> Anyway, previously I suggested some changes and now impleented them in
> another file here: https://svnweb.freebsd.org/changeset/base/273746
> 
> Care to alter your patch in similar manner?
> 

Yeah, of course! It's my honor! Thank you very much!

This is my new patch:

diff --git a/sys/kern/kern_loginclass.c b/sys/kern/kern_loginclass.c
index b20f60b..92eb3cf 100644
--- a/sys/kern/kern_loginclass.c
+++ b/sys/kern/kern_loginclass.c
@@ -51,13 +51,13 @@ __FBSDID("$FreeBSD$");
 #include <sys/lock.h>
 #include <sys/loginclass.h>
 #include <sys/malloc.h>
-#include <sys/mutex.h>
 #include <sys/types.h>
 #include <sys/priv.h>
 #include <sys/proc.h>
 #include <sys/queue.h>
 #include <sys/racct.h>
 #include <sys/refcount.h>
+#include <sys/rwlock.h>
 #include <sys/sysproto.h>
 #include <sys/systm.h>
 
@@ -68,8 +68,8 @@ LIST_HEAD(, loginclass)	loginclasses;
 /*
  * Lock protecting loginclasses list.
  */
-static struct mtx loginclasses_lock;
-MTX_SYSINIT(loginclasses_init, &loginclasses_lock, "loginclasses lock", MTX_DEF);
+static struct rwlock loginclasses_lock;
+RW_SYSINIT(loginclasses_init, &loginclasses_lock, "loginclasses lock");
 
 void
 loginclass_hold(struct loginclass *lc)
@@ -87,16 +87,37 @@ loginclass_free(struct loginclass *lc)
 	if (old > 1 && atomic_cmpset_int(&lc->lc_refcount, old, old - 1))
 		return;
 
-	mtx_lock(&loginclasses_lock);
-	if (refcount_release(&lc->lc_refcount)) {
-		racct_destroy(&lc->lc_racct);
-		LIST_REMOVE(lc, lc_next);
-		mtx_unlock(&loginclasses_lock);
-		free(lc, M_LOGINCLASS);
-
+	rw_wlock(&loginclasses_lock);
+	if (!refcount_release(&lc->lc_refcount)) {
+		rw_wunlock(&loginclasses_lock);
 		return;
 	}
-	mtx_unlock(&loginclasses_lock);
+
+	racct_destroy(&lc->lc_racct);
+	LIST_REMOVE(lc, lc_next);
+	rw_wunlock(&loginclasses_lock);
+
+	free(lc, M_LOGINCLASS);
+}
+
+/*
+ * Look up a loginclass struct for the parameter name.
+ * loginclasses_lock must be locked.
+ * Increase refcount on loginclass struct returned.
+ */
+static struct loginclass *
+loginclass_lookup(const char *name)
+{
+	struct loginclass *lc;
+
+	rw_assert(&loginclasses_lock, RA_LOCKED);
+	LIST_FOREACH(lc, &loginclasses, lc_next)
+		if (strcmp(name, lc->lc_name) == 0) {
+			loginclass_hold(lc);
+			break;
+		}
+
+	return (lc);
 }
 
 /*
@@ -109,34 +130,39 @@ loginclass_free(struct loginclass *lc)
 struct loginclass *
 loginclass_find(const char *name)
 {
-	struct loginclass *lc, *newlc;
+	struct loginclass *lc, *new_lc;
 
 	if (name[0] == '\0' || strlen(name) >= MAXLOGNAME)
 		return (NULL);
 
-	newlc = malloc(sizeof(*newlc), M_LOGINCLASS, M_ZERO | M_WAITOK);
-	racct_create(&newlc->lc_racct);
-
-	mtx_lock(&loginclasses_lock);
-	LIST_FOREACH(lc, &loginclasses, lc_next) {
-		if (strcmp(name, lc->lc_name) != 0)
-			continue;
-
-		/* Found loginclass with a matching name? */
-		loginclass_hold(lc);
-		mtx_unlock(&loginclasses_lock);
-		racct_destroy(&newlc->lc_racct);
-		free(newlc, M_LOGINCLASS);
+	rw_rlock(&loginclasses_lock);
+	lc = loginclass_lookup(name);
+	rw_runlock(&loginclasses_lock);
+	if (lc != NULL)
 		return (lc);
-	}
 
-	/* Add new loginclass. */
-	strcpy(newlc->lc_name, name);
-	refcount_init(&newlc->lc_refcount, 1);
-	LIST_INSERT_HEAD(&loginclasses, newlc, lc_next);
-	mtx_unlock(&loginclasses_lock);
+	new_lc = malloc(sizeof(*new_lc), M_LOGINCLASS, M_ZERO | M_WAITOK);
+	racct_create(&new_lc->lc_racct);
+	refcount_init(&new_lc->lc_refcount, 1);
+	strcpy(new_lc->lc_name, name);
+
+	rw_wlock(&loginclasses_lock);
+	/*
+	 * There's a chance someone created our loginclass while we
+	 * were in malloc and not holding the lock, so we have to
+	 * make sure we don't insert a duplicate loginclass.
+	 */
+	if ((lc = loginclass_lookup(name)) == NULL) {
+		LIST_INSERT_HEAD(&loginclasses, new_lc, lc_next);
+		rw_wunlock(&loginclasses_lock);
+		lc = new_lc;
+	} else {
+		rw_wunlock(&loginclasses_lock);
+		racct_destroy(&new_lc->lc_racct);
+		free(new_lc, M_LOGINCLASS);
+	}
 
-	return (newlc);
+	return (lc);
 }
 
 /*
@@ -222,8 +248,8 @@ loginclass_racct_foreach(void (*callback)(struct racct *racct,
 {
 	struct loginclass *lc;
 
-	mtx_lock(&loginclasses_lock);
+	rw_rlock(&loginclasses_lock);
 	LIST_FOREACH(lc, &loginclasses, lc_next)
 		(callback)(lc->lc_racct, arg2, arg3);
-	mtx_unlock(&loginclasses_lock);
+	rw_runlock(&loginclasses_lock);
 }
-- 
2.1.0

PS. I found a bug in my previous patches. I initialized lc->lc_refcount
    to 1 and called loginclass_hold(lc) again. Sorry. LoL..

PPS. In loginclass_free(), I checked the return value of refcount_release()
     with the following code:

           if (!refcount_release(&lc->lc_refcount)) {
                  rw_wunlock(&loginclasses_lock);
                  return;
           }

     Instead of

           if (refcount_release(&lc->lc_refcount) == 0) {
                  rw_wunlock(&loginclasses_lock);
                  return;
           }

     Because the retval is a bool value. And I think

          if (!refcount_release(&lc->lc_refcount))

     could be read as 'if not release lc'.

Tiwei Bie