Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surround reference count decrement-branch by release/acquire membars. #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

riastradh
Copy link
Contributor

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.

@rmind
Copy link
Owner

rmind commented Jan 22, 2023

@riastradh: Thanks. However, I think __HAVE_ATOMIC_AS_MEMBAR is really ugly, error-prone and should better be removed.

Can we just use C11 here? I would even use a plain atomic_fetch_add() / atomic_fetch_sub() as performance is really not a concern in this code path (i.e. just keep it straightorward to read!). If needed, we can define NetBSD-specific wrappers in the npf_os.c (under the #ifdef __NetBSD__ fragment).

continue;
#ifndef __HAVE_ATOMIC_AS_MEMBAR
membar_acquire();
#endif
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what happen to npf_table_destroy() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops -- missed a spot while transcribing the patch from NetBSD npf. Fixed with a forced update.

@riastradh riastradh force-pushed the riastradh-20230122-refcntmembar branch from 9c6a255 to 4e543a0 Compare January 22, 2023 20:16
@riastradh
Copy link
Contributor Author

@riastradh: Thanks. However, I think __HAVE_ATOMIC_AS_MEMBAR is really ugly, error-prone and should better be removed.

Can we just use C11 here? I would even use a plain atomic_fetch_add() / atomic_fetch_sub() as performance is really not a concern in this code path (i.e. just keep it straightorward to read!). If needed, we can define NetBSD-specific wrappers in the npf_os.c (under the #ifdef __NetBSD__ fragment).

No real disagreement here. This was part of a much larger change I made in NetBSD to audit reference counts so I could fix bugs promptly -- without taking extra effort to tidy everything up or rework the abstractions, and without incurring a performance penalty on machines that don't need it. I'm just posting all of my commits to npf in NetBSD for posterity so they don't languish as local changes.

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.
@riastradh riastradh force-pushed the riastradh-20230122-refcntmembar branch from 4e543a0 to 9e9a1c2 Compare February 24, 2023 11:07
@rmind
Copy link
Owner

rmind commented Feb 24, 2023

@riastradh: I would encourage you to amend atomic_{inc,dec}_uint{_nv,} in NetBSD to include the memory barriers. Otherwise, such semantics makes code more error-prone and more verbose. It is not worth, especially when acquire/release are free on x86/amd64.

netbsd-srcmastr pushed a commit to NetBSD/src that referenced this pull request Feb 24, 2023
ryo pushed a commit to IIJ-NetBSD/netbsd-src that referenced this pull request Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants