| Message ID | 20231025195339.1431894-1-boqun.feng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] rust: types: Add read_once and write_once | expand |
> In theory, `read_volatile` and `write_volatile` in Rust can have UB in > case of the data races [1]. However, kernel uses volatiles to implement I would not write "In theory", but rather state that data races involving `read_volatile` is documented to still be UB. > READ_ONCE() and WRITE_ONCE(), and expects races on these marked accesses Missing "`"? > don't cause UB. And they are proven to have a lot of usages in kernel. > > To close this gap, `read_once` and `write_once` are introduced, they > have the same semantics as `READ_ONCE` and `WRITE_ONCE` especially > regarding data races under the assumption that `read_volatile` and I would separate implementation from specification. We specify `read_once` and `write_once` to have the same semantics as `READ_ONCE` and `WRITE_ONCE`. But we implement them using `read_volatile`/`write_volatile`, so we might still encounter UB, but it is still a sort of best effort. As soon as we have the actual thing in Rust, we will switch the implementation. > `write_volatile` have the same behavior as a volatile pointer in C from > a compiler point of view. > > Longer term solution is to work with Rust language side for a better way > to implement `read_once` and `write_once`. But so far, it should be good > enough. > > Suggested-by: Alice Ryhl <aliceryhl@google.com> > Link: https://doc.rust-lang.org/std/ptr/fn.read_volatile.html#safety [1] > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > > Notice I also make the primitives only work on T: Copy, since I don't > think Rust side and C side will use a !Copy type to communicate, but we > can always remove that constrait later. > > > rust/kernel/prelude.rs | 2 ++ > rust/kernel/types.rs | 43 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs > index ae21600970b3..351ad182bc63 100644 > --- a/rust/kernel/prelude.rs > +++ b/rust/kernel/prelude.rs > @@ -38,3 +38,5 @@ > pub use super::init::{InPlaceInit, Init, PinInit}; > > pub use super::current; > + > +pub use super::types::{read_once, write_once}; Do we really want people to use these so often that they should be in the prelude? Sure there will not really be any name conflicts, but I think an explicit import might make sense. > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs > index d849e1979ac7..b0872f751f97 100644 > --- a/rust/kernel/types.rs > +++ b/rust/kernel/types.rs I don't think this should go into `types.rs`. But I do not have a good name for the new module. > @@ -432,3 +432,46 @@ pub enum Either<L, R> { > /// Constructs an instance of [`Either`] containing a value of type `R`. > Right(R), > } > + > +/// (Concurrent) Primitives to interact with C side, which are considered as marked access: > +/// > +/// tools/memory-memory/Documentation/access-marking.txt > + Accidental empty line? Or is this meant as a comment for both functions? > +/// The counter part of C `READ_ONCE()`. > +/// > +/// The semantics is exactly the same as `READ_ONCE()`, especially when used for intentional data > +/// races. It would be great if these semantics are either linked or spelled out here. Ideally both. > +/// > +/// # Safety > +/// > +/// * `src` must be valid for reads. > +/// * `src` must be properly aligned. > +/// * `src` must point to a properly initialized value of value `T`. > +#[inline(always)] > +pub unsafe fn read_once<T: Copy>(src: *const T) -> T { Why only `T: Copy`? > + // SAFETY: the read is valid because of the function's safety requirement, plus the assumption > + // here is that 1) a volatile pointer dereference in C and 2) a `read_volatile` in Rust have the > + // same semantics, so this function should have the same behavior as `READ_ONCE()` regarding > + // data races. I would explicitly state that we might have UB here due to data races. But that we have not seen any invalid codegen and thus assume there to be no UB (you might also want to write this in the commit message). -- Cheers, Benno > + unsafe { src.read_volatile() } > +} > + > +/// The counter part of C `WRITE_ONCE()`. > +/// > +/// The semantics is exactly the same as `WRITE_ONCE()`, especially when used for intentional data > +/// races. > +/// > +/// # Safety > +/// > +/// * `dst` must be valid for writes. > +/// * `dst` must be properly aligned. > +#[inline(always)] > +pub unsafe fn write_once<T: Copy>(dst: *mut T, value: T) { > + // SAFETY: the write is valid because of the function's safety requirement, plus the assumption > + // here is that 1) a write to a volatile pointer dereference in C and 2) a `write_volatile` in > + // Rust have the same semantics, so this function should have the same behavior as > + // `WRITE_ONCE()` regarding data races. > + unsafe { > + core::ptr::write_volatile(dst, value); > + } > +} > -- > 2.41.0
On Wed, Oct 25, 2023 at 09:51:28PM +0000, Benno Lossin wrote: > > In theory, `read_volatile` and `write_volatile` in Rust can have UB in > > case of the data races [1]. However, kernel uses volatiles to implement > > I would not write "In theory", but rather state that data races involving > `read_volatile` is documented to still be UB. > Good point. > > READ_ONCE() and WRITE_ONCE(), and expects races on these marked accesses > > Missing "`"? > Yeah, but these are C macros, and here is the commit log, so I wasn't so sure I want to add "`", but I guess it's good for consistency. > > don't cause UB. And they are proven to have a lot of usages in kernel. > > > > To close this gap, `read_once` and `write_once` are introduced, they > > have the same semantics as `READ_ONCE` and `WRITE_ONCE` especially > > regarding data races under the assumption that `read_volatile` and > > I would separate implementation from specification. We specify > `read_once` and `write_once` to have the same semantics as `READ_ONCE` > and `WRITE_ONCE`. But we implement them using > `read_volatile`/`write_volatile`, so we might still encounter UB, but it > is still a sort of best effort. As soon as we have the actual thing in > Rust, we will switch the implementation. > Sounds good, I will use this in the next version. > > `write_volatile` have the same behavior as a volatile pointer in C from > > a compiler point of view. > > > > Longer term solution is to work with Rust language side for a better way > > to implement `read_once` and `write_once`. But so far, it should be good > > enough. > > > > Suggested-by: Alice Ryhl <aliceryhl@google.com> > > Link: https://doc.rust-lang.org/std/ptr/fn.read_volatile.html#safety [1] > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > --- > > > > Notice I also make the primitives only work on T: Copy, since I don't > > think Rust side and C side will use a !Copy type to communicate, but we > > can always remove that constrait later. > > > > > > rust/kernel/prelude.rs | 2 ++ > > rust/kernel/types.rs | 43 ++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 45 insertions(+) > > > > diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs > > index ae21600970b3..351ad182bc63 100644 > > --- a/rust/kernel/prelude.rs > > +++ b/rust/kernel/prelude.rs > > @@ -38,3 +38,5 @@ > > pub use super::init::{InPlaceInit, Init, PinInit}; > > > > pub use super::current; > > + > > +pub use super::types::{read_once, write_once}; > > Do we really want people to use these so often that they should be in > the prelude? > The reason I prelude them is because that `READ_ONCE` and `WRITE_ONCE` have total ~7000 users in kernel, but now think about it, maybe it's better not. > Sure there will not really be any name conflicts, but I think an > explicit import might make sense. > > > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs > > index d849e1979ac7..b0872f751f97 100644 > > --- a/rust/kernel/types.rs > > +++ b/rust/kernel/types.rs > > I don't think this should go into `types.rs`. But I do not have a good > name for the new module. > kernel::sync? > > @@ -432,3 +432,46 @@ pub enum Either<L, R> { > > /// Constructs an instance of [`Either`] containing a value of type `R`. > > Right(R), > > } > > + > > +/// (Concurrent) Primitives to interact with C side, which are considered as marked access: > > +/// > > +/// tools/memory-memory/Documentation/access-marking.txt > > + > > Accidental empty line? Or is this meant as a comment for both > functions? > Right, it's the documentation for both functions. > > +/// The counter part of C `READ_ONCE()`. > > +/// > > +/// The semantics is exactly the same as `READ_ONCE()`, especially when used for intentional data > > +/// races. > > It would be great if these semantics are either linked or spelled out > here. Ideally both. > Actually I haven't found any document about `READ_ONCE()` races with writes is not UB: we do have document saying `READ_ONCE()` will disable KCSAN checks, but no document says (explicitly) that it's not a UB. > > +/// > > +/// # Safety > > +/// > > +/// * `src` must be valid for reads. > > +/// * `src` must be properly aligned. > > +/// * `src` must point to a properly initialized value of value `T`. > > +#[inline(always)] > > +pub unsafe fn read_once<T: Copy>(src: *const T) -> T { > > Why only `T: Copy`? > I actually explained this above, after "---" of the commit log, but maybe it's worth its own documentation? The reason that it only works with `T: Copy`, is that these primitives should be mostly used for C/Rust communication, and using a `T: !Copy` is probably wrong (or at least complicated) for communication, since users need to handle which one should be used after `read_once()`. This is in the same spirit as `read_volatile` documentation: ``` Like read, read_volatile creates a bitwise copy of T, regardless of whether T is Copy. If T is not Copy, using both the returned value and the value at *src can violate memory safety. However, storing non-Copy types in volatile memory is almost certainly incorrect. ``` I want to start with restrict usage. > > + // SAFETY: the read is valid because of the function's safety requirement, plus the assumption > > + // here is that 1) a volatile pointer dereference in C and 2) a `read_volatile` in Rust have the > > + // same semantics, so this function should have the same behavior as `READ_ONCE()` regarding > > + // data races. > > I would explicitly state that we might have UB here due to data races. > But that we have not seen any invalid codegen and thus assume there to I'd rather not claim this (no invalid codegen), not because it's not true, but because it's not under our control. We have written doc in Rust says: ``` ... so the precise semantics of what “volatile” means here is subject to change over time. That being said, the semantics will almost always end up pretty similar to C11’s definition of volatile. ``` , so we have some confidence to say `read_volatile` equals to a volatile read, and `write_volatile` equals to a volatile write. Therefore, we can assume they have the same behaviors as `READ_ONCE()` and `WRITE_ONCE()`, but that's it. Going futher to talk about codegen means we have more guarantee from Rust compiler implementation. In other words, we are not saying racing `read_volatile`s don't have UBs, we are saying racing `read_volatile`s behave the same as a volatile read on UBs. Regards, Boqun > be no UB (you might also want to write this in the commit message). > > -- > Cheers, > Benno > > > + unsafe { src.read_volatile() } > > +} > > + > > +/// The counter part of C `WRITE_ONCE()`. > > +/// > > +/// The semantics is exactly the same as `WRITE_ONCE()`, especially when used for intentional data > > +/// races. > > +/// > > +/// # Safety > > +/// > > +/// * `dst` must be valid for writes. > > +/// * `dst` must be properly aligned. > > +#[inline(always)] > > +pub unsafe fn write_once<T: Copy>(dst: *mut T, value: T) { > > + // SAFETY: the write is valid because of the function's safety requirement, plus the assumption > > + // here is that 1) a write to a volatile pointer dereference in C and 2) a `write_volatile` in > > + // Rust have the same semantics, so this function should have the same behavior as > > + // `WRITE_ONCE()` regarding data races. > > + unsafe { > > + core::ptr::write_volatile(dst, value); > > + } > > +} > > -- > > 2.41.0 >
On Wed, Oct 25, 2023 at 04:02:45PM -0700, Boqun Feng wrote: [...] > > > +/// The counter part of C `READ_ONCE()`. > > > +/// > > > +/// The semantics is exactly the same as `READ_ONCE()`, especially when used for intentional data > > > +/// races. > > > > It would be great if these semantics are either linked or spelled out > > here. Ideally both. > > > > Actually I haven't found any document about `READ_ONCE()` races with > writes is not UB: we do have document saying `READ_ONCE()` will disable > KCSAN checks, but no document says (explicitly) that it's not a UB. > Apparently I wasn't carefully reading the doc, in tools/memory-model/Documentation/explanation.txt, there is: In technical terms, the compiler is allowed to assume that when the program executes, there will not be any data races. A "data race" occurs when there are two memory accesses such that: 1. they access the same location, 2. at least one of them is a store, 3. at least one of them is plain, 4. they occur on different CPUs (or in different threads on the same CPU), and 5. they execute concurrently. the #3 limits that in LKMM, data races cannot happen if both accesses are marked (i.e. not plain). Thank Paul for bringing this to me, and I will update this accordingly in the next version. Regards, Boqun > > > +/// > > > +/// # Safety > > > +/// > > > +/// * `src` must be valid for reads. > > > +/// * `src` must be properly aligned. > > > +/// * `src` must point to a properly initialized value of value `T`. > > > +#[inline(always)] > > > +pub unsafe fn read_once<T: Copy>(src: *const T) -> T { > > > > Why only `T: Copy`? > > [...]
On 26.10.23 01:02, Boqun Feng wrote: > On Wed, Oct 25, 2023 at 09:51:28PM +0000, Benno Lossin wrote: >>> In theory, `read_volatile` and `write_volatile` in Rust can have UB in >>> case of the data races [1]. However, kernel uses volatiles to implement >> >> I would not write "In theory", but rather state that data races involving >> `read_volatile` is documented to still be UB. >> > > Good point. > >>> READ_ONCE() and WRITE_ONCE(), and expects races on these marked accesses >> >> Missing "`"? >> > > Yeah, but these are C macros, and here is the commit log, so I wasn't so > sure I want to add "`", but I guess it's good for consistency. I was just wondering if it was intentional, since you did it below. >>> don't cause UB. And they are proven to have a lot of usages in kernel. >>> >>> To close this gap, `read_once` and `write_once` are introduced, they >>> have the same semantics as `READ_ONCE` and `WRITE_ONCE` especially >>> regarding data races under the assumption that `read_volatile` and >> >> I would separate implementation from specification. We specify >> `read_once` and `write_once` to have the same semantics as `READ_ONCE` >> and `WRITE_ONCE`. But we implement them using >> `read_volatile`/`write_volatile`, so we might still encounter UB, but it >> is still a sort of best effort. As soon as we have the actual thing in >> Rust, we will switch the implementation. >> > > Sounds good, I will use this in the next version. > >>> `write_volatile` have the same behavior as a volatile pointer in C from >>> a compiler point of view. >>> >>> Longer term solution is to work with Rust language side for a better way >>> to implement `read_once` and `write_once`. But so far, it should be good >>> enough. >>> >>> Suggested-by: Alice Ryhl <aliceryhl@google.com> >>> Link: https://doc.rust-lang.org/std/ptr/fn.read_volatile.html#safety [1] >>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com> >>> --- >>> >>> Notice I also make the primitives only work on T: Copy, since I don't >>> think Rust side and C side will use a !Copy type to communicate, but we >>> can always remove that constrait later. >>> >>> >>> rust/kernel/prelude.rs | 2 ++ >>> rust/kernel/types.rs | 43 ++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 45 insertions(+) >>> >>> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs >>> index ae21600970b3..351ad182bc63 100644 >>> --- a/rust/kernel/prelude.rs >>> +++ b/rust/kernel/prelude.rs >>> @@ -38,3 +38,5 @@ >>> pub use super::init::{InPlaceInit, Init, PinInit}; >>> >>> pub use super::current; >>> + >>> +pub use super::types::{read_once, write_once}; >> >> Do we really want people to use these so often that they should be in >> the prelude? >> > > The reason I prelude them is because that `READ_ONCE` and `WRITE_ONCE` > have total ~7000 users in kernel, but now think about it, maybe it's > better not. I think we should start out with it not in the prelude. Drivers should not need to call this often (I hope that only abstractions actually need this). >> Sure there will not really be any name conflicts, but I think an >> explicit import might make sense. >> >>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs >>> index d849e1979ac7..b0872f751f97 100644 >>> --- a/rust/kernel/types.rs >>> +++ b/rust/kernel/types.rs >> >> I don't think this should go into `types.rs`. But I do not have a good >> name for the new module. >> > > kernel::sync? I like that. >>> @@ -432,3 +432,46 @@ pub enum Either<L, R> { >>> /// Constructs an instance of [`Either`] containing a value of type `R`. >>> Right(R), >>> } >>> + >>> +/// (Concurrent) Primitives to interact with C side, which are considered as marked access: >>> +/// >>> +/// tools/memory-memory/Documentation/access-marking.txt >>> + >> >> Accidental empty line? Or is this meant as a comment for both >> functions? >> > > Right, it's the documentation for both functions. That will not work, it will just be rendered only on `read_once`. Maybe just copy it to both and also cross link the two functions. So `read_once` mentions the counterpart `write_once`. >>> +/// The counter part of C `READ_ONCE()`. >>> +/// >>> +/// The semantics is exactly the same as `READ_ONCE()`, especially when used for intentional data >>> +/// races. >> >> It would be great if these semantics are either linked or spelled out >> here. Ideally both. >> > > Actually I haven't found any document about `READ_ONCE()` races with > writes is not UB: we do have document saying `READ_ONCE()` will disable > KCSAN checks, but no document says (explicitly) that it's not a UB. > >>> +/// >>> +/// # Safety >>> +/// >>> +/// * `src` must be valid for reads. >>> +/// * `src` must be properly aligned. >>> +/// * `src` must point to a properly initialized value of value `T`. >>> +#[inline(always)] >>> +pub unsafe fn read_once<T: Copy>(src: *const T) -> T { >> >> Why only `T: Copy`? >> > > I actually explained this above, after "---" of the commit log, but Oh I missed that, sorry. > maybe it's worth its own documentation? The reason that it only works Yeah, lets document it. Otherwise I agree with your reasoning. > with `T: Copy`, is that these primitives should be mostly used for > C/Rust communication, and using a `T: !Copy` is probably wrong (or at > least complicated) for communication, since users need to handle which > one should be used after `read_once()`. This is in the same spirit as > `read_volatile` documentation: > > ``` > Like read, read_volatile creates a bitwise copy of T, regardless of > whether T is Copy. If T is not Copy, using both the returned value and > the value at *src can violate memory safety. However, storing non-Copy > types in volatile memory is almost certainly incorrect. > ``` > > I want to start with restrict usage. > >>> + // SAFETY: the read is valid because of the function's safety requirement, plus the assumption >>> + // here is that 1) a volatile pointer dereference in C and 2) a `read_volatile` in Rust have the >>> + // same semantics, so this function should have the same behavior as `READ_ONCE()` regarding >>> + // data races. >> >> I would explicitly state that we might have UB here due to data races. >> But that we have not seen any invalid codegen and thus assume there to > > I'd rather not claim this (no invalid codegen), not because it's not > true, but because it's not under our control. We have written doc in But it is under our control, we pin the compiler version and can always just check if the codegen is correct. If someone finds that it is not, we also want to be informed, so I think we should write that we rely on it here. > Rust says: > > ``` > ... so the precise semantics of what “volatile” means here is subject > to change over time. That being said, the semantics will almost always > end up pretty similar to C11’s definition of volatile. > ``` But this is not a guarantee, that they behave exactly the same as C11 _now_.
On Wed, Oct 25, 2023 at 12:53:39PM -0700, Boqun Feng wrote: > In theory, `read_volatile` and `write_volatile` in Rust can have UB in > case of the data races [1]. However, kernel uses volatiles to implement > READ_ONCE() and WRITE_ONCE(), and expects races on these marked accesses > don't cause UB. And they are proven to have a lot of usages in kernel. > > To close this gap, `read_once` and `write_once` are introduced, they > have the same semantics as `READ_ONCE` and `WRITE_ONCE` especially > regarding data races under the assumption that `read_volatile` and > `write_volatile` have the same behavior as a volatile pointer in C from > a compiler point of view. > > Longer term solution is to work with Rust language side for a better way > to implement `read_once` and `write_once`. But so far, it should be good > enough. So the whole READ_ONCE()/WRITE_ONCE() thing does two things we care about (AFAIR): - single-copy-atomicy; this can also be achieved using the C11 __atomic_load_n(.memorder=__ATOMIC_RELAXED) / __atomic_store_n(.memorder=__ATOMIC_RELAXED) thingies. - the ONCE thing; that is inhibits re-materialization, and here I'm not sure C11 atomics help, they might since re-reading an atomic is definitely dodgy -- after all it could've changed. Now, traditionally we've relied on the whole volatile thing simply because there was no C11, or our oldest compiler didn't do C11. But these days we actually *could*. Now, obviously C11 has issues vs LKMM, but perhaps the load/store semantics are near enough to be useful. (IIRC this also came up in the *very* long x86/percpu thread) So is there any distinction between the volatile load/store and the C11 atomic load/store that we care about and could not Rust use the atomic load/store to avoid their UB ?
On Thu, 26 Oct 2023 10:13:45 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Oct 25, 2023 at 12:53:39PM -0700, Boqun Feng wrote: > > In theory, `read_volatile` and `write_volatile` in Rust can have UB in > > case of the data races [1]. However, kernel uses volatiles to implement > > READ_ONCE() and WRITE_ONCE(), and expects races on these marked accesses > > don't cause UB. And they are proven to have a lot of usages in kernel. > > > > To close this gap, `read_once` and `write_once` are introduced, they > > have the same semantics as `READ_ONCE` and `WRITE_ONCE` especially > > regarding data races under the assumption that `read_volatile` and > > `write_volatile` have the same behavior as a volatile pointer in C from > > a compiler point of view. > > > > Longer term solution is to work with Rust language side for a better way > > to implement `read_once` and `write_once`. But so far, it should be good > > enough. > > So the whole READ_ONCE()/WRITE_ONCE() thing does two things we care > about (AFAIR): > > - single-copy-atomicy; this can also be achieved using the C11 > __atomic_load_n(.memorder=__ATOMIC_RELAXED) / > __atomic_store_n(.memorder=__ATOMIC_RELAXED) thingies. > > - the ONCE thing; that is inhibits re-materialization, and here I'm not > sure C11 atomics help, they might since re-reading an atomic is > definitely dodgy -- after all it could've changed. > > Now, traditionally we've relied on the whole volatile thing simply > because there was no C11, or our oldest compiler didn't do C11. But > these days we actually *could*. > > Now, obviously C11 has issues vs LKMM, but perhaps the load/store > semantics are near enough to be useful. (IIRC this also came up in the > *very* long x86/percpu thread) > > So is there any distinction between the volatile load/store and the C11 > atomic load/store that we care about and could not Rust use the atomic > load/store to avoid their UB ? There's two reasons that we are using volatile read/write as opposed to relaxed atomic: * Rust lacks volatile atomics at the moment. Non-volatile atomics are not sufficient because the compiler is allowed (although they currently don't) optimise atomics. If you have two adjacent relaxed loads, they could be merged into one. * Atomics only works for integer types determined by the platform. On some 32-bit platforms you wouldn't be able to use 64-bit atomics at all, and on x86 you get less optimal sequence since volatile load is permitted to tear while atomic load needs to use LOCK CMPXCHG8B. * Atomics doesn't work for complex structs. Although I am not quite sure of the value of supporting it.
On Thu, Oct 26, 2023 at 11:36:10AM +0100, Gary Guo wrote: > There's two reasons that we are using volatile read/write as opposed to > relaxed atomic: > * Rust lacks volatile atomics at the moment. Non-volatile atomics are > not sufficient because the compiler is allowed (although they > currently don't) optimise atomics. If you have two adjacent relaxed > loads, they could be merged into one. Ah yes, that would be problematic, eg, if lifted out of a loop things could go sideways fast. > * Atomics only works for integer types determined by the platform. On > some 32-bit platforms you wouldn't be able to use 64-bit atomics at > all, and on x86 you get less optimal sequence since volatile load is > permitted to tear while atomic load needs to use LOCK CMPXCHG8B. We only grudgingly allowed u64 READ_ONCE() on 32bit platforms because the fallout was too numerous to fix. Some of them are probably bugs. Also, I think cmpxchg8b without lock prefix would be sufficient, but I've got too much of a head-ache to be sure. Worse is that we still support targets without cmpxchg8b. It might be interesting to make the Rust side more strict in this regard and see where/when we run into trouble. > * Atomics doesn't work for complex structs. Although I am not quite sure > of the value of supporting it. So on the C side we mandate the size is no larger than machine word, with the exception of the u64 on 32bit thing. We don't mandate strict integer types because things like pte_t are wrapper types.
On Wed, 25 Oct 2023 at 21:54, Boqun Feng <boqun.feng@gmail.com> wrote: > > In theory, `read_volatile` and `write_volatile` in Rust can have UB in > case of the data races [1]. However, kernel uses volatiles to implement > READ_ONCE() and WRITE_ONCE(), and expects races on these marked accesses > don't cause UB. And they are proven to have a lot of usages in kernel. > > To close this gap, `read_once` and `write_once` are introduced, they > have the same semantics as `READ_ONCE` and `WRITE_ONCE` especially > regarding data races under the assumption that `read_volatile` and > `write_volatile` have the same behavior as a volatile pointer in C from > a compiler point of view. > > Longer term solution is to work with Rust language side for a better way > to implement `read_once` and `write_once`. But so far, it should be good > enough. One thing you may also want to address is the ability to switch between READ_ONCE implementations depending on config. For one, arm64 with LTO will promote READ_ONCE() to acquires.
On Thu, Oct 26, 2023 at 01:16:25PM +0200, Peter Zijlstra wrote: > On Thu, Oct 26, 2023 at 11:36:10AM +0100, Gary Guo wrote: > > > There's two reasons that we are using volatile read/write as opposed to > > relaxed atomic: > > * Rust lacks volatile atomics at the moment. Non-volatile atomics are > > not sufficient because the compiler is allowed (although they > > currently don't) optimise atomics. If you have two adjacent relaxed > > loads, they could be merged into one. > > Ah yes, that would be problematic, eg, if lifted out of a loop things > could go sideways fast. > Maybe we can workaround this limitation by using compiler barriers, i.e. compiler_fence(SeqCst); load(Relaxed); compiler_fence(Acquire); this is slightly stronger than a volatile atomic. > > * Atomics only works for integer types determined by the platform. On > > some 32-bit platforms you wouldn't be able to use 64-bit atomics at > > all, and on x86 you get less optimal sequence since volatile load is > > permitted to tear while atomic load needs to use LOCK CMPXCHG8B. > > We only grudgingly allowed u64 READ_ONCE() on 32bit platforms because > the fallout was too numerous to fix. Some of them are probably bugs. > > Also, I think cmpxchg8b without lock prefix would be sufficient, but > I've got too much of a head-ache to be sure. Worse is that we still > support targets without cmpxchg8b. > > It might be interesting to make the Rust side more strict in this regard > and see where/when we run into trouble. > Sounds good to me. If the compiler barriers make sense for now, then we can do: pub unsafe fn read_once_usize(ptr: *const usize) -> usize { core::sync::atomic::compiler_fence(SeqCst); let r = unsafe { *ptr.cast::<AtomicUsize>() }.load(Relaxed); core::sync::atomic::compiler_fence(Acquire); r } and if the other side (i.e. write) is also atomic (e.g. WRITE_ONCE()), we don't have data race. However, there are still cases where data races are ignored in C code, for example inode::i_state: reads out of locks race with writes inside locks, since writes are done by plain accesses. Nothing can be done to fix that from Rust side only, and fixing the C side is a separate topic. Thoughts? Regards, Boqun > > * Atomics doesn't work for complex structs. Although I am not quite sure > > of the value of supporting it. > > So on the C side we mandate the size is no larger than machine word, > with the exception of the u64 on 32bit thing. We don't mandate strict > integer types because things like pte_t are wrapper types. > >
On Thu, Oct 26, 2023 at 07:21:53AM -0700, Boqun Feng wrote: > On Thu, Oct 26, 2023 at 01:16:25PM +0200, Peter Zijlstra wrote: > > On Thu, Oct 26, 2023 at 11:36:10AM +0100, Gary Guo wrote: > > > > > There's two reasons that we are using volatile read/write as opposed to > > > relaxed atomic: > > > * Rust lacks volatile atomics at the moment. Non-volatile atomics are > > > not sufficient because the compiler is allowed (although they > > > currently don't) optimise atomics. If you have two adjacent relaxed > > > loads, they could be merged into one. > > > > Ah yes, that would be problematic, eg, if lifted out of a loop things > > could go sideways fast. > > > > Maybe we can workaround this limitation by using compiler barriers, i.e. > > compiler_fence(SeqCst); > load(Relaxed); > compiler_fence(Acquire); > > this is slightly stronger than a volatile atomic. > > > > * Atomics only works for integer types determined by the platform. On > > > some 32-bit platforms you wouldn't be able to use 64-bit atomics at > > > all, and on x86 you get less optimal sequence since volatile load is > > > permitted to tear while atomic load needs to use LOCK CMPXCHG8B. > > > > We only grudgingly allowed u64 READ_ONCE() on 32bit platforms because > > the fallout was too numerous to fix. Some of them are probably bugs. > > > > Also, I think cmpxchg8b without lock prefix would be sufficient, but > > I've got too much of a head-ache to be sure. Worse is that we still > > support targets without cmpxchg8b. > > > > It might be interesting to make the Rust side more strict in this regard > > and see where/when we run into trouble. > > > > Sounds good to me. If the compiler barriers make sense for now, then > we can do: > > pub unsafe fn read_once_usize(ptr: *const usize) -> usize { > core::sync::atomic::compiler_fence(SeqCst); > let r = unsafe { *ptr.cast::<AtomicUsize>() }.load(Relaxed); > core::sync::atomic::compiler_fence(Acquire); > r > } > I forgot to mention, this can also resolve the comments from Marco, i.e. switching implemention to Acquire if ARM64 & LTO. Regards, Boqun > and if the other side (i.e. write) is also atomic (e.g. WRITE_ONCE()), > we don't have data race. > > However, there are still cases where data races are ignored in C code, > for example inode::i_state: reads out of locks race with writes inside > locks, since writes are done by plain accesses. Nothing can be done to > fix that from Rust side only, and fixing the C side is a separate topic. > > Thoughts? > > Regards, > Boqun > > > > * Atomics doesn't work for complex structs. Although I am not quite sure > > > of the value of supporting it. > > > > So on the C side we mandate the size is no larger than machine word, > > with the exception of the u64 on 32bit thing. We don't mandate strict > > integer types because things like pte_t are wrapper types. > > > >
On Thu, Oct 26, 2023 at 01:16:25PM +0200, Peter Zijlstra wrote: > On Thu, Oct 26, 2023 at 11:36:10AM +0100, Gary Guo wrote: > > > There's two reasons that we are using volatile read/write as opposed to > > relaxed atomic: > > * Rust lacks volatile atomics at the moment. Non-volatile atomics are > > not sufficient because the compiler is allowed (although they > > currently don't) optimise atomics. If you have two adjacent relaxed > > loads, they could be merged into one. > > Ah yes, that would be problematic, eg, if lifted out of a loop things > could go sideways fast. > > > * Atomics only works for integer types determined by the platform. On > > some 32-bit platforms you wouldn't be able to use 64-bit atomics at > > all, and on x86 you get less optimal sequence since volatile load is > > permitted to tear while atomic load needs to use LOCK CMPXCHG8B. > > We only grudgingly allowed u64 READ_ONCE() on 32bit platforms because > the fallout was too numerous to fix. Some of them are probably bugs. > > Also, I think cmpxchg8b without lock prefix would be sufficient, but > I've got too much of a head-ache to be sure. Worse is that we still > support targets without cmpxchg8b. Plus cmpxchg8b can be quite a bit heavier weight than READ_ONCE(), in some cases to the point that you would instead use some other concurrency design. > It might be interesting to make the Rust side more strict in this regard > and see where/when we run into trouble. And maybe have some other name for READ_ONCE() that is permitted to tear. > > * Atomics doesn't work for complex structs. Although I am not quite sure > > of the value of supporting it. > > So on the C side we mandate the size is no larger than machine word, > with the exception of the u64 on 32bit thing. We don't mandate strict > integer types because things like pte_t are wrapper types. On C-language atomics, people who have talked about implementing atomics for objects too large for tear-free loads and stores have tended to want ot invent locks. :-( Thanx, Paul
Boqun Feng <boqun.feng@gmail.com> writes: >> On Wed, Oct 25, 2023 at 09:51:28PM +0000, Benno Lossin wrote: >>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs >>> index d849e1979ac7..b0872f751f97 100644 >>> --- a/rust/kernel/types.rs >>> +++ b/rust/kernel/types.rs >> >> I don't think this should go into `types.rs`. But I do not have a good >> name for the new module. > > kernel::sync? I think `kernel::sync` is a reasonable choice, but here's another possibility: Put them in the `bindings` crate. Why? Well, they are a utility that intends to replicate the C `READ_ONCE` and `WRITE_ONCE` macros. All of our other methods that do the same thing for C functions are functions in the bindings crate. Similarly, if we ever decide to reimplement a C function in Rust for performance/inlining reasons, then I also think that it is reasonable to put that Rust-reimplementation in the bindings crate. This approach also makes it easy to transparently handle cases where we only reimplement a function in Rust under some configurations. Alice
On 30.10.23 14:58, Alice Ryhl wrote: > Boqun Feng <boqun.feng@gmail.com> writes: >>> On Wed, Oct 25, 2023 at 09:51:28PM +0000, Benno Lossin wrote: >>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs >>>> index d849e1979ac7..b0872f751f97 100644 >>>> --- a/rust/kernel/types.rs >>>> +++ b/rust/kernel/types.rs >>> >>> I don't think this should go into `types.rs`. But I do not have a good >>> name for the new module. >> >> kernel::sync? > > I think `kernel::sync` is a reasonable choice, but here's another > possibility: Put them in the `bindings` crate. > > Why? Well, they are a utility that intends to replicate the C > `READ_ONCE` and `WRITE_ONCE` macros. All of our other methods that do > the same thing for C functions are functions in the bindings crate. I think we should keep things separate, that way `bindings` can be fully automatically generated. Stuff in the bindings crate is just an interface to the C world. Functions are not implemented there, but rather linked to. > Similarly, if we ever decide to reimplement a C function in Rust for > performance/inlining reasons, then I also think that it is reasonable to > put that Rust-reimplementation in the bindings crate. This approach also > makes it easy to transparently handle cases where we only reimplement a > function in Rust under some configurations. Is it really going to make things easier? We would have to make bindgen conditionally not create bindings for an item, so I think it would be easier to just always have the bindings function and handle the conditional reimplementation fully in the Rust code. Having extra code in the bindings crate will also make it more difficult to ensure that only abstractions use the bindings (we already have some exceptions, but why make it worse?).
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs index ae21600970b3..351ad182bc63 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -38,3 +38,5 @@ pub use super::init::{InPlaceInit, Init, PinInit}; pub use super::current; + +pub use super::types::{read_once, write_once}; diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index d849e1979ac7..b0872f751f97 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -432,3 +432,46 @@ pub enum Either<L, R> { /// Constructs an instance of [`Either`] containing a value of type `R`. Right(R), } + +/// (Concurrent) Primitives to interact with C side, which are considered as marked access: +/// +/// tools/memory-memory/Documentation/access-marking.txt + +/// The counter part of C `READ_ONCE()`. +/// +/// The semantics is exactly the same as `READ_ONCE()`, especially when used for intentional data +/// races. +/// +/// # Safety +/// +/// * `src` must be valid for reads. +/// * `src` must be properly aligned. +/// * `src` must point to a properly initialized value of value `T`. +#[inline(always)] +pub unsafe fn read_once<T: Copy>(src: *const T) -> T { + // SAFETY: the read is valid because of the function's safety requirement, plus the assumption + // here is that 1) a volatile pointer dereference in C and 2) a `read_volatile` in Rust have the + // same semantics, so this function should have the same behavior as `READ_ONCE()` regarding + // data races. + unsafe { src.read_volatile() } +} + +/// The counter part of C `WRITE_ONCE()`. +/// +/// The semantics is exactly the same as `WRITE_ONCE()`, especially when used for intentional data +/// races. +/// +/// # Safety +/// +/// * `dst` must be valid for writes. +/// * `dst` must be properly aligned. +#[inline(always)] +pub unsafe fn write_once<T: Copy>(dst: *mut T, value: T) { + // SAFETY: the write is valid because of the function's safety requirement, plus the assumption + // here is that 1) a write to a volatile pointer dereference in C and 2) a `write_volatile` in + // Rust have the same semantics, so this function should have the same behavior as + // `WRITE_ONCE()` regarding data races. + unsafe { + core::ptr::write_volatile(dst, value); + } +}
In theory, `read_volatile` and `write_volatile` in Rust can have UB in case of the data races [1]. However, kernel uses volatiles to implement READ_ONCE() and WRITE_ONCE(), and expects races on these marked accesses don't cause UB. And they are proven to have a lot of usages in kernel. To close this gap, `read_once` and `write_once` are introduced, they have the same semantics as `READ_ONCE` and `WRITE_ONCE` especially regarding data races under the assumption that `read_volatile` and `write_volatile` have the same behavior as a volatile pointer in C from a compiler point of view. Longer term solution is to work with Rust language side for a better way to implement `read_once` and `write_once`. But so far, it should be good enough. Suggested-by: Alice Ryhl <aliceryhl@google.com> Link: https://doc.rust-lang.org/std/ptr/fn.read_volatile.html#safety [1] Signed-off-by: Boqun Feng <boqun.feng@gmail.com> --- Notice I also make the primitives only work on T: Copy, since I don't think Rust side and C side will use a !Copy type to communicate, but we can always remove that constrait later. rust/kernel/prelude.rs | 2 ++ rust/kernel/types.rs | 43 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)