libcore: expose volatile atomic operations - #161301
Conversation
|
Any special-casing of Miri in the standard library requires review. cc @rust-lang/miri |
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
b33f0bb to
91c5ef3
Compare
This comment has been minimized.
This comment has been minimized.
d212fb0 to
d49ab8f
Compare
This comment has been minimized.
This comment has been minimized.
bfe8c00 to
151703b
Compare
| /// } | ||
| /// }; | ||
| /// // Synchronize with the store whose value we just read. | ||
| /// // Note: a standard acquire fence may not be sufficient to synchronize with DMA devices. |
There was a problem hiding this comment.
For coherent memory, on most platforms CPU fences and DMA fences are the same.. The exception is aarch64, where a CPU acquire fence is dmb(ishld) while you need dmb(oshld) to synchronize with device (similarly, CPU release fence is dmb(ish) and DMA release fence is dmb(osh)).
There was a problem hiding this comment.
IIRC a standard fence also not sufficient on riscv?
Anyway I think you are saying that the text is correct? I don't intend for this to be an exhaustive guide for how to do DMA + MMIO in Rust, I think that should go somewhere else and I hope someone else can write that as I am definitely not qualified. :)
There was a problem hiding this comment.
RISC-V fence ordering can specify i/o in addition to r/w, but that is for ordering against MMIO, not for DMA. Either DMA is coherent which r/w is sufficient to order against, or it is incoherent and then platform-specific cache flush mechanisms would be needed (and that platform will not meet RISC-V Unix Requirements). In Linux the ordering between DMA and MMIO is taken care by MMIO primitives, where each MMIO primitives have a fence before and after it to order against CPU/DMA accesses.
The text is correct, but I am not sure if we want to show a bad example :) Perhaps use a different example, e.g. IPC between processes with shared memory?
There was a problem hiding this comment.
Well there is MMIO here so RISC-V would also need a stronger fence I think?
MMIO is the most obvious usecase for volatile so it felt like the clearest example. For IPC you are more likely to need CAS, not just load/store, but this first MVP only has volatile atomic load/store.
There was a problem hiding this comment.
Oh, my bad. I was somehow very convinced that this isn't about MMIO but just DMA. I guess I was still under the impression that the blessed way of doing MMIO is going to be plain read_volatile and write_volatile as discussed in all-hands 2025 (I believe the discussion back then was that we want to make plain volatile read/write have per-byte atomic semantics, and also generate correct instruction for MMIO if things are naturally aligned).
For MMIO, yes, most architectures would need special fences for them and SMP fences are insufficient.
My current view on MMIO is that there are too many quirks related to them that it's probably best to always handle them via inline assembly (given that any barrier/fences involving them also need to be inline asm)...
There was a problem hiding this comment.
This is for MMIO with a device that also does DMA, where one has to enforce an order between (non-volatile) accesses to the DMA region and (volatile) accesses to the MMIO region. The interaction involves preparing a buffer in the DMA region and then telling the device that it is ready by doing an MMIO write. At least, that's how I understood it; I am not a hardware expert. :)
(I believe the discussion back then was that we want to make plain volatile read/write have per-byte atomic semantics, and also generate correct instruction for MMIO if things are naturally aligned).
Per-byte atomics are stuck in limbo and the libs team was not happy about having "sometimes actually this is atomic" semantics for volatile, so the current plan is to have explicit volatile atomic operations, as tracked in #158947.
| #[inline] | ||
| #[unstable(feature = "atomic_volatile", issue = "158947")] | ||
| pub const fn from_ptr_raw(ptr: *mut bool) -> *const AtomicBool { | ||
| ptr as _ |
There was a problem hiding this comment.
nit, maybe .cast() is better here to avoid the ptr as cast which can do more? That also aligns the impl with from_ptr just above.
There was a problem hiding this comment.
This casts both the pointee type and the mutability so it'd have to be .cast().cast_mut(). At that point I feel there's no relevant difference any more to as, but I can use the methods anyway to make this more clear.
| #![feature(adt_const_params)] | ||
| #![feature(allow_internal_unsafe)] | ||
| #![feature(allow_internal_unstable)] | ||
| #![feature(arbitrary_self_types_pointers)] |
There was a problem hiding this comment.
Is it worth explicitly adding this to the tracking issue, or do we think it's fine to stabilize the methods added here without worrying about this feature?
There was a problem hiding this comment.
I'll add a note to the tracking issue.
151703b to
54e6177
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
54e6177 to
7f0cd46
Compare
|
@bors r=Mark-Simulacrum |
Rollup of 8 pull requests Successful merges: - #161301 (libcore: expose volatile atomic operations) - #161379 (Use better generic type parameter names for `Extend` and `FromIterator`) - #161926 (borrowck: Restore alias rigidity from HIR typeck) - #161956 (Remove unused `perform_locally_with_next_solver`) - #162026 (Emit delayed bug instead of ICEing when `TypeOutlives` goal fails) - #162034 (Make the LLVM version mismatch ICE a fatal error) - #162037 (LLVM wrapper cleanups) - #162043 (_ an unused parameter)
Rollup merge of #161301 - RalfJung:volatile-atomic-pub, r=Mark-Simulacrum libcore: expose volatile atomic operations Tracking issue: #158947 ACP: rust-lang/libs-team#801 @rust-lang/opsem @Darksonn @ojeda help with the docs would be appreciated :) (I figured kernel folks would have things to say about the DMA usecase that I allude to in my example.) Unlike in the ACP, I called the operations `load_volatile` instead of `volatile_load`, to be consistent with the existing `read_volatile` that also makes `volatile` a suffix rather than a prefix (and same for stores).
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (d014ca7): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary -1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: missing data |
Tracking issue: #158947
ACP: rust-lang/libs-team#801
@rust-lang/opsem @Darksonn @ojeda help with the docs would be appreciated :)
(I figured kernel folks would have things to say about the DMA usecase that I allude to in my example.)
Unlike in the ACP, I called the operations
load_volatileinstead ofvolatile_load, to be consistent with the existingread_volatilethat also makesvolatilea suffix rather than a prefix (and same for stores).