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

[Mateusz Guzik]

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>