Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Volatile might not be the best (or even right) implementation for atomics #4700

Closed
josephnoir opened this issue Jan 27, 2016 · 6 comments
Closed
Assignees
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care!

Comments

@josephnoir
Copy link
Contributor

As discussed in #4583 this is a separate issue and needs its own thread. The original source for this consideration was a problem with static initialization of structs containing non-literal members (e.g., ones with volatile) in C++.

However, during the following discussion it was questioned if volatile is the right way to implement atomic integers. Here are some of our sources:

And some posts on Stack Overflow:

And related Wikipedia entires:

My current understanding is that memory barriers would be preferable to volatile. However, there are different barriers (compiler and hardware), which prevent memory reordering at different points. ARM architectures seem to have different reordering strategies than x86 ones.

Hardware memory barriers:

Two questions:

  • Would an implementation without volatile be preferable?
  • What would it look like?
@jnohlgard
Copy link
Member

Worth mentioning is that our target platforms, tiny MCUs, are usually without CPU cache, so hardware barriers may not be necessary (or even available) on all platforms.

Added link to ARM document about Cortex-M memory barriers

@jnohlgard jnohlgard added the Area: core Area: RIOT kernel. Handle PRs marked with this with care! label Feb 8, 2016
@jnohlgard
Copy link
Member

I have been thinking of this issue for some time now, and I agree that the current implementation is not optimal.

  • volatile (if used at all) probably fits better as a cast for the immediate operation as opposed to the variable typedef, e.g. in the ATOMIC_VALUE() macro or similar.
  • Optimally, to ease maintenance and less work for new CPUs, we would like the lock-free atomic operations (e.g. STREX/LDREX on Cortex-M) to be a part of the toolchain.

To solve the maintenance problem, we could use the upstream solutions from the toolchain developers:

  • C11 stdatomic.h would likely give us all the desired operations, but requires a switch to -std=c11 and a recent enough compiler: >=gcc-4.8 or >=clang-3.1. This will exclude the current mspgcc (4.6.3) used by Debian and Ubuntu, (and riotdocker). We only need to implement the locking version of the atomic operations (for use on platforms which don't support lock-free atomic operations, or for sizes which are larger than the biggest supported lock-free atomic), but cortex-m etc. will get the atomic operations "for free" as a part of the compiler builtins.
  • GCC's __sync builtins may pose a viable alternative to stdatomic.h. It seems like it is supported by both GCC and Clang, and we will have to implement the same parts as for stdatomic here, only the locking variants of the operations. When eventually the msp-elf-gcc 4.9 toolchain becomes a part of the distributions we could switch to stdatomic.

@josephnoir
Copy link
Contributor Author

Using a maintained implementation sounds like the right thing to do, no need to do this once more--especially when considering how complicated the topic is. The docs say that the compiler gives a warning if __sync is not supported, maybe it would be helpful to see how many of our targets are not supported.

For our implementation this would mean to use __sync or stdatomic.h for our own atomic type which has a fallback for unsupported hardware? And why do we need to implement locking for larger types?

The C11 atomics seem complicated to use. None the less, if we'd move to C11 I think going with the standard is the right thing to do.

@jnohlgard
Copy link
Member

#5688 fixes this by removing atomic.h and replacing it with compiler intrinsics and a helper library for when the hardware is missing support for lockless atomic operations.

@josephnoir
Copy link
Contributor Author

@gebart So ... this issue can be closed, right?

@jnohlgard
Copy link
Member

Yes, I forgot to close after #5688 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care!
Projects
None yet
Development

No branches or pull requests

2 participants