td_state
ClosedPublic
Actions

Authored by arichardson on Feb 10 2021, 11:14 AM.

Details

Reviewers

kib
jhb
markj
mjg
andrew

Commits

rGfa2528ac6435: Use atomic loads/stores when updating td->td_state

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.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 37089
Build 33978: arc lint + arc unit

Event Timeline

arichardson created this revision.Feb 10 2021, 11:14 AM

Herald added a subscriber: imp. · View Herald TranscriptFeb 10 2021, 11:14 AM

arichardson requested review of this revision.Feb 10 2021, 11:14 AM

Harbormaster completed remote builds in B36853: Diff 83629.Feb 10 2021, 11:14 AM

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.

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

Herald added a subscriber: emaste. · View Herald TranscriptFeb 10 2021, 11:45 AM

Harbormaster completed remote builds in B36854: Diff 83631.Feb 10 2021, 11:46 AM

arichardson edited the summary of this revision. (Show Details)Feb 10 2021, 11:46 AM

arichardson added inline comments.

sys/sys/proc.h
599	Yes, much better. Thanks for the suggestion.

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.

This revision is now accepted and ready to land.Feb 10 2021, 11:50 AM

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 added inline comments.Feb 10 2021, 3:18 PM

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 added inline comments.Feb 10 2021, 3:22 PM

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 added inline comments.Feb 16 2021, 7:04 PM

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