Re: [PATCH v1 1/3] rust: revocable: Switch to kernel::sync atomic primitives

[Boqun Feng <boqun.feng@gmail.com>]

On Tue, Dec 30, 2025 at 06:37:16PM +0900, FUJITA Tomonori wrote:
> Convert uses of `AtomicBool` to `Atomic<bool>`.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/revocable.rs | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 0f4ae673256d..47cd2b45eada 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -7,12 +7,20 @@
>  
>  use pin_init::Wrapper;
>  
> -use crate::{bindings, prelude::*, sync::rcu, types::Opaque};
> +use crate::{
> +    bindings,
> +    prelude::*,
> +    sync::atomic::{
> +        ordering::Relaxed,
> +        Atomic, //
> +    },
> +    sync::rcu,
> +    types::Opaque, //
> +};
>  use core::{
>      marker::PhantomData,
>      ops::Deref,
> -    ptr::drop_in_place,
> -    sync::atomic::{AtomicBool, Ordering},
> +    ptr::drop_in_place, //
>  };
>  
>  /// An object that can become inaccessible at runtime.
> @@ -65,7 +73,7 @@
>  /// ```
>  #[pin_data(PinnedDrop)]
>  pub struct Revocable<T> {
> -    is_available: AtomicBool,
> +    is_available: Atomic<bool>,
>      #[pin]
>      data: Opaque<T>,

I actually think we should use Atomic<i32> instead of Atomic<bool> here,
because in most of the cases `T` is a aligned to at least 4-bytes (e.g.
Devres uses a Revocable<IoMem>) And in that case, using Atomic<bool>
doesn't save extra memory but makes the xchg() operation slower on
architectures that don't support byte-wise RmW instructions (e.g.
riscv).

My rule of thumb for using Atomic<bool> is: when you really care about
the memory usage, and you can guarantee using Atomic<bool> is saving
memory. For a general "flag" usage, Atomic<bool> is not strictly better
than Atomic<i32> in most cases imo.

Regards,
Boqun

>  }
> @@ -84,7 +92,7 @@ impl<T> Revocable<T> {
>      /// Creates a new revocable instance of the given data.
>      pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>          try_pin_init!(Self {
> -            is_available: AtomicBool::new(true),
> +            is_available: Atomic::new(true),
>              data <- Opaque::pin_init(data),
>          }? E)
>      }
> @@ -98,7 +106,7 @@ pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>      /// because another CPU may be waiting to complete the revocation of this object.
>      pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
>          let guard = rcu::read_lock();
> -        if self.is_available.load(Ordering::Relaxed) {
> +        if self.is_available.load(Relaxed) {
>              // Since `self.is_available` is true, data is initialised and has to remain valid
>              // because the RCU read side lock prevents it from being dropped.
>              Some(RevocableGuard::new(self.data.get(), guard))
> @@ -116,7 +124,7 @@ pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
>      /// allowed to sleep because another CPU may be waiting to complete the revocation of this
>      /// object.
>      pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a T> {
> -        if self.is_available.load(Ordering::Relaxed) {
> +        if self.is_available.load(Relaxed) {
>              // SAFETY: Since `self.is_available` is true, data is initialised and has to remain
>              // valid because the RCU read side lock prevents it from being dropped.
>              Some(unsafe { &*self.data.get() })
> @@ -157,7 +165,7 @@ pub unsafe fn access(&self) -> &T {
>      ///
>      /// Callers must ensure that there are no more concurrent users of the revocable object.
>      unsafe fn revoke_internal<const SYNC: bool>(&self) -> bool {
> -        let revoke = self.is_available.swap(false, Ordering::Relaxed);
> +        let revoke = self.is_available.xchg(false, Relaxed);
>  
>          if revoke {
>              if SYNC {
> -- 
> 2.43.0
>