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

Tracking Issue for atomic_from_mut #76314

Open
4 of 6 tasks
KodrAus opened this issue Sep 4, 2020 · 40 comments
Open
4 of 6 tasks

Tracking Issue for atomic_from_mut #76314

KodrAus opened this issue Sep 4, 2020 · 40 comments
Labels
A-atomic Area: Atomics, barriers, and sync primitives A-concurrency Area: Concurrency C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@KodrAus
Copy link
Contributor

KodrAus commented Sep 4, 2020

Feature gate: #![feature(atomic_from_mut)]

Public API

impl AtomicU32 {
    pub fn from_mut(v: &mut u32) -> &mut Self;
    pub fn from_mut_slice(v: &mut [u32]) -> &mut [Self];
    pub fn get_mut_slice(this: &mut [Self]) -> &mut [u32];
}

// And same for AtomicBool, AtomicU8, AtomicU16, AtomicU64, AtomicI8, AtomicI16, AtomicI32, AtomicI64, and AtomicPtr

Each one is gated on #[cfg(target_has_atomic_equal_alignment = "..")] and is only available on platforms where Atomic<size> has the same alignment as u<size>.

Steps / History

Unresolved Questions

@KodrAus KodrAus added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. labels Sep 4, 2020
@RalfJung
Copy link
Member

RalfJung commented Sep 20, 2020

For some types, these functions introduce a significant risk of accidentally writing non-portable code: when I am on x86_64, I might not realize that by calling AtomicU64::from_mut I am excluding platforms like x64_32 where u64 has alignment less than 8.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 20, 2020

But the same holds for the other atomics operations, right? Using 64-bit atomic cas operations will also exclude a lot of platforms.

The alternative is that people write this manually using transmute. That code will be equally non-portable, except it'll compile just fine and silently cause UB.

That said, it might be good to extend the note in the documentation to mention platforms like x86 and x64_32 specifically.

@RalfJung
Copy link
Member

Yeah, I am not saying this is a blocker. This could be something that good documentation can solve.

@KodrAus KodrAus added the A-concurrency Area: Concurrency label Sep 22, 2020
@RustyYato
Copy link
Contributor

Can Atomic*::from_mut's signature be changed to fn(&mut _) -> &mut Self instead of fn(&mut _) -> &Self. This way from_mut and get_mut are inverses. (I view Cell::from_mut's signature as a wart, that can't be fixed, but Atomic*::from_mut can be changed).

@m-ou-se
Copy link
Member

m-ou-se commented Jan 18, 2021

@RustyYato Is there a use case for that? There's not much more you can do with a &mut Atomic other than get_mut().

@RustyYato
Copy link
Contributor

@m-ou-se No specific use-case, it just seems odd to me that from_mut and get_mut are not inverses.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 18, 2021

Okay. I've added your suggestion to the unresolved questions to resolve before stabilizing this.

@the8472
Copy link
Member

the8472 commented Apr 4, 2021

Should these return &mut Self instead, such that it's the exact inverse of get_mut?

I think the following two make sense.

  • from_mut(&mut _) -> &mut Self, inverse of get_mut
  • unsafe from_mut_ptr(*mut _) -> &Self, inverse of as_mut_ptr and does not allow get_mut

@biabeb
Copy link

biabeb commented Jul 27, 2021

unsafe from_mut_ptr(*mut _) -> &Self, inverse of as_mut_ptr and does not allow get_mut

What would be the safety requirements on from_mut_ptr? Would overlapping atomics of different sizes be alllowed?

Overlapping atomics of different sizes is probably an obscure use case, but it would be really helpful in some situations. However, Rust's memory model (i.e., C++ memory model) does not appear to account for that possibility.

@RalfJung
Copy link
Member

RalfJung commented Jul 27, 2021

Such overlapping atomics are called "mixed accesses" in the weak memory literature, and indeed most memory models do not support them.

@eira-fransham
Copy link

eira-fransham commented Aug 24, 2021

What is the reasoning behind it being invalid to cast from &usize to &AtomicUsize or the equivalents for other types? I would assume the UB invariants are the same as for casting &T to &UnsafeCell<T> (or, equivalently, &T to &Cell<T>) but I can't find any info on why that is invalid either.

@CryZe
Copy link
Contributor

CryZe commented Aug 24, 2021

What is the reasoning behind it being invalid to cast from &usize to &AtomicUsize or the equivalents for other types?

&usize is immutable, the compiler is allowed to assume the value behind the reference will never change. &AtomicUsize is "mutable" through its atomic operations. So any sort of cast like that is unsound.

@eira-fransham
Copy link

Ah right, that makes sense. It's not the same as a &T -> &mut T cast but it causes the same issues with code motion.

@WaffleLapkin
Copy link
Member

These methods were in std for quite a while now, can we move towards stabilization?

As a first step, I can make a PR that changes the output from &Self to &mut Self. It doesn't seem there are any concerns with that and this makes the API nicer (by making from_mut and get_mut inverses of each other).

@RalfJung
Copy link
Member

