rms: Order a writer's stores before its readers see it leave

[rcm]

Summary

Readers check rms->writers without a fence, and rms_wunlock() drops
it with a plain store. rms_wlock() interrupts every CPU before the
first writer blocks readers, but nothing orders them when the last
writer leaves: on a weakly ordered CPU a reader can see the count
reach zero and still read what the writer replaced, such as callbacks
of a module unloaded under the lock.

Fence every CPU before the last writer drops the count: rename
rms_wlock_switch() to rms_block_readers() and add
rms_unblock_readers(), a wrapper for cpus_fence_seq_cst().

Readers stay fence-free; the cost lands on the (rare) unlock.

Fixes: 1f162fef76a0 ("Add read-mostly sleepable locks")
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")

[kib]

sys/kern/kern_rmlock.c

1040

I think that the acq load is only needed when we do not fallback to the slow path. Then instead of unconditionally do load_acq() above, you could replace this interrupt fence with the atomic_thread_fence_acq(), and same for the trylock.

[mjg]

While I agree the fence is missing, I completely disagree with the fix.

First a nit that converting writers to u_int renders the ->writers >= 0 asserts moot.

My complaint is that this avoidably penalizes said weakly-ordered archs when using a primitive which is almost never expected to write-lock.

A fix which would take care of everything and avoid penalisation would use IPIs when getting rid of the last writer, just like IPIs are used for the first writer.

Say rename rms_wlock_switch to rms_block_readers and add rms_unblock_readers which issues IPI with seq cst. there was a routine somewhere which precisely that, you can just wrap it.

[rcm]

rms_wunlock() now interrupts all CPUs before the last writer drops the count, as rms_wlock_switch() does when the first one comes, and the read path is back to what it was, with no fence. writers stays int.

For the record, kib's variant measured 21-59% slower on arm64 and the acquire load was free, but this costs readers nothing at all, so it is better than both.

[rcm]

In D60658#1391083, @mjg wrote:

While I agree the fence is missing, I completely disagree with the fix.

First a nit that converting writers to u_int renders the ->writers >= 0 asserts moot.

My complaint is that this avoidably penalizes said weakly-ordered archs when using a primitive which is almost never expected to write-lock.

A fix which would take care of everything and avoid penalisation would use IPIs when getting rid of the last writer, just like IPIs are used for the first writer.

Say rename rms_wlock_switch to rms_block_readers and add rms_unblock_readers which issues IPI with seq cst. there was a routine somewhere which precisely that, you can just wrap it.

Actually yeah your suggestion sounds better. I will re-work a v3.

[rcm]

Rename rms_wlock_switch() to rms_block_readers() and add rms_unblock_readers(), which wraps cpus_fence_seq_cst(), before the last writer drops the count, as suggested. The readers and the writers field are back to what they were.