[RFC PATCH] rust: block: Use 32-bit atomics

[RFC PATCH] rust: block: Use 32-bit atomics

[David Gow]

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(-)

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 {
-- 
2.46.0.469.g59c65b2a67-goog

Re: [RFC PATCH] rust: block: Use 32-bit atomics

[Alice Ryhl]

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

Re: [RFC PATCH] rust: block: Use 32-bit atomics

[Gary Guo]

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},