Add INVARIANTS-only fences to synchronize lockless refcount updates.
ClosedPublic
Actions

Authored by markj on Aug 16 2018, 9:16 PM.

Details

Reviewers

kib
mjg
rlibby
jhibbits

Commits

rS337974: Add INVARIANTS-only fences around lockless vnode refcount updates.

Summary

This is based on the discussion around the (now reverted) r334708.
Some internal routines inspect v_iflags after a lockless refcount bump.
In general the locking protocol requires the interlock for this, and
without it memory reordering introduces races that can cause spurious
assertion failures. Add some INVARIANTS-only macros to address some
known races. I don't claim that this is an exhaustive fix, though.
Please feel free to suggest alternate names for the macros.

Test Plan

Ask Justin to test.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Aug 16 2018, 9:16 PM

Harbormaster completed remote builds in B18872: Diff 46802.Aug 16 2018, 9:16 PM

markj edited the test plan for this revision. (Show Details)Aug 16 2018, 9:17 PM

markj added reviewers: kib, mjg, rlibby.

markj added a subscriber: jhibbits.

Please add comments explaining that these fences are used to ensure visibility of the v_iflags vs v_holdcnt updates.

This revision is now accepted and ready to land.Aug 16 2018, 9:59 PM

Survives a buildworld, something it couldn't do before.

Make the comment more explicit.

This revision now requires review to proceed.Aug 16 2018, 10:53 PM

Harbormaster completed remote builds in B18877: Diff 46807.Aug 16 2018, 10:53 PM

I think the current names while justifiable only add confusion (why is refcount *acquire* paired with fence *rel*?). How about 'VNODE_REFCOUNT_FENCE_BEFORE' and after.

No strong opinions though.

This revision is now accepted and ready to land.Aug 17 2018, 2:53 PM

In D16756#356654, @mjg wrote:

I think the current names while justifiable only add confusion (why is refcount *acquire* paired with fence *rel*?). How about 'VNODE_REFCOUNT_FENCE_BEFORE' and after.

I'm not crazy about the names, but I do think it's reasonably clear that "acquire" refers to the fence and not a refcount operation. At least, the naming is consistent with atomic(9). _BEFORE and _AFTER is tied too closely to the existing uses IMHO. We might want to add uses where those suffixes don't make much sense.

Closed by commit rS337974: Add INVARIANTS-only fences around lockless vnode refcount updates. (authored by markj). · Explain WhyAug 17 2018, 3:41 PM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: imp. · View Herald TranscriptAug 17 2018, 3:41 PM