Skip to content

Commit

Permalink
Surround reference count decrement-branch by release/acquire membars.
Browse files Browse the repository at this point in the history
In order to ensure that that all users of an object have finished all
access to it before the last one actually frees it, we need the
decrement to have release/acquire semantics to establish a
happens-before relation.

	... x->f = 42 ... x->f ...		/* A */
	if (--x->refcnt != 0)	/* atomic */
	        return;
	x->a = x->b = ... = x->f = 0;		/* B */
	free(x);

To guarantee that A in one thread happens-before B in another thread,
we need the reference count decrement (and branch) to have
release/acquire semantics, so put membar_release before and
membar_acquire after.  (We could use memory_order_acq_rel with
atomic_fetch_add_explicit in C11.)

Note: membar_producer and membar_consumer are not enough, because
they only order stores on one side and loads on the other, whereas it
is necessary to order loads and stores on both sides.

v2: Nix __HAVE_MEMBAR_AS_ATOMIC.
  • Loading branch information
Taylor R Campbell committed Feb 24, 2023
1 parent 52dbeac commit 9e9a1c2
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 3 deletions.
2 changes: 2 additions & 0 deletions src/kern/npf_nat.c
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,11 @@ npf_natpolicy_release(npf_natpolicy_t *np)
{
KASSERT(atomic_load_relaxed(&np->n_refcnt) > 0);

membar_release();
if (atomic_dec_uint_nv(&np->n_refcnt) != 0) {
return;
}
membar_acquire();
KASSERT(LIST_EMPTY(&np->n_nat_list));
mutex_destroy(&np->n_lock);
kmem_free(np, sizeof(npf_natpolicy_t));
Expand Down
2 changes: 2 additions & 0 deletions src/kern/npf_rproc.c
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,11 @@ npf_rproc_release(npf_rproc_t *rp)
{
KASSERT(atomic_load_relaxed(&rp->rp_refcnt) > 0);

membar_release();
if (atomic_dec_uint_nv(&rp->rp_refcnt) != 0) {
return;
}
membar_acquire();
/* XXXintr */
for (unsigned i = 0; i < rp->rp_ext_count; i++) {
npf_ext_t *ext = rp->rp_ext[i];
Expand Down
10 changes: 7 additions & 3 deletions src/kern/npf_tableset.c
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,13 @@ npf_tableset_destroy(npf_tableset_t *ts)
for (u_int tid = 0; tid < ts->ts_nitems; tid++) {
npf_table_t *t = ts->ts_map[tid];

if (t && atomic_dec_uint_nv(&t->t_refcnt) == 0) {
npf_table_destroy(t);
}
if (t == NULL)
continue;
membar_release();
if (atomic_dec_uint_nv(&t->t_refcnt) > 0)
continue;
membar_acquire();
npf_table_destroy(t);
}
kmem_free(ts, NPF_TABLESET_SIZE(ts->ts_nitems));
}
Expand Down
2 changes: 2 additions & 0 deletions src/kern/stand/npf_stand.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ npfkern_atomic_swap_ptr(volatile void *ptr, void *newval)
#define membar_sync() __sync_synchronize()
#define membar_consumer() __sync_synchronize()
#define membar_producer() __sync_synchronize()
#define membar_release() __sync_synchronize()
#define membar_acquire() __sync_synchronize()
#define atomic_inc_uint(x) __sync_fetch_and_add((x), 1)
#define atomic_inc_uint_nv(x) __sync_add_and_fetch((x), 1)
#define atomic_inc_ulong_nv(x) __sync_add_and_fetch((x), 1)
Expand Down

0 comments on commit 9e9a1c2

Please sign in to comment.