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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Alice Ryhl" <aliceryhl@google.com> writes:

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

Nice 👍

>
>> > +//!
>> > +//! 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.

Cool 👍

>
>> > +///
>> > +/// 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.

Well I guess there is value in using same names as C. The additional
docs you sent help a lot so I guess it is fine.

If we were writing from scratch I would have held hard on `AddressSpace`
or `MemoryMap` over `Mm`. `Mm` has got to be one of the least
descriptive names we can come up with.


Best regards,
Andreas Hindborg