Syncvar Nees Atomic Loads and Stores

[insertinterestingnamehere]

Our syncvar implementation also needs to be updated to use explicit atomic reads and writes instead of just relying on the x86 memory consistency guarantees.

One example race:

qthreads/src/syncvar.c

Line 175 in d6ce514

tmp = *addr;

and

qthreads/src/syncvar.c

Line 1359 in d6ce514

UNLOCK_THIS_MODIFIED_SYNCVAR(dest, val, SYNCFEB_STATE_FULL_NO_WAITERS);

.

This shows up consistently with the thread sanitizer in the syncvar_prodcons test.

[insertinterestingnamehere]

Similar:

qthreads/src/syncvar.c

Line 268 in d6ce514

syncvar_t local_copy_of_v = *v;

and

qthreads/src/syncvar.c

Line 455 in d6ce514

UNLOCK_THIS_MODIFIED_SYNCVAR(src, ret, SYNCFEB_STATE_EMPTY_WITH_WAITERS);

.

[insertinterestingnamehere]

Okay, it looks like the syncvar code frequently uses the idiom of copying the whole syncvar_t to the local stack to manipulate things there. That's not something we can fix by just replacing accesses with atomic loads and stores since syncvar_t is 16 bytes. We could maybe cast to _Atomic __uint128_t or something like that to get 16 byte atomics, but it's not clear how portable that would be. I'm going to keep mulling this over while fixing some other stuff first.

[insertinterestingnamehere]

Case in point:

qthreads/src/syncvar.c

Lines 263 to 268 in d6ce514

	/* I'm being optimistic here; this only works if a basic 64-bit load is
	* atomic (on most platforms it is). Thus, if I've done an atomic read
	* and the syncvar is unlocked, then I figure I can trust
	* that state and do not need to do a locked atomic operation of any
	* kind (e.g. cas) */
	syncvar_t local_copy_of_v = *v;

Basically the assumption described in that comment isn't true for ARM and this idiom is (rightly) being flagged by the thread sanitizer.

[insertinterestingnamehere]

Alright, I did some more reading on this this morning. Apparently doing mixed-size atomics within the same block of memory is allowed by the x86 and ARM memory models as long as the outermost atomic is not bigger than 128 bits on x86 and 64 bits on ARM. There's a weird sticking point on ARM though where 128 bit loads are sometimes still implemented in libatomic using locks on ARM for backcompat reasons. I haven't tracked down what the C/C++ memory model says about this yet, but it seems like it'd probably be fine.

Another interesting consequence of this idiom: the syncvar struct has an explicit lock anyway which means if we load the whole thing as a 128 bit atomic speculatively but then instead use the lock and non-atomic accesses to the other members we'd be doing mixed atomic and non-atomic reads and writes to the syncvar members other than the lock itself. I suspect the fix is to use atomic reads and writes for the other members too even when they're protected by the lock. At least in theory that should not have significant performance penalties since the thread that's acquired the lock has fresh access to the whole cache line.

[olivier-snl]

It is for situations such as this that we do try to keep most of our structs within one cache line in size.

[insertinterestingnamehere]

Actually this is fixed in #235. It can be closed once that's merged. Long story elsewhere (#215). Essentially we can get by without 128 bit atomics. Mixed-size atomics should be fine on all architectures we're supporting going forward as long as the atomics are actually lock-free.