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