Repository navigation
motion: fix the split read check on the shared status structs - #4672
Open
grandixximo wants to merge 1 commit into
Open
grandixximo wants to merge 1 commit into
grandixximo wants to merge 1 commit into
Conversation
Task kept a copy of emcmotStatus, emcmotConfig or emcmotInternal when the copied head equalled the copied tail. head is copied first and tail last, so a copy that straddled the start or the end of a write was kept torn; about 0.2% of status reads were, on x86 and ARM. Read tail before the copy and head after it, with release/acquire ordering so ARM cannot reorder the counters and the data, in a new rtapi_seqlock.h used by every writer and reader. The counters widen to unsigned int and stay plain fields, so the structs stay trivially copyable in C++. Also protect the writes that bypassed the counters: five commands that changed emcmotConfig without emcmot_config_change(), so task never re-read them, the controller's writes to emcmotInternal, and emcmotSetCycleTime() running before the controller's window opened. tests/rtapi-seqlock checks rtapi_seqlock_read() against a writer thread and fails on any torn copy kept.
grandixximo
force-pushed
the
motion-split-read
branch
from
October 15, 2026 06:22
6d256eb to
52c66ef
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Task reads
emcmotStatus,emcmotConfigandemcmotInternalby copying the struct and keeping the copy when the copiedheadequals the copiedtail.headis copied first andtaillast, so a copy that straddles the start or end of a motion write is kept torn. Only copies wholly inside a write were rejected.I measured it with markers written at both ends of every status write: about 0.2% of the copies task kept were torn, on x86 locally and on the GitHub x86 and ARM (Neoverse-N2) runners.
Fix:
rtapi_seqlock.h: readtailbefore the copy andheadafter it, with release/acquire ordering. On ARM the right pairing alone still kept torn copies; the fences are needed.emcmot_config_change(), motion-logger) and the three readers inusrmotintf.ccuse it.head/tailwiden fromunsigned chartounsigned int, so a reader stalled mid-copy cannot see the counter wrap back to the same value (256 writes, about 256 ms at a 1 kHz servo thread). They stay plain fields so the structs stay trivially copyable in C++.SET_NUM_SPINDLES,SET_MAX_FEED_OVERRIDE,SETUP_ARC_BLENDS,SET_PROBE_ERR_INHIBITandSELECT_KINS_TYPEchangedemcmotConfigwithoutemcmot_config_change(), so task also never re-read them; the controller wroteemcmotInternal(the planner included) without a window;emcmotSetCycleTime()ran before the controller's window opened.A read now needs a moment with no write under way, long enough to copy the struct (about 6 KB). If the servo thread overruns continuously and nothing runs between
motion-controllerand the nextmotion-command-handler, task gets no status at all, where the old check got torn copies. Other HAL functions aftermotion-controller, as in any config with hardware, leave that gap.Test:
tests/rtapi-seqlockruns a writer thread againstrtapi_seqlock_read()for 3 s and fails on any torn copy kept.runtests -upasses locally.