ule: Use explicit atomic accesses for tdq fields

[markj]

Summary

Different fields in the tdq have different synchronization protocols.
Some are constant, some are accessed only while holding the tdq lock,
some are modified with the lock held but accessed without the lock, some
are accessed only on the tdq's CPU, and some are not synchronized by the
lock at all.

Convert ULE to stop using volatile and instead use atomic_load_* and
atomic_store_* to provide the desired semantics for lockless accesses.
This makes the intent of the code more explicit, gives more freedom to
the compiler when accesses do not need to be qualified, and lets KCSAN
intercept unlocked accesses.

Thus:

• Introduce macros to provide unlocked accessors for certain fields.
• Use atomic_load/store for all accesses of tdq_cpu_idle, which is not synchronized by the mutex.
• Use atomic_load/store for accesses of the switch count, which is updated by sched_clock().

No functional change intended.

[jhb]

sys/kern/sched_ule.c

245–246

Was reordering this one on purpose?

278

Not atomic_add_short(..., 1)?

2562–2563

Should this one be an atomic store? Maybe you could write this as:

tdq->tdq_oldswitchcnt = atomic_swap(&tdq_switchcnt, tdq->tdq_load);

3003

I guess with atomic(9) we can't combine the fence with the store? I wonder if C11 atomics would let us do that here?

[markj]

sys/kern/sched_ule.c

245–246

Yes, since tdq_load and tdq_sysload are updated together I had some notion that the compiler could do something clever if they reside in the same double word.

260

This is redundant because tdq_lock is cacheline size-aligned.

278

Hmm. The increment in sched_switch() does not need to be atomic, which is why I wrote it this way. But the one in sched_idletd() perhaps should be. It could be interrupted by sched_clock() or sched_preempt() and then later store a stale value. I'm not sure how harmful that is, though.

3003

Indeed, I don't believe we have an MI atomic(9) primitive for that purpose.

In C11, atomic_store() has seq_cst semantics, and on amd64 it appears to compile to an xchg instruction. I'm not sure if that'd be much better in practice than what we get here, which is a plain store followed by a __storeload_barrier(). And the pairing fence in tdq_notify() doesn't have an associated memory operation.

[mav]

I have no objections, but neither care much. Hope it won't create extra work. tdq_load in cpu_search_*() are extremely hot path.

sys/kern/sched_ule.c

2562–2563

atomic(9):

The atomic_swap() functions are not implemented for the types “char”,
“short”, “ptr”, “8”, and “16” and do not have any variants with memory
barriers at this time.

And I am not sure what would it give us.

[kib]

sys/kern/sched_ule.c

230

In fact would be useful to annotate each field explicitly.

[markj]

In D35737#810658, @mav wrote:

I have no objections, but neither care much. Hope it won't create extra work. tdq_load in cpu_search_*() are extremely hot path.

It shouldn't have any effect on the lockless loads in cpu_search_*.

sys/kern/sched_ule.c

230

I tried to do it, I'm not sure how helpful it is.

[markj]

Add a legend and annotate tdq fields with their synchronization protocols.

[kib]

Thanks.