Date: Tue, 28 Oct 2014 18:44:28 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: John-Mark Gurney <jmg@funkthat.com>, freebsd-arch@freebsd.org Subject: Re: refcount_release_take_##lock Message-ID: <20141028174428.GA12014@dft-labs.eu> In-Reply-To: <201410281154.54581.jhb@freebsd.org> References: <20141025184448.GA19066@dft-labs.eu> <2629048.tOq3sNXcCP@ralph.baldwin.cx> <20141027192721.GA28049@dft-labs.eu> <201410281154.54581.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141028174428.GA12014>