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

Move to atomic_polyfill from radium #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

dberlin
Copy link

@dberlin dberlin commented Mar 19, 2023

Radium is a nice library for possible atomics, but it is really not usable
in embedded no_std environments. Version 1.0 also significantly changes the
API, and isn't really what bitvec wants (which is atomics that work everywhere).
It instead has "native atomics" and "best-effort atomics".

The standard way to make atomics work everywhere is now to use atomic_polyfill,
which makes atomics available either using native atomics or through polyfills
that use critical sections, and is a drop in replacement for core's Atomic*;

This works on essentially all supported rust environments, both no_std and not.

Radium 0.7 does not work on no_std esp32, for example.

This patch makes bitvec use atomic_polyfill rather than radium.

It requires bumping the minimum rust version slightly from 1.56 to 1.60.

I have not finished updating the docs, wanted to see what you think first.

@dberlin
Copy link
Author

dberlin commented Mar 19, 2023

(It looks like there are some spurious formatting changes due to format on save and not picking up the rustfmt file that matches the old style. I'll fix them)

@dberlin
Copy link
Author

dberlin commented Mar 19, 2023

(should be fixed now)

@myrrlyn
Copy link
Contributor

myrrlyn commented Apr 30, 2023

I'm not very familiar with what the embedded-systems part of the ecosystem is up to, which is unfortunate because I'm very interested in that kind of work. So I'd never heard of this, and I really appreciate you bringing it to my attention! Here are my current hold-ups: I'll keep reading this crate and see if I can bridge it myself, but I wanted to write down my immediate thoughts so that both you and I know what I'm thinking and can refer to it in the future.


  1. bitvec needs to be able to switch between Atomic types and Cell<> types at runtime, in order to correctly handle memory that is only known to be not-aliased at runtime (such as through subslicing). No matter what else we do, I'll need to have the ability to dynamically disable both atomic and IRQ-control instructions when the memory layout doesn't actually require them.
  2. bitvec needs to be able to support targets that don't have ISA atomic instructions, but do have IRQ-control instructions, and allow programs to simulate atomics by locking the processor into a single control flow.
  3. I don't want to manually maintain a build script that tries to inspect the compilation target and swap out symbols according to that result. We tried that in radium v0 and, as you rightfully point out, we had some misses. I'm leery of other people manually updating a build script against compiler definitions, but being able to hand off bug reports to somebody else is a Good Enough solution for me :p

I think the most useful action here is not to replace radium with atomic_polyfill in bitvec's usage, but instead to make radium depend on atomic_polyfill and create the following order of symbol routing: radium::RadiumT is

  1. core::sync::atomic::AtomicT when this symbol exists, otherwise
  2. atomic_polyfill::polyfill::AtomicT that symbol exists, otherwise
  3. Cell<T>

If I understand correctly, a_p already does the first two actions: a_p::AtomicT routes to core when that exists and to itself when that does not. The build script looks like it is all-or-nothing, so I'm uncertain about whether targets that have some, but not all, atomic instructions (a) are handled correctly or (b) even exist and need to be handled.

I think that from a semantic perspective, programs shouldn't particularly care whether an atomic memory access uses ISA instructions or an IRQ blink: all that matters is that the access is not contended by other writes. As such, I feel pretty comfortable making radium silently switch between core and a_p implementations without exposing this difference to the user, and then bitvec can get this behavior for free.

Does that make sense?


Also, we are already planning to lift MSRV to 1.60 for radium v1 or even to 1.65 for funty v3. My informal policy is "try to stay at least four versions behind stable" but I formally do not make promises about keeping the MSRV low.

Again: thank you for bringing this to my attention. I definitely want to use a_p; I just want to see if I can jam it into radium first before giving up on that entirely. Switching at runtime between atomics and cells is, apparently, a surprisingly important capability! I had some users argue very strongly to get me to make that possible.

@dberlin
Copy link
Author

dberlin commented Apr 30, 2023

I'm not very familiar with what the embedded-systems part of the ecosystem is up to, which is unfortunate because I'm very interested in that kind of work. So I'd never heard of this, and I really appreciate you bringing it to my attention! Here are my current hold-ups: I'll keep reading this crate and see if I can bridge it myself, but I wanted to write down my immediate thoughts so that both you and I know what I'm thinking and can refer to it in the future.

  1. bitvec needs to be able to switch between Atomic types and Cell<> types at runtime, in order to correctly handle memory that is only known to be not-aliased at runtime (such as through subslicing). No matter what else we do, I'll need to have the ability to dynamically disable both atomic and IRQ-control instructions when the memory layout doesn't actually require them.
  2. bitvec needs to be able to support targets that don't have ISA atomic instructions, but do have IRQ-control instructions, and allow programs to simulate atomics by locking the processor into a single control flow.
  3. I don't want to manually maintain a build script that tries to inspect the compilation target and swap out symbols according to that result. We tried that in radium v0 and, as you rightfully point out, we had some misses. I'm leery of other people manually updating a build script against compiler definitions, but being able to hand off bug reports to somebody else is a Good Enough solution for me :p

I think the most useful action here is not to replace radium with atomic_polyfill in bitvec's usage, but instead to make radium depend on atomic_polyfill and create the following order of symbol routing: radium::RadiumT is

  1. core::sync::atomic::AtomicT when this symbol exists, otherwise
  2. atomic_polyfill::polyfill::AtomicT that symbol exists, otherwise
  3. Cell<T>

If I understand correctly, a_p already does the first two actions: a_p::AtomicT routes to core when that exists and to itself when that does not. The build script looks like it is all-or-nothing, so I'm uncertain about whether targets that have some, but not all, atomic instructions (a) are handled correctly or (b) even exist and need to be handled.

I think that from a semantic perspective, programs shouldn't particularly care whether an atomic memory access uses ISA instructions or an IRQ blink: all that matters is that the access is not contended by other writes. As such, I feel pretty comfortable making radium silently switch between core and a_p implementations without exposing this difference to the user, and then bitvec can get this behavior for free.

Does that make sense?

Also, we are already planning to lift MSRV to 1.60 for radium v1 or even to 1.65 for funty v3. My informal policy is "try to stay at least four versions behind stable" but I formally do not make promises about keeping the MSRV low.

Again: thank you for bringing this to my attention. I definitely want to use a_p; I just want to see if I can jam it into radium first before giving up on that entirely. Switching at runtime between atomics and cells is, apparently, a surprisingly important capability! I had some users argue very strongly to get me to make that possible.

Hey, I totally get it.
What you are suggesting makes sense to me. The only thing that gives me a little pause at all is the use of irq control/blink instructions for atomics. I get it. Like you said, you want this stuff to just work, and people to not worry about it.

Thinking through it out loud, I don't think it would not be crazy to expect that trying to treat an atomic as a bitvec and set or change some bits in an irq handler, which i expect will break on those platforms if they are trying to use irqs to simulate atomics.

Now, at the same time, it's bare metal programming anyway, I think if you are this bare metal (no std, etc), you probably know what's going on in your platform enough to realize this would be an issue. I also guess if Radium has to do it this way, it's not like there is some magic other way, so whether you use radium/bitvec or lower level munging, it won't work any better/worse that way anyway.

So yeah, makes sense.
Let me know if and where I can help - I appreciate you taking the time to write a library that owkrs really well, and tries to carefully think through the issues - it doesn't happen enough, and i know it also can be a huge time sink to try to do it right sometimes ;)

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.

None yet

2 participants