Date: Mon, 27 Oct 2014 08:48:28 +0800 From: Tiwei Bie <btw@mail.ustc.edu.cn> To: mjg@freebsd.org Cc: freebsd-hackers@freebsd.org Subject: Re: Re: [PATCH] Finish the task 'Replace loginclass mutex with rwlock' Message-ID: <1414370908-34925-1-git-send-email-btw@mail.ustc.edu.cn> In-Reply-To: <20141026175938.GB30512@dft-labs.eu> References: <20141026175938.GB30512@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
> 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. ;-)
#1. in loginclass_free():
I removed an unnecessary rw_wunlock(&loginclasses_lock) call;
#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);
Following is my new patch:
diff --git a/sys/kern/kern_loginclass.c b/sys/kern/kern_loginclass.c
index b20f60b..986c51b 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,30 @@ loginclass_free(struct loginclass *lc)
if (old > 1 && atomic_cmpset_int(&lc->lc_refcount, old, old - 1))
return;
- mtx_lock(&loginclasses_lock);
+ rw_wlock(&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);
-
- return;
}
- mtx_unlock(&loginclasses_lock);
+ rw_wunlock(&loginclasses_lock);
+}
+
+/*
+ * Look up a loginclass struct for the parameter name.
+ * loginclasses_lock must be locked.
+ */
+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)
+ break;
+
+ return (lc);
}
/*
@@ -109,34 +123,39 @@ loginclass_free(struct loginclass *lc)
struct loginclass *
loginclass_find(const char *name)
{
- struct loginclass *lc, *newlc;
+ struct loginclass *lc, *oldlc;
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);
- return (lc);
+ rw_rlock(&loginclasses_lock);
+ lc = loginclass_lookup(name);
+ if (lc == NULL) {
+ rw_runlock(&loginclasses_lock);
+ lc = malloc(sizeof(*lc), M_LOGINCLASS, M_ZERO | M_WAITOK);
+ racct_create(&lc->lc_racct);
+ 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 ((oldlc = loginclass_lookup(name)) != NULL) {
+ /* Someone else beat us to it. */
+ racct_destroy(&lc->lc_racct);
+ free(lc, M_LOGINCLASS);
+ lc = oldlc;
+ } else {
+ /* Add new loginclass. */
+ strcpy(lc->lc_name, name);
+ refcount_init(&lc->lc_refcount, 1);
+ LIST_INSERT_HEAD(&loginclasses, lc, lc_next);
+ }
}
- /* 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);
-
- return (newlc);
+ loginclass_hold(lc);
+ rw_unlock(&loginclasses_lock);
+ return (lc);
}
/*
@@ -222,8 +241,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
[1] https://www.freebsd.org/cgi/man.cgi?query=rwlock&sektion=9
Tiwei Bie
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1414370908-34925-1-git-send-email-btw>