Date: Tue, 28 Oct 2014 12:10:17 +0800 From: Tiwei Bie <btw@mail.ustc.edu.cn> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-hackers@freebsd.org Subject: Re: Re: [PATCH] Finish the task 'Replace loginclass mutex with rwlock' Message-ID: <20141028041017.GA2205@freebsd> In-Reply-To: <20141027211553.GB28049@dft-labs.eu> References: <20141026175938.GB30512@dft-labs.eu> <1414370908-34925-1-git-send-email-btw@mail.ustc.edu.cn> <20141027211553.GB28049@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141028041017.GA2205>