-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
ESP-IDF targets' Atomic*64 is not lock-free #117305
Comments
WG-prioritization assigning priority (Zulip discussion). @rustbot label -I-prioritize +P-low |
Set max_atomic_width for riscv32*-esp-espidf to 32 Fixes rust-lang#117305 > Since riscv32 does not have 64-bit atomic instructions, I do not believe there is any way to fix this problem other than setting max_atomic_width of these targets to 32. This is a breaking change because Atomic\*64 will become unavailable, but all affected targets are tier 3, and the current Atomic*64 violates the standard library's API contract and can cause problems with code that rely on the standard library's atomic types being lock-free. r? `@Amanieu` cc `@ivmarkov` `@MabezDev`
Sorry to revive this old issue. At the time I just trusted your judgement when you removed the The problem is, we now we have this new issue - and - without diving into the details of the new issue just yet - we (or I) really need to understand what exactly a "lock-free" atomic does mean and how exactly an atomic that is allowed to "wait" is different from a "lockful" atomic (all atomics in To give you a concrete example where I struggle: for the But why did you consider this spinlock a "lock" and not a "wait" (as per the terminology of the The spinlock implementation is essentially (in pseudocode): static hardware_atomic_bool_spinlock; // A `bool` **hardware** atomic declared statically somewhere
fn software_cmpxchg_for_something_big(something: *mut SomethingBig, old: SomethingBig, new: SomethingBig) -> SomethingBig {
disable_all_interrupts_for_the_core_that_runs_this_code();
while hardware_cmpxchg(&hardware_atomic_bool_spinlock, false, true) == true {
// Ah, the spinlock is set to true; must be the other core; re-try in a busy-loop until we succeed
}
// Here we have interrupts disabled for our core, _and_ we have indicated to the other core (via the `hardware_atomic_bool_spinlock`) to wait until we do our job so we can just manipulate `something` now
let current = unsafe { *something };
if current == old {
unsafe { *something = new; }
}
let ret = hardware_cmpxchg(&hardware_atomic_bool_spinlock, true, false);
assert!(ret); // Should always return `true`
enable_all_interrupts_for_the_core_that_runs_this_code();
ret
} So why is the above ^^^ a "lock" and not just a "wait"? And could you give an example of a "wait" which is not a "lock"? |
To quote wikipedia
Your A lock-free but non-wait-free implementation allows some threads get stuck, but still requires that at least one thread can continue at some point. Only a non-lock-free implementation is allowed to have all threads that attempt to perform atomic operations to get permanently stuck. |
So are you saying that
|
As for
Wouldn't that mean the whole "user-space" (non-interrupt code) = all threads would starve, and nothing would make progress anyway. Including threads that utilize native atomics only. Asking, because I don't see how that particular example (NMIs firing back to back) applies here, as it would (obviously?) starve absolutely everything else, hardware atomics or not? |
I think it would be assuming that the underlying CPU core ensures that it will always keep making forward progress itself. So for example another core must not be able to shutdown any core that has
Even an NMI handler which immediately returns would make it non-lock-free if there is a possibility that NMI's arrive back to back. If however the CPU guarantees to execute at least one instruction between returning from an NMI and calling the NMI handler again (eg because the only source of NMI's is a watchdog timer and the NMI handler is fast enough), then it should be fine when the NMI handler doesn't touch
With lock-free atomics if you suspend all userspace threads except for one, that one thread will always be able to make forward progress with regards to executing atomic operations. In case of a non-lock-free implementation it is possible that it blocks forever waiting on one of the suspended threads releasing the lock. |
I think I might understand it a bit now. =====================
=====================
Note also that the condition that the NMI handlers should not touch such software atomics is a MUST for this case (2), or else such a software implementation would of course be unsound. It is a must for the "lockful" implementation in case (1) from above as well, but for other reason (the NMI might deadlock). |
Both examples are correct afaik. |
max_atomic_width of these targets is currently set to 64
rust/compiler/rustc_target/src/spec/riscv32imac_esp_espidf.rs
Line 20 in 0237aa3
rust/compiler/rustc_target/src/spec/riscv32imc_esp_espidf.rs
Line 23 in 0237aa3
However, as mentioned in #115577 (comment), esp-idf's 64-bit atomic builtins use global spinlocks on multi-core systems:
https://github.com/espressif/esp-idf/blob/3b748a6cb76c2db7c6368a0dea32a88bc58bc44d/components/newlib/stdatomic.c#L64
And it violates what the standard library is intended to guarantee.
https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#portability
Since riscv32 does not have 64-bit atomic instructions, I do not believe there is any way to fix this problem other than setting max_atomic_width of these targets to 32.
cc @ivmarkov @MabezDev (mentioned because you both are target maintainers)
@rustbot label +A-atomic +I-unsound
The text was updated successfully, but these errors were encountered: