Re: [RFC 2/2] rust: sync: Add atomic support

[Benno Lossin <benno.lossin@proton.me>]

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