Message-ID: <87034744-5d9b-4ac6-bc36-a54aa32eafb2@proton.me>
Date: Tue, 30 Jul 2024 18:37:07 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Matthew Wilcox <willy@...radead.org>, linux-kernel@...r.kernel.org, linux-mm@...ck.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2] rust: mm: add abstractions for mm_struct and vm_area_struct

On 30.07.24 12:57, Alice Ryhl wrote:
> On Mon, Jul 29, 2024 at 6:13 PM Benno Lossin <benno.lossin@...ton.me> wrote:
>> On 27.07.24 11:03, Alice Ryhl wrote:
>>> +/// Equivalent to `ARef<MmWithUser>` but uses `mmput_async` in destructor.
>>> +///
>>> +/// The destructor of this type will never sleep.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// `inner` points to a valid `mm_struct` and the `ARefMmWithUserAsync` owns an `mmget` refcount.
>>> +pub struct ARefMmWithUserAsync {
>>> +    inner: NonNull<bindings::mm_struct>,
>>
>> I am confused, why doesn't `mm: MM` work here? I.e. also allow usage of
>> `ARef<MmWithUserAsync>`.
> 
> We could do that, but I don't know how much sense it makes. With Mm
> and MmWithUser there's a legitimate distinction between them that
> makes sense regardless of whether it's behind an ARef or &. But with
> the `mmput_async` case, the distinction only makes sense for ARef
> pointers, and &MmWithUser and &MmWithUserAsync would be 100%
> interchangeable.
> 
> That is to say, this is a property of the pointer, not the pointee. I
> don't think it makes sense semantically to have it be a wrapper around
> MmWithUser.

Hmm, I don't think that is a problem. We have `ARef` for the following
reasons (quoting myself from the ARef pattern thread):
(1) prevents having to implement multiple abstractions for a single C
    object: say there is a `struct foo` that is both used via reference
    counting and by-value on the stack. Without `ARef`, we would have to
    write two abstractions, one for each use-case. With `ARef`, we can
    have one `Foo` that can be wrapped with `ARef` to represent a
    reference-counted object.
(2) `ARef<T>` always represents a reference counted object, so it helps
    with understanding the code. If you read `Foo`, you cannot be sure
    if it is heap or stack allocated.
(3) generalizes common code of reference-counted objects (ie avoiding
    code duplication) and concentration of `unsafe` code.

If you don't use `ARef`, you
- have to implement `Deref`, `Drop`, `From<ARef<_>>` manually,
- have a rather ugly name,
- don't benefit from the three points above.

I don't really see a downside to just using `ARef` in this case.

>> Another approach might be to have the function on `MmWithUser`:
>>
>>     fn put_async(this: ARef<Self>)
>>
>> Or do you need it to be done on drop?
> 
> This needs to happen in drop so that use of the question mark
> operation doesn't suddenly result in sleep-in-atomic-ctx bugs.
> 
>>> +}
>>> +
>>> +// Make all `Mm` methods available on `MmWithUser`.
>>> +impl Deref for MmWithUser {
>>> +    type Target = Mm;
>>> +
>>> +    #[inline]
>>> +    fn deref(&self) -> &Mm {
>>> +        &self.mm
>>> +    }
>>
>> Does it really make sense to expose every function? E.g.
>> `mmget_not_zero` would always succeed, right?
> 
> I don't think it's a problem. Right now it exposes mmget_not_zero,
> is_same_mm, and as_raw. The only one where it doesn't make much sense
> is mmget_not_zero, but I don't think it hurts either.
> 
>>> +}
>>> +
>>> +// These methods are safe to call even if `mm_users` is zero.
>>
>> [...]
>>
>>> diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs
>>> new file mode 100644
>>> index 000000000000..2e97ef1eac58
>>> --- /dev/null
>>> +++ b/rust/kernel/mm/virt.rs
>>> @@ -0,0 +1,199 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +// Copyright (C) 2024 Google LLC.
>>> +
>>> +//! Virtual memory.
>>> +
>>> +use crate::{
>>> +    bindings,
>>> +    error::{to_result, Result},
>>> +    page::Page,
>>> +    types::Opaque,
>>> +};
>>> +
>>> +/// A wrapper for the kernel's `struct vm_area_struct`.
>>> +///
>>> +/// It represents an area of virtual memory.
>>> +#[repr(transparent)]
>>> +pub struct VmArea {
>>> +    vma: Opaque<bindings::vm_area_struct>,
>>> +}
>>> +
>>> +impl VmArea {
>>> +    /// Access a virtual memory area given a raw pointer.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must ensure that `vma` is valid for the duration of 'a, with shared access. The
>>> +    /// caller must ensure that using the pointer for immutable operations is okay.
>>
>> Nothing here states that the pointee is not allowed to be changed,
>> unless you mean that by "shared access" which would not match my
>> definition.
> 
> How about this?
> 
> Callers must ensure that:
> * `vma` is valid for the duration of 'a.
> * the caller holds the mmap read lock for 'a.
> 
> And `from_raw_vma_mut` would instead require the caller to hold the
> mmap write lock.

SGTM.

>>> +    #[inline]
>>> +    pub unsafe fn from_raw_vma<'a>(vma: *const bindings::vm_area_struct) -> &'a Self {
>>> +        // SAFETY: The caller ensures that the pointer is valid.
>>> +        unsafe { &*vma.cast() }
>>> +    }
>>> +
>>> +    /// Access a virtual memory area given a raw pointer.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must ensure that `vma` is valid for the duration of 'a, with exclusive access. The
>>> +    /// caller must ensure that using the pointer for immutable and mutable operations is okay.
>>> +    #[inline]
>>> +    pub unsafe fn from_raw_vma_mut<'a>(vma: *mut bindings::vm_area_struct) -> &'a mut Self {
>>> +        // SAFETY: The caller ensures that the pointer is valid.
>>> +        unsafe { &mut *vma.cast() }
>>> +    }
>>> +
>>> +    /// Returns a raw pointer to this area.
>>> +    #[inline]
>>> +    pub fn as_ptr(&self) -> *mut bindings::vm_area_struct {
>>> +        self.vma.get()
>>> +    }
>>> +
>>> +    /// Returns the flags associated with the virtual memory area.
>>> +    ///
>>> +    /// The possible flags are a combination of the constants in [`flags`].
>>> +    #[inline]
>>> +    pub fn flags(&self) -> usize {
>>> +        // SAFETY: The pointer is valid since self is a reference. The field is valid for reading
>>> +        // given a shared reference.
>>
>> Why is the field not changed from the C side? Is this part readonly?
> 
> Because we hold the mmap read lock. (or the write lock)

Oh, then it would be good to have it be an invariant.

---
Cheers,
Benno

Powered by blists - more mailing lists