Skip to content

hal: access 64-bit pin values with relaxed atomics - #4683

Open
grandixximo wants to merge 1 commit into
LinuxCNC:masterfrom
grandixximo:hal-atomic-accessors
Open

grandixximo wants to merge 1 commit into
LinuxCNC:masterfrom
grandixximo:hal-atomic-accessors

Conversation

@grandixximo

Copy link
Copy Markdown
Contributor

Follow-up to the discussion in #4614.

Since #4565 every HAL integer pin is 64 bits wide. On 32-bit targets the plain volatile accessors compile to two 32-bit accesses (two movl on i386), so a reader in another thread can see half of a write; at a zero crossing that is a value off by 2^32. With one thread toggling a sint pin between -1 and 0 and another reading it, i386 shows millions of torn reads per second, and about 30 per second even pinned to one CPU. A real path is a userspace jog wheel such as shuttle writing counts that the servo thread reads as joint.N.jog-counts.

This PR:

  • Uses __atomic_load_n/__atomic_store_n (__atomic_load/__atomic_store for real) with __ATOMIC_RELAXED in the hal.h getters and setters. On amd64 and arm64 the generated code is the same single load or store as before. On i386 it is fildq/fistpq, on ARMv7 ldrexd/strexd. The same test sees no torn values. Real pins become atomic on ARMv7 too.
  • Adds a configure check, as Bertho asked: without lock-free 64-bit atomics the compiler falls back to libatomic, whose locks are process-local and do not protect HAL shared memory, so configure fails on such targets (e.g. i486, ARM before ARMv6K).

Tested in my fork together with the 32-bit CI job from #4682: rip-and-test 340/340, i386 and armhf 333/333.

Comment thread src/hal/hal.h
Comment thread src/configure.ac
@BsAtHome

Copy link
Copy Markdown
Contributor

And you have a conflict after the include change

Comment thread include/hal.h
Since the move to 64-bit HAL types every integer pin is 64 bits wide.
On 32-bit targets the plain volatile accessors compile to two 32-bit
accesses (two movl on i386), so a reader in another thread can see half
of a write: at a zero crossing the value is off by 2^32. A test with
one thread toggling a sint pin between -1 and 0 and another reading it
sees millions of torn values per second on i386, and still about 30 per
second pinned to one CPU. A real path is a userspace jog wheel (e.g.
shuttle) writing counts that the servo thread reads as
joint.N.jog-counts.

Use __atomic_load_n/__atomic_store_n (__atomic_load/__atomic_store for
real) with __ATOMIC_RELAXED in the getters and setters. On amd64 and
arm64 this compiles to the same single loads and stores as before; on
i386 it is fildq/fistpq and on ARMv7 ldrexd/strexd. The same test sees
no torn values. This also makes real pins atomic on ARMv7, where
vldr/vstr were not guaranteed to be.

Without lock-free 64-bit atomics the compiler falls back to libatomic,
whose locks are process-local and do not protect HAL shared memory, so
configure now fails on such targets (e.g. i486, ARM before ARMv6K), as
requested by Bertho Stultiens in LinuxCNC#4614.
@grandixximo

Copy link
Copy Markdown
Contributor Author

Rebased onto the include move; the change is now in include/hal.h.

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.

2 participants