Skip to content

motion: fix the split read check on the shared status structs - #4672

Open
grandixximo wants to merge 1 commit into
LinuxCNC:masterfrom
grandixximo:motion-split-read
Open

grandixximo wants to merge 1 commit into
LinuxCNC:masterfrom
grandixximo:motion-split-read

Conversation

@grandixximo

@grandixximo grandixximo commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Task reads emcmotStatus, emcmotConfig and emcmotInternal by copying the struct and keeping the copy when the copied head equals the copied tail. head is copied first and tail last, 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:

  • New rtapi_seqlock.h: read tail before the copy and head after it, with release/acquire ordering. On ARM the right pairing alone still kept torn copies; the fences are needed.
  • All writers (controller, command handler, emcmot_config_change(), motion-logger) and the three readers in usrmotintf.cc use it.
  • head/tail widen from unsigned char to unsigned 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++.
  • Writes that bypassed the counters: SET_NUM_SPINDLES, SET_MAX_FEED_OVERRIDE, SETUP_ARC_BLENDS, SET_PROBE_ERR_INHIBIT and SELECT_KINS_TYPE changed emcmotConfig without emcmot_config_change(), so task also never re-read them; the controller wrote emcmotInternal (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-controller and the next motion-command-handler, task gets no status at all, where the old check got torn copies. Other HAL functions after motion-controller, as in any config with hardware, leave that gap.

Test: tests/rtapi-seqlock runs a writer thread against rtapi_seqlock_read() for 3 s and fails on any torn copy kept.

x86 ARM
old protocol, 3 s 18k-23k torn kept 75k-190k torn kept
right pairing, no fences, 30 s 0 8.7k-2.3M torn kept
this PR, 30 s 0 of ~32M 0 of ~37M
motion sim with markers, this PR 0 torn of 6.1M 0 torn of 6.2M

runtests -u passes locally.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant