Use atomic loads/stores when updating td->td_state

[arichardson]

Summary

KCSAN complains about racy accesses in the locking code. Those races are
fine since they are inside a TD_SET_RUNNING() loop that expects the value
to be changed by another CPU.

Use relaxed atomic stores/loads to indicate that this variable can be
written/read by multiple CPUs at the same time. This will also prevent
the compiler from doing unexpected re-ordering.

Reported by: GENERIC-KCSAN

Test Plan

KCSAN no longer complains, kernel still runs fine.

[mjg]

There are several other places which play with ->td_state, if rolling with atomic_* they all should be patched. Preferably there would be an atomic_int_t type or similar so that unpatched cases would fail to compile.

Maybe I should rephrase. The day is coming where someone will add the type and then this change will break if it does not take care of all uses.

sys/sys/proc.h

66

comments like this notoriously get stale

599

I think this should be TD_SET_STATE(td, state) and used by all of these and other places.

[arichardson]

Use macro accessors and wrap in a struct to prevent non-atomic accesses

[arichardson]

sys/sys/proc.h

599

Yes, much better. Thanks for the suggestion.

[mjg]

Assuming this runs and builds with tinderbox LGTM modulo the union comment.

sys/sys/proc.h

355

If this was added to induce compilation failures for plain td_state access it needs a comment.

I don't have an opinion one way or the other if this should be done here.

[arichardson]

Haven't done tinderbox yet, but grep shows there are no uses for other architecture:

sys/dev/twe/twe.c:    if (twe_get_param_1(sc, table, TWE_PARAM_UNITINFO_Status, &dr->td_state)) {
sys/dev/twe/twe_freebsd.c:      twe_describe_code(twe_table_unitstate, dr->td_state & TWE_PARAM_UNITSTATUS_MASK));
sys/dev/twe/twevar.h:    u_int8_t       td_state;
sys/sys/proc.h:     enum td_states {
sys/sys/proc.h: } td_state;         /* (t) thread state */
sys/sys/proc.h:#define  TD_GET_STATE(td)    atomic_load_int(&(td)->td_state.value)
sys/sys/proc.h:#define  TD_SET_STATE(td, state) atomic_store_int(&(td)->td_state.value, state)

Will add the comment before committing this.

[markj]

sys/sys/proc.h

355

I would guess it'll also break some number of ports. Probably not many, but to me it's an argument against the name change.

544–552

I would define separate implementations for _KERNEL and !_KERNEL rather than pulling in atomic.h.

[arichardson]

sys/sys/proc.h

355

Good point, I guess the wrapper struct could either be only for _KERNEL, or I just drop it entirely now that I've found all uses.

544–552

Sounds good, will do so later this week.

[jhb]

sys/sys/proc.h

355

It will break devel/mdb as mdb uses some weird hacks where it create mirror structures with a subset of members and depends on the CTF data for its own version and the CTF data in the kernel for the kernel's version to copy the right fields of the kernel structure into its mirrored structure. However, this depends on 1:1 correspondence of structures and is a PITA to deal with. (kgdb wouldn't care either way)

592–593

[arichardson]

Avoid atomics for !_KERNEL