One concern we might have is that AFAIK C++ does not have a comparable API, so this could move us into a situation where we cannot just copy the C++ memory model for atomics.

@WaffleLapkin
Copy link
Member

I'm not very familiar with C++, but after a quick scan of its atomic docs it seems like C++ doesn't have get_mut either, so we are already in the said situation, right?

@RalfJung
Copy link
Member

C++ has atomic_ref.

But I think I just realized that atomic_ref is like from_mut, so the problematic one is the one we already stabilized...

@comex
Copy link
Contributor

comex commented Oct 30, 2021

However, atomic_ref is not guaranteed to have a trivial destructor or copy constructor, and all accesses to the object while the atomic_ref is alive are required to be through atomic_ref itself. Therefore, it almost seems like atomic_ref could be implemented "pathologically" as:

  • the constructor takes a (bit) copy of the object and stores it in the atomic_ref itself;
  • all methods on atomic_ref access the copy rather than the original pointer;
  • the destructor puts the object back.

In reality, this isn't quite allowed, because you can separately make multiple atomic_refs to the same pointer, and then access all of them concurrently. All accesses are required to be through atomic_ref, but not through the same atomic_ref.

However, this can be fixed by adding a layer of indirection. Instead of atomic_ref storing a copy of the object directly, it would be essentially shared_ptr<atomic_ref_backing>, where atomic_ref_backing stores a copy of the object, and there's a global hash map from pointers to atomic_ref_backings so that creating multiple atomic_refs from the same pointer gives you references to the same atomic_ref_backing.

I'm not sure whether the fact this is possible is an intentional choice to support architectures that can't manipulate arbitrary memory atomically, or just a coincidence. Either way, it means that AtomicFoo::from_mut is a bit more demanding of the platform than atomic_ref, since it returns a reference, which is guaranteed to be trivially copyable and droppable, ruling out such an implementation.

That said, AtomicFoo in general is already more demanding than C++ atomic, as it is required to be lock-free while C++ atomic is not.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 9, 2022
…_ref, r=m-ou-se

Make `Atomic*::from_mut` return `&mut Atomic*`

```rust
impl Atomic* {
    pub fn from_mut(v: &mut bool) -> &mut Self;
    //                               ^^^^---- previously was just a &
}
```

This PR makes `from_mut` atomic methods tracked in rust-lang#76314 return unique references to atomic types, instead of shared ones. This makes `from_mut` and `get_mut` inverses of each other, allowing to undo either of them by the other.

