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

Avoid emulating wider atomics #196

Closed

Conversation

workingjubilee
Copy link

Generally, Rust eschews emulating atomics where it can. This is done precisely so people can write interrupt handlers in Rust, without accidentally arriving in other interrupt handlers, or other complications that such things tend to induce.

No solution is yet proposed for the xtensa-esp32s2-espidf target. However, it would be less-than-ideal if an ecosystem grew up around Rust-on-Xtensa that was dependent on something historically rejected by upstream Rust in the cases where it was easily avoidable.

Generally, Rust eschews emulating atomics where it can.
This is done precisely so people can write interrupt handlers in Rust,
without accidentally arriving in other interrupt handlers,
or other complications that such things tend to induce.

No solution is yet proposed for the xtensa-esp32s2-espidf target.
However, it would be less-than-ideal if an ecosystem grew up around
Rust-on-Xtensa that was dependent on something historically rejected by
upstream Rust in the cases where it was easily avoidable.
@MabezDev
Copy link
Member

Thank you for taking the time to look into this!

Generally, Rust eschews emulating atomics where it can. This is done precisely so people can write interrupt handlers in Rust, without accidentally arriving in other interrupt handlers, or other complications that such things tend to induce.

I would agree with this in general, however we actually discussed this when upstreaming our RISC-V based targets where we decided it was okay because:

a) The implementation is in esp-idf lock-free, and disables all interrupts globally (no chance of preemption when emulating atomic instructions.)
b) The implementation is emitted automatically via __sync and __atomic libcalls. Not something the user has to opt into.

Note that the no_std targets only emit max_atomic_width: Some(32), because esp-idf isn't available there, and hence can't emulate atomics. We did also think about adding the emulation to compiler_builtins, like the arm linux implementation that currently exists there: https://github.com/rust-lang/compiler-builtins/blob/master/src/arm_linux.rs.

@workingjubilee
Copy link
Author

Ah, yet another consensus decision consigned to the artifact of "halfway down a pull request thread", with no other location recorded.

@workingjubilee
Copy link
Author

I would not call "taking a lock on interrupts" as "lock-free" but I suppose I will take that up with upstream.

@workingjubilee
Copy link
Author

Sorry to bother, I was just mildly perplexed that it wasn't somewhat more... discoverable, or clear why the reasons were what they were.

@MabezDev
Copy link
Member

No bother at all! I appreciate you taking the time to look into this. Upon reflection (sleep) I think you are actually correct. The difference between the upstream RISC-V targets, and the Xtensa targets listed here is that these Xtensa targets are dual core, and therefore the 64bits atomic emulation is not lock-free. The impl can be found here. I believe this will be an issue for the upstream RISCV targets soon, as the new ESP32-P4 will be a dual-core RISCV chip.

Not having 64bit atomics available blocks a lot of crates in the std Rust ecosystem, and it does work in practice. What do you suggest? Is this something better discussed upstream?

@workingjubilee
Copy link
Author

Not having 64bit atomics available blocks a lot of crates in the std Rust ecosystem, and it does work in practice. What do you suggest? Is this something better discussed upstream?

Well.

About 64-bit specifically: I think those that want to support 32-bit platforms should probably be using AtomicUsize. There are plenty of crates that will compile for 32-bit platforms but only "support" them in the loosest sense, and using AtomicUsize is the first and obvious step towards actually supporting a 32-bit platform for most "I need an atomic of a decent size" cases. I maintain one such crate, where compiling for 32-bit support is technically possible, but in practice... it's a small miracle it works at all?

But yes, I do think this should be discussed upstream for the RISC-V chip you mentioned. Though it's on your shoulders, as I see it, for how Xtensa currently goes, so I will reopen this. If nothing else, to reflect that there are some open questions still.

@workingjubilee
Copy link
Author

Not having 64bit atomics available blocks a lot of crates in the std Rust ecosystem,

I'd be interested in knowing what crates you're most concerned about losing access to... whether it's just a general smattering of crates or whether we're also talking about some "keystone" crates that a lot of the ecosystem depends on. Given some names and (web) addresses, this sort of problem can be dealt with one way or another.

Your platform, after all, is not the only one that gets shortchanged by having weakened atomic support, here. It's in the greater interest to at least better understand the scale of the problem.

@workingjubilee
Copy link
Author

These targets are about to become the exception amongst ESP targets, so I believe these targets are about to engender more confusion by their existence than not:

@workingjubilee
Copy link
Author

Ah,

From a brief look around the ecosystem, there is better support for 32-bit targets now. tokio was a main concern of mine, but it looks like that should now work with 32-bit targets ootb. There could be an argument to leave the imc target unchanged as only single-core variants exist, but I think it's best to keep them in sync.

Yes, it is quite understandable to be concerned about tokio!

There are also ecosystem crates like portable-atomic that allow shimming a lot of behavior, somewhat similarly to how your current implementation does.

Personally, I am exploring making a crate for better signal handler support. The design I am currently intending is one that will probably need atomics that are lockfree inside the signal handler, as well, which means anything involving recursive interruption would be... subpar.

@workingjubilee
Copy link
Author

My latest commit also narrows ESP32-S2's width, as I believe the rationale for support does fully apply and, while emulation via various schemes is possible, most of those schemes involve making performance harder to reason about.

@ivmarkov
Copy link

ivmarkov commented Nov 9, 2023

UPDATE:

@MabezDev @workingjubilee Shall we close this, given that more or less the same changes were upstreamed in Rust already?

Argh, sorry. I did not realize this is for the xtensa targets. I'm actually fine with this change. I think we should just not claim support for AtomicU64 on any Espressif target (including esp32s2) not only because of the above arguments, but because it is also simpler that way. Even though for single core targets, supporting AtomicU64 is no different from supporting e.g. AtomicU32 - all done by disabling interrupts.

@ivmarkov
Copy link

ivmarkov commented Nov 9, 2023

Personally, I am exploring making a crate for better signal handler support. The design I am currently intending is one that will probably need atomics that are lockfree inside the signal handler, as well, which means anything involving recursive interruption would be... subpar.

A bit offtopic, but... I'm struggling to understand
(a) how signal handling even applies to ESP IDF, given that this platform does not even have the notion of a "process"?
(b) what interrupts in general (and the fact that single-core atomics emulation works by disabling interrupts in particular) has anything to do with signal handling and regular Rust style locks (as in Mutex and friends) in the first place

Can you clarify?

@workingjubilee
Copy link
Author

@ivmarkov The support code involved may see more general usage if I carry my experiment to the end. I'm just expressing a concern that it's impossible to apply bottom-up reasoning if the bottom I'm reasoning from has been undercut.

@MabezDev
Copy link
Member

@workingjubilee I've gone ahead and reflected your changes in the 1.74 release. Thanks for bringing this to my attention!

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.

3 participants