| Message ID | 20240905061214.3954271-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] rust: block: Use 32-bit atomics | expand |
On Thu, Sep 5, 2024 at 8:12 AM David Gow <davidgow@google.com> wrote: > > Not all architectures have core::sync::atomic::AtomicU64 available. In > particular, 32-bit x86 doesn't support it. AtomicU32 is available > everywhere, so use that instead. > > Hopefully we can add AtomicU64 to Rust-for-Linux more broadly, so this > won't be an issue, but it's not supported in core from upstream Rust: > https://doc.rust-lang.org/std/sync/atomic/#portability > > This can be tested on 32-bit x86 UML via: > ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --kconfig_add CONFIG_RUST=y --kconfig_add CONFIG_64BIT=n --kconfig_add CONFIG_FORTIFY_SOURCE=n > > Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module") > Signed-off-by: David Gow <davidgow@google.com> > --- > > Hi all, > > I encountered this build error with Rust/UML since the kernel::block::mq > stuff landed. I'm not 100% sure just swapping AtomicU64 with AtomicU32 > is correct -- please correct me if not -- but this does at least get the > Rust/UML/x86-32 builds here compiling and running again. > > (And gives me more encouragement to go to the Rust atomics talk at > Plumbers.) I would probably go with AtomicUsize over AtomicU32 in this case. Alice
On Thu, 5 Sep 2024 14:12:14 +0800 David Gow <davidgow@google.com> wrote: > Not all architectures have core::sync::atomic::AtomicU64 available. In > particular, 32-bit x86 doesn't support it. AtomicU32 is available > everywhere, so use that instead. Switching to 32-bit directly makes it vulnerable to counter overflow issue. If 32-bit atomics are to be used for this, saturation logic must be implemented to deal with it. Ideally we should just use `refcount_t` instead of custom atomic ops. Although it appears that the rust block driver needs a cmpxchg which refcount_t doesn't provide. > > Hopefully we can add AtomicU64 to Rust-for-Linux more broadly, so this > won't be an issue, but it's not supported in core from upstream Rust: > https://doc.rust-lang.org/std/sync/atomic/#portability Kernel has a 64-bit atomic implementation which is backed by spinlocks if the architecture doesn't support it. Although, I think for the purpose in rust block, it's unlikely to be necessary. Best, Gary > > This can be tested on 32-bit x86 UML via: > ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --kconfig_add CONFIG_RUST=y --kconfig_add CONFIG_64BIT=n --kconfig_add CONFIG_FORTIFY_SOURCE=n > > Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module") > Signed-off-by: David Gow <davidgow@google.com> > --- > > Hi all, > > I encountered this build error with Rust/UML since the kernel::block::mq > stuff landed. I'm not 100% sure just swapping AtomicU64 with AtomicU32 > is correct -- please correct me if not -- but this does at least get the > Rust/UML/x86-32 builds here compiling and running again. > > (And gives me more encouragement to go to the Rust atomics talk at > Plumbers.) > > Cheers, > -- David > > --- > rust/kernel/block/mq/operations.rs | 4 ++-- > rust/kernel/block/mq/request.rs | 12 ++++++------ > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs > index 9ba7fdfeb4b2..c31c36af6bc4 100644 > --- a/rust/kernel/block/mq/operations.rs > +++ b/rust/kernel/block/mq/operations.rs > @@ -11,7 +11,7 @@ > error::{from_result, Result}, > types::ARef, > }; > -use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering}; > +use core::{marker::PhantomData, sync::atomic::AtomicU32, sync::atomic::Ordering}; > > /// Implement this trait to interface blk-mq as block devices. > /// > @@ -186,7 +186,7 @@ impl<T: Operations> OperationsVTable<T> { > > // 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(AtomicU32::new(0)) }; > > Ok(0) > }) > diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs > index a0e22827f3f4..418256dcd45b 100644 > --- a/rust/kernel/block/mq/request.rs > +++ b/rust/kernel/block/mq/request.rs > @@ -13,7 +13,7 @@ > use core::{ > marker::PhantomData, > ptr::{addr_of_mut, NonNull}, > - sync::atomic::{AtomicU64, Ordering}, > + sync::atomic::{AtomicU32, Ordering},
diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs index 9ba7fdfeb4b2..c31c36af6bc4 100644 --- a/rust/kernel/block/mq/operations.rs +++ b/rust/kernel/block/mq/operations.rs @@ -11,7 +11,7 @@ error::{from_result, Result}, types::ARef, }; -use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering}; +use core::{marker::PhantomData, sync::atomic::AtomicU32, sync::atomic::Ordering}; /// Implement this trait to interface blk-mq as block devices. /// @@ -186,7 +186,7 @@ impl<T: Operations> OperationsVTable<T> { // 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(AtomicU32::new(0)) }; Ok(0) }) diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs index a0e22827f3f4..418256dcd45b 100644 --- a/rust/kernel/block/mq/request.rs +++ b/rust/kernel/block/mq/request.rs @@ -13,7 +13,7 @@ use core::{ marker::PhantomData, ptr::{addr_of_mut, NonNull}, - sync::atomic::{AtomicU64, Ordering}, + sync::atomic::{AtomicU32, Ordering}, }; /// A wrapper around a blk-mq `struct request`. This represents an IO request. @@ -159,13 +159,13 @@ pub(crate) struct RequestDataWrapper { /// - 0: The request is owned by C block layer. /// - 1: The request is owned by Rust abstractions but there are no ARef references to it. /// - 2+: There are `ARef` references to the request. - refcount: AtomicU64, + refcount: AtomicU32, } impl RequestDataWrapper { /// Return a reference to the refcount of the request that is embedding /// `self`. - pub(crate) fn refcount(&self) -> &AtomicU64 { + pub(crate) fn refcount(&self) -> &AtomicU32 { &self.refcount } @@ -175,7 +175,7 @@ pub(crate) fn refcount(&self) -> &AtomicU64 { /// # Safety /// /// - `this` must point to a live allocation of at least the size of `Self`. - pub(crate) unsafe fn refcount_ptr(this: *mut Self) -> *mut AtomicU64 { + pub(crate) unsafe fn refcount_ptr(this: *mut Self) -> *mut AtomicU32 { // SAFETY: Because of the safety requirements of this function, the // field projection is safe. unsafe { addr_of_mut!((*this).refcount) } @@ -193,7 +193,7 @@ unsafe impl<T: Operations> Sync for Request<T> {} /// Store the result of `op(target.load())` in target, returning new value of /// target. -fn atomic_relaxed_op_return(target: &AtomicU64, op: impl Fn(u64) -> u64) -> u64 { +fn atomic_relaxed_op_return(target: &AtomicU32, op: impl Fn(u32) -> u32) -> u32 { let old = target.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |x| Some(op(x))); // SAFETY: Because the operation passed to `fetch_update` above always @@ -205,7 +205,7 @@ fn atomic_relaxed_op_return(target: &AtomicU64, op: impl Fn(u64) -> u64) -> u64 /// Store the result of `op(target.load)` in `target` if `target.load() != /// pred`, returning true if the target was updated. -fn atomic_relaxed_op_unless(target: &AtomicU64, op: impl Fn(u64) -> u64, pred: u64) -> bool { +fn atomic_relaxed_op_unless(target: &AtomicU32, op: impl Fn(u32) -> u32, pred: u32) -> bool { target .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |x| { if x == pred {
Not all architectures have core::sync::atomic::AtomicU64 available. In particular, 32-bit x86 doesn't support it. AtomicU32 is available everywhere, so use that instead. Hopefully we can add AtomicU64 to Rust-for-Linux more broadly, so this won't be an issue, but it's not supported in core from upstream Rust: https://doc.rust-lang.org/std/sync/atomic/#portability This can be tested on 32-bit x86 UML via: ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --kconfig_add CONFIG_RUST=y --kconfig_add CONFIG_64BIT=n --kconfig_add CONFIG_FORTIFY_SOURCE=n Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module") Signed-off-by: David Gow <davidgow@google.com> --- Hi all, I encountered this build error with Rust/UML since the kernel::block::mq stuff landed. I'm not 100% sure just swapping AtomicU64 with AtomicU32 is correct -- please correct me if not -- but this does at least get the Rust/UML/x86-32 builds here compiling and running again. (And gives me more encouragement to go to the Rust atomics talk at Plumbers.) Cheers, -- David --- rust/kernel/block/mq/operations.rs | 4 ++-- rust/kernel/block/mq/request.rs | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-)