r? `@RalfJung`
(as Ralf was [concerned](rust-lang#76314 (comment)) about this)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 1, 2022
Add Atomic*::from_mut_slice

Tracking issue rust-lang#76314 for `from_mut` has a question about the possibility of `from_mut_slice`, and I found a real case for it. A user in the forum had a parallelism problem that could be solved by open-indexing updates to a vector of atomics, but they didn't want to affect the other code using that vector. Using `from_mut_slice`, they could borrow that data as atomics just long enough for their parallel loop.

ref: https://users.rust-lang.org/t/sharing-vector-with-rayon-par-iter-correctly/72022
@bonzini
Copy link

bonzini commented May 19, 2022

Is it possible to write a signature that consumes the &mut? The point of from_mut is that an AtomicUxxx can be accessed safely from multiple threads, without causing UB. It supports interior mutability, so it should be possible to get a shared reference to an atomic without using unsafe. But that's only possible if one cannot write

    let mut x: usize = 42;
    let xref = &mut x;
    let xaref = AtomicUsize::from_mut(xref); // Shared ref
    // Send xaref around...
    *xref = 53;    // Undefined behavior!!!

If that's not possible then yeah it has to be unsafe.

@WaffleLapkin
Copy link
Member

@bonzini borrow checker will stop you from doing any undefined behavior here :)

AtomicUsize::from_mut returns a reference, so the lifetime is preserved, xref will be frozen while xaref is alive.
You can use playground to see that this code is rejected:

let mut x: usize = 42;
let xref = &mut x;
let xaref = AtomicUsize::from_mut(xref);

// Send xaref around...

*xref = 53;

dbg!(xaref);
error[[E0506]](https://doc.rust-lang.org/nightly/error-index.html#E0506): cannot assign to `*xref` because it is borrowed
  --> src/main.rs:10:5
   |
6  |     let xaref = AtomicUsize::from_mut(xref);
   |                                       ---- borrow of `*xref` occurs here
...
10 |     *xref = 53;
   |     ^^^^^^^^^^ assignment to borrowed `*xref` occurs here
11 |     
12 |     dbg!(xaref);
   |          ----- borrow later used here

@RalfJung
Copy link
Member

Is it possible to write a signature that consumes the &mut?

Your example uses an &, not an &mut, so I am a bit confused by the question in general.

@bonzini
Copy link

bonzini commented May 20, 2022

Your example uses an &, not an &mut

Don't write Rust on a phone is the logical conclusion of that (I edited now). Anyway, then I think from_mut should return a shared reference.

@RalfJung
Copy link
Member

But why? There is no benefit from that, only less (well-defined) code that can compile.

@bonzini
Copy link

bonzini commented May 20, 2022

There is no benefit from that

The benefit is being able to go safely from single-thread mutable &mut u32 to multi-thread mutable &AtomicU32. For example you can get a &mut u32, let multiple threads do their things on it and then give it back to the caller. (Of course the benefit is more visible with large slices than with a single value).

Here is a playground link that demonstrate this possibility.

@RalfJung
Copy link
Member

RalfJung commented May 20, 2022

The benefit is being able to go safely from single-thread mutable &mut u32 to multi-thread mutable &AtomicU32.

You can do that with the current signature. &mut AtomicU32 coerces to &AtomicU32.

So, what you want can already be done. No need to make from_mut return a shared reference.

Here is your playground example adjusted accordingly. (Due to the closure, you have to tell the type checker to apply the coercion. In most situations it would be applied implicitly.)

@bonzini
Copy link

bonzini commented May 20, 2022

Ah, of course (let result_atomic: &AtomicU32 = AtomicU32::from_mut(result);). Sorry for missing the obvious.

@workingjubilee workingjubilee added the A-atomic Area: Atomics, barriers, and sync primitives label Aug 17, 2022
@fogti
Copy link
Contributor

fogti commented Dec 20, 2022

could the alignment requirement be integrated into the type signatures for the cases where that is problematic? (perhaps via some Option checked-construction function)

@WaffleLapkin
Copy link
Member

I don't think that this function makes sense as a fallible one. I don't see a usecase where you'd be able to recover somehow/having a function work only sometimes is valuable...

@fogti
Copy link
Contributor

fogti commented Dec 21, 2022

this is why I'd like to see it integrated into the type signatures, to make sure it can't even be called with under-aligned objects.

@WaffleLapkin
Copy link
Member

The function is unavailable on targets where align_of::<T>() != align_of::<AtomicT>(), so it is impossible to call with under-aligned objects.

@fogti
Copy link
Contributor

fogti commented Dec 22, 2022

It would be nice to have an alternative on platforms where the alignment doesn't match per default, that is, a way to force the alignment and also specify that in the type signature would be nice (on platforms where the alignment wouldn't be widened, that would be a no-op)

@WaffleLapkin
Copy link
Member

We could have some sort of struct AlignedTo<T, const ALIGN: usize>(...);, but that's not quite simple (you need to set the alignment somehow and also probably use it as AlignedTo<u32, align_of::<AtomicU32>()>), I don't think the we are ready for this (yet). So, I'd say it would be nice to have eventually, but it's a task for the future....

@uablrek
Copy link

uablrek commented May 16, 2024

Have you considered shared memory (Linux)?

I am writing a traffic generator (in rust) where the statistics is stored in shared memory. An independent presentation program (in any language) can map the shared memory and present statistics in real time.

The traffic generator is multi-threaded (tokio), so counters must be updated atomically. Example:

// The "stats" struct is mapped to shared memory
let bytes_sent = AtomicU64::from_mut(&mut stats.bytes_sent);
bytes_sent.fetch_add(bytes, Ordering::Relaxed);

This works fine, but I must use rustup default nightly (no big deal since I am experimenting).

Is there a way to accomplish this in a way that can be used in "stable"?
(perhaps some trick with Box<> and from_ptr?)

Or shall I wait until this feature makes it to "stable".

@RalfJung
Copy link
Member

RalfJung commented May 16, 2024 via email

@uablrek
Copy link

uablrek commented May 16, 2024

Um, are you saying that the use of Linux shared memory is forbidden in rust? Or at least writing to it?

@the8472
Copy link
Member

the8472 commented May 16, 2024

You shouldn't use &mut for those and use raw pointers and AtomicXXX::from_ptr instead.

@RalfJung
Copy link
Member

RalfJung commented May 16, 2024 via email

@uablrek
Copy link

uablrek commented May 16, 2024

Changed to:

let bytes_sent = unsafe {
    AtomicU64::from_ptr(&mut stats.bytes_sent as *mut u64)
};

and switched back to rustup default stable. Works fine. Thanks a lot @the8472

@RalfJung
Copy link
Member

RalfJung commented May 16, 2024

&mut stats.bytes_sent

You're still creating a mutable reference here, so this code still breaks the aliasing rules (and hence has Undefined Behavior).
You can use addr_of_mut!(stats.bytes_sent) to avoid that.

Also, if stats is a mutable reference, then that should be changed as well.
But anyway that's all off-topic in this issue; feel free to ask further questions on Zulip. :)

@uablrek
Copy link

uablrek commented May 16, 2024

But anyway that's all off-topic in this issue; feel free to ask further questions on Zulip. :)

Will do. You are right, I thought it wasn't possible to use an existing u64 for atomic operations without "nightly". Hence my comment in the issue. Sorry.

Updated again to;

let bytes_sent = unsafe {
    AtomicU64::from_ptr(addr_of_mut!(stats.bytes_sent))
};

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-atomic Area: Atomics, barriers, and sync primitives A-concurrency Area: Concurrency C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests