Provide generic sub-word atomic *cmpset

[kevans]

Summary

Again, naive attempt... this would be broken into three commits, and it seems to work in userland, at least.

Provide *cmpset_{8,16} that proxy through to atomic_fcmpset_32. Initial users will be mips and sparc64, with hopes to wean mips off of it later.

Initially tried to provide these in atomic_common with an #ifdef PLATFORM_NEEDS_SUBWORD_OPS, but this ends up being cleaner with only two archs using it and allows the possibility of KASSERT if we want to do so as systm.h needs bits from machine/atomic.h.

Most of the diff in machine/atomic.h are to refactor definitions of _acq/_rel versions.

[kib]

sys/sys/_atomic_subword.h

53

You need () around p.

57

Perhaps 8 should be NBBY if the symbol is availiable (ignore my note if not).

78

This should be static inline. Or static (non-inline) in C file.

93

I think what you describe there is a good reason to not implement cmpset(), only adding fcmpset().

First, look at the atomic(9) note for fcmpset(), where it is mentioned that the function can fail spuriously. Next, lets calculate loops: on ll/sc arch like mips, cmpset already loops in the asm, then you are adding a loop around the call, and then the caller would loop around that.

Hm, I am wrong there, it would be only two nested loops since you use fcmpset() and fcmpset() does not retry, But my point should be clear: do not loop in your implementation, leave it to the caller and allow spurious failures.

[kevans]

sys/sys/_atomic_subword.h

57

Hmm... The odds are good, considering mandatory include order. I'd be tempted to #ifndef ... #define it to 8 for those cases that may have only included sys/types.h to make this use clear.

93

I have to wonder why the note isn't written to include cmpset. The same caveat about ll/sc applies and the caller has to re-read to detect that. It doesn't note anything to the contrary, such as cmpset being guaranteed to have successfully written. I think it should be amended to clarify intent on cmpset, perhaps.

The mips fcmpset actually will retry on write failure though, from my reading. I'll eliminate the loop either way.

[kib]

sys/sys/_atomic_subword.h

93

I think ARM/ARM64 are much better examples for ll/sc arches, in the sense that more efforts go into them. There, fcmpset family does not retry on sc transient failure, but cmpset retries. (fcmpset on arm64 has seemingly unused '1' label which it got by copy/paste, perhaps).

[kevans]

sys/sys/_atomic_subword.h

93

Yeah, ok- RISCV/ARM/ARM64 all seem to agree on cmpset/fcmpset spinning internally (the former spins, the latter not)

From my reading, cmpset *could* still be implemented with this function and the fcmpset version spawned off and not looping.

Is it better not to provide cmpset in that case? mjg just needs fcmpset, right?

[kib]

sys/sys/_atomic_subword.h

93

Or fix fcmpset on mips to not loop.

[kevans]

An earlier version of the mips fcmpset fix snuck in here... Will fix in another revision, just putting this one out.

Key points:

• addressed various review comments
• fixed include guard
• shuffled systm.h around to allow use of KASSERT in machine/atomic.h ... can scratch this idea if it looks like a terrible one
• Translated KASSERT to userland assert

[kevans]

sys/sys/_atomic_subword.h

168

This seems to have snuck in during local merge conflicts, detected in testing... Will fix after it's decided whether or not systm.h shuffling is OK.

[kib]

sys/sys/systm.h

44

I would leave systm.h alone and not used KASSERT() in atomic.h. We do not do that on other arches. Also, I suspect that unaligned access would be reported by hw regardless.

[kevans]

Stop mucking with sys/systm.h, drop all assertions and fix the s/HWORD/BYTE/ typo that snuck in.

[jhibbits]

This will simplify D21682 a bit, as well. PowerPC has sub-word atomics since PowerISA 2.06, so we'll need dual personality there.

[mjg]

I think this looks fine, but would preferably take care of all ports which are currently missing these atomics. There are reviews pending for powerpc and arm but they seem a little stalled. Thus the proposed solution is to just add this for everyone and let maintainers remove it as they provide their own version.

With this in mind, I think a passing a passing make universe with the above addition + D21626 mixed with at least sanity testing by booting would make the patch more qualified for commit.