Date: Wed, 19 Jun 2024 09:30:15 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>
Cc: John Hubbard <jhubbard@...dia.com>, Miguel Ojeda <miguel.ojeda.sandonis@...il.com>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org, llvm@...ts.linux.dev, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, Alan Stern <stern@...land.harvard.edu>, Andrea Parri <parri.andrea@...il.com>, Will Deacon <will@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Nicholas Piggin <npiggin@...il.com>, David Howells <dhowells@...hat.com>, Jade Alglave <j.alglave@....ac.uk>, Luc Maranget <luc.maranget@...ia.fr>, "Paul E. McKenney" <paulmck@...nel.org>, Akira Yokosawa <akiyks@...il.com>, Daniel Lustig <dlustig@...dia.com>, Joel Fernandes <joel@...lfernandes.org>, Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>,
	kent.overstreet@...il.com, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, elver@...gle.com, Mark Rutland <mark.rutland@....com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>, Catalin Marinas <catalin.marinas@....com>, torvalds@...ux-foundation.org, linux-arm-kernel@...ts.infradead.org, linux-fsdevel@...r.kernel.org, Trevor Gross <tmgross@...ch.edu>, dakr@...hat.com
Subject: Re: [RFC 2/2] rust: sync: Add atomic support

On 17.06.24 07:36, Boqun Feng wrote:
> On Sun, Jun 16, 2024 at 08:06:05AM -0700, Boqun Feng wrote:
> [...]
>>>
>>> Note that crossbeam's AtomicCell is also generic, and crossbeam is used
>>> by tons of crates. As Miguel mentioned, I think it's very likely that in
>>> the future we want be able to do atomics on new types (e.g. for
>>> seqlocks perhaps). We probably don't need the non-lock-free fallback of
>>
>> Good, another design bit, thank you!
>>
>> What's our overall idea on sub-word types, like Atomic<u8> and
>> Atomic<u16>, do we plan to say no to them, or they could have a limited
>> APIs? IIUC, some operations on them are relatively sub-optimal on some
>> architectures, supporting the same set of API as i32 and i64 is probably
>> a bad idea.
>>
>> Another thing in my mind is making `Atomic<T>`
>>
>> 	pub struct Atomic<T: Send + ...> { ... }
>>
>> so that `Atomic<T>` will always be `Sync`, because quite frankly, an
>> atomic type that cannot `Sync` is pointless.

That is true, but adding semantically "unnecessary" bounds can be bad.
This is because they infect everything that wants to use `Atomic<T>`,
since they also need to add that bound.

> Also, how do we avoid this issue [1] in kernel?

I think that we can first go the way of my second approach (ie adding a
private trait as a bound on `Atomic<T>` to prevent generic usage). And
only allow primitives.
If we then see that people would like to put their own (u8, u16) tuple
structs into `Atomic<T>`, we have multiple options:

1. Field projection:
   Only primitives can be `load`ed and `store`ed, to access the values
   of the tuple, one would need to project to each field and read them.

2. Disallow padding:
   We add an `unsafe` trait that asserts there are no padding bytes in
   there (like `NoUinit` from below) and also add a macro that
   implements the trait safely.

3. Use `MaybeUninit` under the hood:
   I don't know if this would fix the issue entirely, since that is what
   crossbeam currently uses (but the issue remains open).

But I don't think that we should encourage large structs to be put into
`Atomic<T>`, since that would be bad for perf, right? So I think that
going the way of 1 would be great (if we had FP, otherwise 2 seems fine).

> `atomic_load()` in C is implemented as READ_ONCE() and it's, at most
> time, a volatile read, so the eventual code is:
> 
>     let a: (u8, u16) = (1, 2);
>     let b = unsafe { core::ptr::read_volatile::<i32>(&a as *const _ as *const i32) };
> 
> I know we probably ignore data race here and treat `read_volatile` as a
> dependency read per LKMM [2]. But this is an using of uninitialized
> data, so it's a bit different.

But would we implement it this way? Or would it go through a C function?
If we entirely do it in Rust, then yes this is bad.

---
Cheers,
Benno

> We can do what https://crates.io/crates/atomic does:
> 
> 	pub struct Atomic<T: NoUninit + ..> { ... }
> 
> , where `NoUinit` means no internal padding bytes, but it loses the
> ability to put a
> 
> 	#[repr(u32)]
> 	pub enum Foo { .. }
> 
> into `Atomic<T>`, right? Which is probably a case you want to support?
> 
> Regards,
> Boqun
> 
> [1]: https://github.com/crossbeam-rs/crossbeam/issues/748#issuecomment-1133926617
> [2]: tools/memory-model/Documentation/access-marking.txt

Powered by blists - more mailing lists