Re: [RFC v2 00/13] LKMM *generic* atomics in Rust
On Sat, Nov 02, 2024 at 03:35:36PM +0800, David Gow wrote: > On Fri, 1 Nov 2024 at 14:04, Boqun Feng wrote: > > > > Hi, > > > > This is another RFC version of LKMM atomics in Rust, you can find the > > previous versions: > > > > v1: > > https://lore.kernel.org/rust-for-linux/[email protected]/ > > wip: > > https://lore.kernel.org/rust-for-linux/[email protected]/ > > > > I add a few more people Cced this time, so if you're curious about why > > we choose to implement LKMM atomics instead of using the Rust ones, you > > can find some explanation: > > > > * In my Kangrejos talk: https://lwn.net/Articles/993785/ > > * In Linus' email: > > https://lore.kernel.org/rust-for-linux/CAHk-=whkQk=zq5ximcau3xj4v69+jyop-y6sywhq-tvxssv...@mail.gmail.com/ > > > > This time, I try implementing a generic atomic type `Atomic`, since > > Benno and Gary suggested last time, and also Rust standard library is > > also going to that direction [1]. > > > > Honestly, a generic atomic type is still not quite necessary for myself, > > but here are a few reasons that it's useful: > > > > * It's useful for type alias, for example, if you have: > > > > type c_long = isize; > > > > Then `Atomic` and `Atomic` is the same type, > > this may make FFI code (mapping a C type to a Rust type or vice > > versa) more readable. > > > > * In kernel, we sometimes switch atomic to percpu for better > > scalabity, percpu is naturally a generic type, because it can > > have data that is larger than machine word size. Making atomic > > generic ease the potential switching/refactoring. > > > > * Generic atomics provide all the functionalities that non-generic > > atomics could have. > > > > That said, currently "generic" is limited to a few types: the type must > > be the same size as atomic_t or atomic64_t, other than basic types, only > > #[repr()] struct can be used in a `Atomic`. > > > > Also this is not a full feature set patchset, things like different > > arithmetic operations and bit operations are still missing, they can be > > either future work or for future versions. > > > > I included an RCU pointer implementation as one example of the usage, so > > a patch from Danilo is added, but I will drop it once his patch merged. > > > > This is based on today's rust-next, and I've run all kunit tests from > > the doc test on x86, arm64 and riscv. > > > > Feedbacks and comments are welcome! Thanks. > > > > Regards, > > Boqun > > > > [1]: https://github.com/rust-lang/rust/issues/130539 > > > > Thanks, Boqun. > Hi David, > I played around a bit with porting the blk-mq atomic code to this. As > neither an expert in Rust nor an expert in atomics, this is probably > both non-idiomatic and wrong, but unlike the `core` atomics, it > provides an Atomic:: on 32-bit systems, which gets UML's 32-bit > build working again. > > Diff below -- I'm not likely to have much time to work on this again > soon, so feel free to pick it up/fix it if it suits. > Thanks. These look good to me, however, I think I prefer Gary's patch for this: https://lore.kernel.org/lkml/[email protected]/ therefore, I won't take this into the next version. But thank you for taking a look! Regards, Boqun > Thanks, > -- David > > --- > diff --git a/rust/kernel/block/mq/operations.rs > b/rust/kernel/block/mq/operations.rs > index 9ba7fdfeb4b2..822d64230e11 100644 > --- a/rust/kernel/block/mq/operations.rs > +++ b/rust/kernel/block/mq/operations.rs > @@ -11,7 +11,8 @@ > error::{from_result, Result}, > types::ARef, > }; > -use core::{marker::PhantomData, sync::atomic::AtomicU64, > sync::atomic::Ordering}; > +use core::marker::PhantomData; > +use kernel::sync::atomic::{Atomic, Relaxed}; > > /// Implement this trait to interface blk-mq as block devices. > /// > @@ -77,7 +78,7 @@ impl OperationsVTable { > let request = unsafe { &*(*bd).rq.cast::>() }; > > // One refcount for the ARef, one for being in flight > -request.wrapper_ref().refcount().store(2, Ordering::Relaxed); > +request.wrapper_ref().refcount().store(2, Relaxed); > > // SAFETY: > // - We own a refcount that we took above. We pass that to `ARef`. > @@ -186,7 +187,7 @@ impl OperationsVTable { > > // SAFETY: The refcount field is allocated but not initialized, > so > // it is valid for writes. > -unsafe { > RequestDataWrapper::refcount_ptr(pdu.as_ptr()).write(AtomicU64::new(0)) > }; > +unsafe { > RequestDataWrapper::refcount_ptr(pdu.as_ptr()).write(Atomicnew(0)) > }; > > Ok(0) > }) > diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs > index 7943f43b9575..8d4060d65159 100644 > --- a/rust/kernel/block/mq/request.rs > +++ b/rust/kernel/block/mq/request.rs > @@ -13,8 +13,8 @@ > use core:
Re: [RFC v2 00/13] LKMM *generic* atomics in Rust
On Fri, 1 Nov 2024 at 14:04, Boqun Feng wrote: > > Hi, > > This is another RFC version of LKMM atomics in Rust, you can find the > previous versions: > > v1: > https://lore.kernel.org/rust-for-linux/[email protected]/ > wip: > https://lore.kernel.org/rust-for-linux/[email protected]/ > > I add a few more people Cced this time, so if you're curious about why > we choose to implement LKMM atomics instead of using the Rust ones, you > can find some explanation: > > * In my Kangrejos talk: https://lwn.net/Articles/993785/ > * In Linus' email: > https://lore.kernel.org/rust-for-linux/CAHk-=whkQk=zq5ximcau3xj4v69+jyop-y6sywhq-tvxssv...@mail.gmail.com/ > > This time, I try implementing a generic atomic type `Atomic`, since > Benno and Gary suggested last time, and also Rust standard library is > also going to that direction [1]. > > Honestly, a generic atomic type is still not quite necessary for myself, > but here are a few reasons that it's useful: > > * It's useful for type alias, for example, if you have: > > type c_long = isize; > > Then `Atomic` and `Atomic` is the same type, > this may make FFI code (mapping a C type to a Rust type or vice > versa) more readable. > > * In kernel, we sometimes switch atomic to percpu for better > scalabity, percpu is naturally a generic type, because it can > have data that is larger than machine word size. Making atomic > generic ease the potential switching/refactoring. > > * Generic atomics provide all the functionalities that non-generic > atomics could have. > > That said, currently "generic" is limited to a few types: the type must > be the same size as atomic_t or atomic64_t, other than basic types, only > #[repr()] struct can be used in a `Atomic`. > > Also this is not a full feature set patchset, things like different > arithmetic operations and bit operations are still missing, they can be > either future work or for future versions. > > I included an RCU pointer implementation as one example of the usage, so > a patch from Danilo is added, but I will drop it once his patch merged. > > This is based on today's rust-next, and I've run all kunit tests from > the doc test on x86, arm64 and riscv. > > Feedbacks and comments are welcome! Thanks. > > Regards, > Boqun > > [1]: https://github.com/rust-lang/rust/issues/130539 > Thanks, Boqun. I played around a bit with porting the blk-mq atomic code to this. As neither an expert in Rust nor an expert in atomics, this is probably both non-idiomatic and wrong, but unlike the `core` atomics, it provides an Atomic:: on 32-bit systems, which gets UML's 32-bit build working again. Diff below -- I'm not likely to have much time to work on this again soon, so feel free to pick it up/fix it if it suits. Thanks, -- David --- diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs index 9ba7fdfeb4b2..822d64230e11 100644 --- a/rust/kernel/block/mq/operations.rs +++ b/rust/kernel/block/mq/operations.rs @@ -11,7 +11,8 @@ error::{from_result, Result}, types::ARef, }; -use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering}; +use core::marker::PhantomData; +use kernel::sync::atomic::{Atomic, Relaxed}; /// Implement this trait to interface blk-mq as block devices. /// @@ -77,7 +78,7 @@ impl OperationsVTable { let request = unsafe { &*(*bd).rq.cast::>() }; // One refcount for the ARef, one for being in flight -request.wrapper_ref().refcount().store(2, Ordering::Relaxed); +request.wrapper_ref().refcount().store(2, Relaxed); // SAFETY: // - We own a refcount that we took above. We pass that to `ARef`. @@ -186,7 +187,7 @@ impl OperationsVTable { // SAFETY: The refcount field is allocated but not initialized, so // it is valid for writes. -unsafe { RequestDataWrapper::refcount_ptr(pdu.as_ptr()).write(AtomicU64::new(0)) }; +unsafe { RequestDataWrapper::refcount_ptr(pdu.as_ptr()).write(Atomicnew(0)) }; Ok(0) }) diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs index 7943f43b9575..8d4060d65159 100644 --- a/rust/kernel/block/mq/request.rs +++ b/rust/kernel/block/mq/request.rs @@ -13,8 +13,8 @@ use core::{ marker::PhantomData, ptr::{addr_of_mut, NonNull}, -sync::atomic::{AtomicU64, Ordering}, }; +use kernel::sync::atomic::{Atomic, Relaxed}; /// A wrapper around a blk-mq [`struct request`]. This represents an IO request. /// @@ -102,8 +102,7 @@ fn try_set_end(this: ARef) -> Result<*mut bindings::request, ARef> { if let Err(_old) = this.wrapper_ref().refcount().compare_exchange( 2, 0, -Ordering::Relaxed, -Ordering::Relaxed, +Relaxed ) { return Err(this); } @@ -168
Re: [RFC v2 00/13] LKMM *generic* atomics in Rust
On Fri, Nov 1, 2024 at 7:03 AM Boqun Feng wrote: > > This time, I try implementing a generic atomic type `Atomic`, since > Benno and Gary suggested last time, and also Rust standard library is > also going to that direction [1]. I would like to thank Boqun for trying out this approach, even when he wasn't (and maybe still isn't) convinced. Cheers, Miguel
