Re: [PATCH] mm: rust: make CONFIG_MMU ifdefs more narrow

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

On Thu, May 08, 2025 at 08:35:40AM +0000, Alice Ryhl wrote:
> Currently the entire kernel::mm module is ifdef'd out when CONFIG_MMU=n.
> However, there are some downstream users of the module in
> rust/kernel/task.rs and rust/kernel/miscdevice.rs. Thus, update the cfgs
> so that only MmWithUserAsync is removed with CONFIG_MMU=n.
> 
> The code is moved into a new file, since the #[cfg()] annotation
> otherwise has to be duplicated several times.
> 
> Closes: https://lore.kernel.org/all/202505071753.kldNHYVQ-lkp@intel.com/
> Closes: https://lore.kernel.org/all/202505072116.eSYC8igT-lkp@intel.com/
> Fixes: 3ca414d725d6 ("mm: rust: add abstraction for struct mm_struct")
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

One nit below, but overall it looks good to me, with or without it, feel
free to add:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

> ---
>  rust/kernel/mm.rs             | 56 +++--------------------------
>  rust/kernel/mm/mmput_async.rs | 68 +++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+), 52 deletions(-)
>  create mode 100644 rust/kernel/mm/mmput_async.rs
> 
> diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> index 615907a0f3b4..43f525c0d16c 100644
> --- a/rust/kernel/mm.rs
> +++ b/rust/kernel/mm.rs
> @@ -10,7 +10,6 @@
>  //! control what happens when userspace reads or writes to that region of memory.
>  //!
>  //! C header: [`include/linux/mm.h`](srctree/include/linux/mm.h)
> -#![cfg(CONFIG_MMU)]
>  
>  use crate::{
>      bindings,
> @@ -21,6 +20,10 @@
>  pub mod virt;
>  use virt::VmaRef;
>  
> +#[cfg(CONFIG_MMU)]
> +pub use mmput_async::MmWithUserAsync;
> +mod mmput_async;
> +
>  /// A wrapper for the kernel's `struct mm_struct`.
>  ///
>  /// This represents the address space of a userspace process, so each process has one `Mm`
> @@ -111,50 +114,6 @@ fn deref(&self) -> &Mm {
>      }
>  }
>  
> -/// A wrapper for the kernel's `struct mm_struct`.
> -///
> -/// This type is identical to `MmWithUser` except that it uses `mmput_async` when dropping a
> -/// refcount. This means that the destructor of `ARef<MmWithUserAsync>` is safe to call in atomic
> -/// context.
> -///
> -/// # Invariants
> -///
> -/// Values of this type are always refcounted using `mmget`. The value of `mm_users` is non-zero.
> -#[repr(transparent)]
> -pub struct MmWithUserAsync {
> -    mm: MmWithUser,
> -}
> -
> -// SAFETY: It is safe to call `mmput_async` on another thread than where `mmget` was called.
> -unsafe impl Send for MmWithUserAsync {}
> -// SAFETY: All methods on `MmWithUserAsync` can be called in parallel from several threads.
> -unsafe impl Sync for MmWithUserAsync {}
> -
> -// SAFETY: By the type invariants, this type is always refcounted.
> -unsafe impl AlwaysRefCounted for MmWithUserAsync {
> -    #[inline]
> -    fn inc_ref(&self) {
> -        // SAFETY: The pointer is valid since self is a reference.
> -        unsafe { bindings::mmget(self.as_raw()) };
> -    }
> -
> -    #[inline]
> -    unsafe fn dec_ref(obj: NonNull<Self>) {
> -        // SAFETY: The caller is giving up their refcount.
> -        unsafe { bindings::mmput_async(obj.cast().as_ptr()) };
> -    }
> -}
> -
> -// Make all `MmWithUser` methods available on `MmWithUserAsync`.
> -impl Deref for MmWithUserAsync {
> -    type Target = MmWithUser;
> -
> -    #[inline]
> -    fn deref(&self) -> &MmWithUser {
> -        &self.mm
> -    }
> -}
> -
>  // These methods are safe to call even if `mm_users` is zero.
>  impl Mm {
>      /// Returns a raw pointer to the inner `mm_struct`.
> @@ -206,13 +165,6 @@ pub unsafe fn from_raw<'a>(ptr: *const bindings::mm_struct) -> &'a MmWithUser {
>          unsafe { &*ptr.cast() }
>      }
>  
> -    /// Use `mmput_async` when dropping this refcount.
> -    #[inline]
> -    pub fn into_mmput_async(me: ARef<MmWithUser>) -> ARef<MmWithUserAsync> {
> -        // SAFETY: The layouts and invariants are compatible.
> -        unsafe { ARef::from_raw(ARef::into_raw(me).cast()) }
> -    }
> -
>      /// Attempt to access a vma using the vma read lock.
>      ///
>      /// This is an optimistic trylock operation, so it may fail if there is contention. In that
> diff --git a/rust/kernel/mm/mmput_async.rs b/rust/kernel/mm/mmput_async.rs
> new file mode 100644
> index 000000000000..9289e05f7a67
> --- /dev/null
> +++ b/rust/kernel/mm/mmput_async.rs
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Version of `MmWithUser` using `mmput_async`.
> +//!
> +//! This is a separate file from `mm.rs` due to the dependency on `CONFIG_MMU=y`.
> +#![cfg(CONFIG_MMU)]
> +
> +use crate::{
> +    bindings,
> +    mm::MmWithUser,

It's better to use 

	use super::MmWithUser;

here, I think.

Regards,
Boqun


> +    types::{ARef, AlwaysRefCounted},
> +};
> +use core::{ops::Deref, ptr::NonNull};
> +
> +/// A wrapper for the kernel's `struct mm_struct`.
> +///
> +/// This type is identical to `MmWithUser` except that it uses `mmput_async` when dropping a
> +/// refcount. This means that the destructor of `ARef<MmWithUserAsync>` is safe to call in atomic
> +/// context.
> +///
> +/// # Invariants
> +///
> +/// Values of this type are always refcounted using `mmget`. The value of `mm_users` is non-zero.
> +#[repr(transparent)]
> +pub struct MmWithUserAsync {
> +    mm: MmWithUser,
> +}
> +
> +// SAFETY: It is safe to call `mmput_async` on another thread than where `mmget` was called.
> +unsafe impl Send for MmWithUserAsync {}
> +// SAFETY: All methods on `MmWithUserAsync` can be called in parallel from several threads.
> +unsafe impl Sync for MmWithUserAsync {}
> +
> +// SAFETY: By the type invariants, this type is always refcounted.
> +unsafe impl AlwaysRefCounted for MmWithUserAsync {
> +    #[inline]
> +    fn inc_ref(&self) {
> +        // SAFETY: The pointer is valid since self is a reference.
> +        unsafe { bindings::mmget(self.as_raw()) };
> +    }
> +
> +    #[inline]
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The caller is giving up their refcount.
> +        unsafe { bindings::mmput_async(obj.cast().as_ptr()) };
> +    }
> +}
> +
> +// Make all `MmWithUser` methods available on `MmWithUserAsync`.
> +impl Deref for MmWithUserAsync {
> +    type Target = MmWithUser;
> +
> +    #[inline]
> +    fn deref(&self) -> &MmWithUser {
> +        &self.mm
> +    }
> +}
> +
> +impl MmWithUser {
> +    /// Use `mmput_async` when dropping this refcount.
> +    #[inline]
> +    pub fn into_mmput_async(me: ARef<MmWithUser>) -> ARef<MmWithUserAsync> {
> +        // SAFETY: The layouts and invariants are compatible.
> +        unsafe { ARef::from_raw(ARef::into_raw(me).cast()) }
> +    }
> +}
> 
> base-commit: 03be7043bbae0380e3e314cde042e4bb5c63b48c
> -- 
> 2.49.0.987.g0cc8ee98dc-goog
>