122a3e8a60
If two threads are using an object that is freed when the reference count goes to zero, we need to ensure that all memory operations related to the object happen before freeing the object. Using an atomic_dec_uint_nv(&refcnt) == 0 ensures that only one thread takes responsibility for freeing, but it's not enough to ensure that the other thread's memory operations happen before the freeing. Consider: Thread A Thread B obj->foo = 42; obj->baz = 73; mumble(&obj->bar); grumble(&obj->quux); /* membar_exit(); */ /* membar_exit(); */ atomic_dec -- not last atomic_dec -- last /* membar_enter(); */ KASSERT(invariant(obj->foo, obj->bar)); free_stuff(obj); The memory barriers ensure that obj->foo = 42; mumble(&obj->bar); in thread A happens before KASSERT(invariant(obj->foo, obj->bar)); free_stuff(obj); in thread B. Without them, this ordering is not guaranteed. So in general it is necessary to do membar_exit(); if (atomic_dec_uint_nv(&obj->refcnt) != 0) return; membar_enter(); to release a reference, for the `last one out hit the lights' style of reference counting. (This is in contrast to the style where one thread blocks new references and then waits under a lock for existing ones to drain with a condvar -- no membar needed thanks to mutex(9).) I searched for atomic_dec to find all these. Obviously we ought to have a better abstraction for this because there's so much copypasta. This is a stop-gap measure to fix actual bugs until we have that. It would be nice if an abstraction could gracefully handle the different styles of reference counting in use -- some years ago I drafted an API for this, but making it cover everything got a little out of hand (particularly with struct vnode::v_usecount) and I ended up setting it aside to work on psref/localcount instead for better scalability. I got bored of adding #ifdef __HAVE_ATOMIC_AS_MEMBAR everywhere, so I only put it on things that look performance-critical on 5sec review. We should really adopt membar_enter_preatomic/membar_exit_postatomic or something (except they are applicable only to atomic r/m/w, not to atomic_load/store_*, making the naming annoying) and get rid of all the ifdefs. |
||
|---|---|---|
| .. | ||
| Makefile | ||
| README | ||
| files.npf | ||
| if_npflog.c | ||
| if_npflog.h | ||
| lpm.c | ||
| lpm.h | ||
| npf.c | ||
| npf.h | ||
| npf_alg.c | ||
| npf_alg_icmp.c | ||
| npf_bpf.c | ||
| npf_conf.c | ||
| npf_conn.c | ||
| npf_conn.h | ||
| npf_conndb.c | ||
| npf_connkey.c | ||
| npf_ctl.c | ||
| npf_ext_log.c | ||
| npf_ext_normalize.c | ||
| npf_ext_rndblock.c | ||
| npf_handler.c | ||
| npf_if.c | ||
| npf_ifaddr.c | ||
| npf_impl.h | ||
| npf_inet.c | ||
| npf_mbuf.c | ||
| npf_nat.c | ||
| npf_os.c | ||
| npf_params.c | ||
| npf_portmap.c | ||
| npf_rproc.c | ||
| npf_ruleset.c | ||
| npf_sendpkt.c | ||
| npf_state.c | ||
| npf_state_tcp.c | ||
| npf_tableset.c | ||
| npf_worker.c | ||
| npfkern.h | ||
README
The NPF project upstream repository: https://github.com/rmind/npf/ Please submit the pull requests to the upstream when possible.