Date: Sun, 26 Oct 2014 18:59:38 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Tiwei Bie <btw@mail.ustc.edu.cn> Cc: freebsd-hackers@freebsd.org Subject: Re: [PATCH] Finish the task 'Replace loginclass mutex with rwlock' Message-ID: <20141026175938.GB30512@dft-labs.eu> In-Reply-To: <1414330126-20098-1-git-send-email-btw@mail.ustc.edu.cn> References: <1414330126-20098-1-git-send-email-btw@mail.ustc.edu.cn>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Oct 26, 2014 at 09:28:46PM +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.
Equivalent treatment can be done to uifind et al in a separate patch.
> diff --git a/sys/kern/kern_loginclass.c b/sys/kern/kern_loginclass.c
> index b20f60b..1c2f81d 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,33 @@ 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);
> + rw_wunlock(&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);
> }
This could hold lc before returning it.
>
> /*
> @@ -109,34 +126,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) {
Here it would be nicer to lc != NULL and handle short case first.
Then we don't have to indent the longer block.
> + 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);
This could be done prior to taking the lock.
> + 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 +244,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://wiki.freebsd.org/JuniorJobs#Replace_loginclass_mutex_with_rwlock
>
> Tiwei Bie
>
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141026175938.GB30512>