Re: [PATCH v11 1/8] mm: rust: add abstraction for struct mm_struct

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Mon, Jan 13, 2025 at 10:53:33AM +0100, Alice Ryhl wrote:
> On Mon, Dec 16, 2024 at 3:50 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >
> > "Alice Ryhl" <aliceryhl@google.com> writes:
> >
> > > These abstractions allow you to reference a `struct mm_struct` using
> > > both mmgrab and mmget refcounts. This is done using two Rust types:
> > >
> > > * Mm - represents an mm_struct where you don't know anything about the
> > >   value of mm_users.
> > > * MmWithUser - represents an mm_struct where you know at compile time
> > >   that mm_users is non-zero.
> > >
> > > This allows us to encode in the type system whether a method requires
> > > that mm_users is non-zero or not. For instance, you can always call
> > > `mmget_not_zero` but you can only call `mmap_read_lock` when mm_users is
> > > non-zero.
> > >
> > > It's possible to access current->mm without a refcount increment, but
> > > that is added in a later patch of this series.
> > >
> > > Acked-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> (for mm bits)
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> > >  rust/helpers/helpers.c |   1 +
> > >  rust/helpers/mm.c      |  39 +++++++++
> > >  rust/kernel/lib.rs     |   1 +
> > >  rust/kernel/mm.rs      | 219 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 260 insertions(+)
> > >
> > > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
> > > new file mode 100644
> > > index 000000000000..84cba581edaa
> > > --- /dev/null
> > > +++ b/rust/kernel/mm.rs
> > > @@ -0,0 +1,219 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +// Copyright (C) 2024 Google LLC.
> > > +
> > > +//! Memory management.
> >
> > Could you add a little more context here?
>
> How about this?
>
> //! Memory management.
> //!
> //! This module deals with managing the address space of userspace
> processes. Each process has an
> //! instance of [`Mm`], which keeps track of multiple VMAs (virtual
> memory areas). Each VMA
> //! corresponds to a region of memory that the userspace process can
> access, and the VMA lets you
> //! control what happens when userspace reads or writes to that region
> of memory.
> //!
> //! C header: [`include/linux/mm.h`](srctree/include/linux/mm.h)
>
> > > +//!
> > > +//! C header: [`include/linux/mm.h`](srctree/include/linux/mm.h)
> > > +
> > > +use crate::{
> > > +    bindings,
> > > +    types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
> > > +};
> > > +use core::{ops::Deref, ptr::NonNull};
> > > +
> > > +/// A wrapper for the kernel's `struct mm_struct`.
> >
> > Could you elaborate the data structure use case? When do I need it, what
> > does it do?
>
> How about this?
>
> /// A wrapper for the kernel's `struct mm_struct`.
> ///
> /// This represents the address space of a userspace process, so each
> process has one `Mm`
> /// instance. It may hold many VMAs internally.
> ///
> /// There is a counter called `mm_users` that counts the users of the
> address space; this includes
> /// the userspace process itself, but can also include kernel threads
> accessing the address space.
> /// Once `mm_users` reaches zero, this indicates that the address
> space can be destroyed. To access
> /// the address space, you must prevent `mm_users` from reaching zero
> while you are accessing it.
> /// The [`MmWithUser`] type represents an address space where this is
> guaranteed, and you can
> /// create one using [`mmget_not_zero`].
> ///
> /// The `ARef<Mm>` smart pointer holds an `mmgrab` refcount. Its
> destructor may sleep.
>
> > > +///
> > > +/// Since `mm_users` may be zero, the associated address space may not exist anymore. You can use
> > > +/// [`mmget_not_zero`] to be able to access the address space.
> > > +///
> > > +/// The `ARef<Mm>` smart pointer holds an `mmgrab` refcount. Its destructor may sleep.
> > > +///
> > > +/// # Invariants
> > > +///
> > > +/// Values of this type are always refcounted using `mmgrab`.
> > > +///
> > > +/// [`mmget_not_zero`]: Mm::mmget_not_zero
> > > +#[repr(transparent)]
> > > +pub struct Mm {
> >
> > Could we come up with a better name? `MemoryMap` or `MemoryMapping`?. You
> > use `MMapReadGuard` later.
>
> Those names seem really confusing to me. The mmap syscall creates a
> new VMA, but MemoryMap sounds like it's the thing that mmap creates.
>
> Lorenzo, what do you think? I'm inclined to just call it Mm since
> that's what C calls it.

I think Mm is better just for aligment with the C stuff, I mean the alternative
is MmStruct or something and... yuck.

And like, here I am TOTALLY onboard with Andreas here, because this naming
SUCKS. But it sucks on the C side too (we're experts at bad naming :). So for
consistency, let's suck everywhere...

Feel free to put a comment about this being a bad name if you like
though... (not obligatory :)

>
> Alice