From owner-freebsd-arch@FreeBSD.ORG Tue Oct 28 17:44:36 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 66CB3262; Tue, 28 Oct 2014 17:44:36 +0000 (UTC) Received: from mail-wi0-x233.google.com (mail-wi0-x233.google.com [IPv6:2a00:1450:400c:c05::233]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id CCAF4D63; Tue, 28 Oct 2014 17:44:35 +0000 (UTC) Received: by mail-wi0-f179.google.com with SMTP id h11so2376202wiw.12 for ; Tue, 28 Oct 2014 10:44:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=BCmsYGm/PuTTSnfkwXGltszK2GwSzK4ABJQUzyTr5gY=; b=ifEYN6Dqz7mg5xt4A1Vys9pqJYcJ5pM+fM3iZ0R7VtR+IO+Yh/WyMtKDetnsK38GXp bTaz0MfD+INVZ2FnCiP4F89pZo0aFL0h63SIbLTcJfNvfj6GGMpLtzLvLja/QO3FSLcj V3J8snEUbKSilkJ13fGOQH3enDW9Bubh2WFuxWKZlm+VaYuRDDn9BBAzVf0AEg5OX6DF vSZ5hMZZejZI80w1N8EQo05T2causCTzX30yblyRLs/2WheYGe0NtugBM8IjxBPosnf/ XMVgvPK0QZ0NpNrQLy22qarSeKIAv6HZ4pUYkNVksr23ALKjWu2SScX/vrZeMVAlIKEW +BDg== X-Received: by 10.180.221.129 with SMTP id qe1mr6701088wic.21.1414518271988; Tue, 28 Oct 2014 10:44:31 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id eu8sm11226564wic.1.2014.10.28.10.44.30 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 28 Oct 2014 10:44:31 -0700 (PDT) Date: Tue, 28 Oct 2014 18:44:28 +0100 From: Mateusz Guzik To: John Baldwin Subject: Re: refcount_release_take_##lock Message-ID: <20141028174428.GA12014@dft-labs.eu> References: <20141025184448.GA19066@dft-labs.eu> <2629048.tOq3sNXcCP@ralph.baldwin.cx> <20141027192721.GA28049@dft-labs.eu> <201410281154.54581.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <201410281154.54581.jhb@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: John-Mark Gurney , freebsd-arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Oct 2014 17:44:36 -0000 On Tue, Oct 28, 2014 at 11:54:54AM -0400, John Baldwin wrote: > On Monday, October 27, 2014 3:27:21 pm Mateusz Guzik wrote: > > On Mon, Oct 27, 2014 at 11:27:45AM -0400, John Baldwin wrote: > > > Please keep the refcount_*() prefix so it matches the rest of the API. I > > > would just declare the functions directly in refcount.h rather than requiring > > > a macro to be invoked in each C file. We can also just implement the needed > > > lock types for now instead of all of them. > > > > > > You could maybe replace 'take' with 'lock', but either name is fine. > > > > > > > > > We need sx and rwlocks (and temporarily mutexes, but that is going away > > in few days). > > Ok. > > > I ran into the following issue: opensolaris code has its own rwlock.h, > > and their refcount.h eventually includes ours refcount.h (and it has to > > since e.g. our file.h requires it). > > > > I don't know any good solution. > > Ugh. > > > We could add locking funcs to a separate header (refcount_lock.h?) or use the > > following hack: > > > > +#ifdef _SYS_RWLOCK_H_ > > +REFCOUNT_RELEASE_LOCK_DEFINE(rwlock, struct rwlock, rw_wlock, rw_wunlock); > > +#else > > The problem here is that typically refcount.h would be included before rwlock.h > (style(9) sorts headers alphabetically). > > Given that you want to inline this anyway, you could perhaps implement it as > a macro instead of an inline function? That would result in it only being > parsed when used which would side-step this. It's not really ideal but might > be less ugly than the other options. Something like: > > #define _refcount_release_lock(count, lock, LOCK_OP, UNLOCK_OP) \ > ... > > #define refcount_release_lock_mtx(count, lock) \ > _refcount_release_lock((count), (lock), mtx_lock, mtx_unlock) > diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c index f8ae0e6..e94ccde 100644 --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -4466,15 +4466,12 @@ prison_racct_free_locked(struct prison_racct *prr) void prison_racct_free(struct prison_racct *prr) { - int old; sx_assert(&allprison_lock, SA_UNLOCKED); - old = prr->prr_refcount; - if (old > 1 && atomic_cmpset_int(&prr->prr_refcount, old, old - 1)) + if (!refcount_release_lock_sx(&prr->prr_refcount, &allprison_lock)) return; - sx_xlock(&allprison_lock); prison_racct_free_locked(prr); sx_xunlock(&allprison_lock); } diff --git a/sys/kern/kern_loginclass.c b/sys/kern/kern_loginclass.c index c0946ef..0771b38 100644 --- a/sys/kern/kern_loginclass.c +++ b/sys/kern/kern_loginclass.c @@ -81,18 +81,10 @@ loginclass_hold(struct loginclass *lc) void loginclass_free(struct loginclass *lc) { - int old; - old = lc->lc_refcount; - if (old > 1 && atomic_cmpset_int(&lc->lc_refcount, old, old - 1)) + if (!refcount_release_lock_rwlock(&lc->lc_refcount, &loginclasses_lock)) return; - rw_wlock(&loginclasses_lock); - if (!refcount_release(&lc->lc_refcount)) { - rw_wunlock(&loginclasses_lock); - return; - } - racct_destroy(&lc->lc_racct); LIST_REMOVE(lc, lc_next); rw_wunlock(&loginclasses_lock); diff --git a/sys/kern/kern_resource.c b/sys/kern/kern_resource.c index 037a257..e1d5237 100644 --- a/sys/kern/kern_resource.c +++ b/sys/kern/kern_resource.c @@ -1303,20 +1303,10 @@ uihold(struct uidinfo *uip) void uifree(struct uidinfo *uip) { - int old; - /* Prepare for optimal case. */ - old = uip->ui_ref; - if (old > 1 && atomic_cmpset_int(&uip->ui_ref, old, old - 1)) + if (!refcount_release_lock_rwlock(&uip->ui_ref, &uihashtbl_lock)) return; - /* Prepare for suboptimal case. */ - rw_wlock(&uihashtbl_lock); - if (refcount_release(&uip->ui_ref) == 0) { - rw_wunlock(&uihashtbl_lock); - return; - } - racct_destroy(&uip->ui_racct); LIST_REMOVE(uip, ui_hash); rw_wunlock(&uihashtbl_lock); diff --git a/sys/sys/refcount.h b/sys/sys/refcount.h index 4611664..343da6d 100644 --- a/sys/sys/refcount.h +++ b/sys/sys/refcount.h @@ -64,4 +64,34 @@ refcount_release(volatile u_int *count) return (old == 1); } +#define _refcount_release_lock(count, lock, TYPE, LOCK_OP, UNLOCK_OP) \ +({ \ + TYPE *__lock; \ + volatile u_int *__cp; \ + u_int __old; \ + bool __ret; \ + \ + __lock = (lock); \ + __cp = (count); \ + __old = *__cp; \ + __ret = 0; \ + if (!(__old > 1 && atomic_cmpset_int(__cp, __old, __old - 1))) { \ + LOCK_OP(__lock); \ + if (refcount_release(__cp) == 0) \ + UNLOCK_OP(__lock); \ + else \ + __ret = 1; \ + } \ + __ret; \ +}) + +#define refcount_release_lock_mtx(count, lock) \ + _refcount_release_lock(count, lock, struct mtx, mtx_lock, mtx_unlock) +#define refcount_release_lock_rmlock(count, lock) \ + _refcount_release_lock(count, lock, struct rmlock, rm_wlock, rm_wunlock) +#define refcount_release_lock_rwlock(count, lock) \ + _refcount_release_lock(count, lock, struct rwlock, rw_wlock, rw_wunlock) +#define refcount_release_lock_sx(count, lock) \ + _refcount_release_lock(count, lock, struct sx, sx_xlock, sx_xunlock) + #endif /* ! __SYS_REFCOUNT_H__ */ -- Mateusz Guzik