From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6EC7EC3DA4A for ; Mon, 29 Jul 2024 16:13:37 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0412E6B0088; Mon, 29 Jul 2024 12:13:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F32CF6B0089; Mon, 29 Jul 2024 12:13:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E21936B008A; Mon, 29 Jul 2024 12:13:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id C5AF16B0088 for ; Mon, 29 Jul 2024 12:13:36 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 4FC9E140470 for ; Mon, 29 Jul 2024 16:13:36 +0000 (UTC) X-FDA: 82393285632.17.9D68896 Received: from mail-4316.protonmail.ch (mail-4316.protonmail.ch [185.70.43.16]) by imf17.hostedemail.com (Postfix) with ESMTP id 63C704001B for ; Mon, 29 Jul 2024 16:13:34 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b="IW/kefeb"; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf17.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.16 as permitted sender) smtp.mailfrom=benno.lossin@proton.me ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722269571; a=rsa-sha256; cv=none; b=hX5SN9HTknLQz+piPCSHRoaBCpJ8M73rsazSe70B1WNyzn3wNQCS8BckJh3O1R/Y474B0a 2+VsyBSoYkduMtyN5rFMpoO1zvG6Ke42mupHSr/Ie8vaWuhl+LR+CPUUg7SFn+FPjpbvT0 Uf66XrzhPxe+3XwbnwOR+HyHtvwteM8= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b="IW/kefeb"; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf17.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.16 as permitted sender) smtp.mailfrom=benno.lossin@proton.me ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722269571; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=DmiCz9CBV2cOT6+9yNy9Fwokps+X5J3kz+JDsmEjePE=; b=ROQcgOkwkvlL2RC+QrIZsy492QzWC0teithMwLQ72fdOTZEcwfj9e7TlmUjuOglggFv+Bl s7Ozx8NEEzxCLSjoIqgBINFwiBaBiaBq6Mw6+jeRv6oaetIfYajggHxJe0LLHV9a5Y13wE quFPOhTw/uQz0zb+3F0XuRNkIPFblAQ= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1722269610; x=1722528810; bh=DmiCz9CBV2cOT6+9yNy9Fwokps+X5J3kz+JDsmEjePE=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=IW/kefebBzp4GDVMAwZ9/upV9CuyhkYG4TTIA/1iqlP7HKj4bU8ymWkHw/KBt8Aip A5GcVOBUXbsqc375EvdE3hAFJAmB7yk4c8KMbvGCZeM+qpHQk5AeKmjyScgqT5HA5D H4LySmnwd/shARqMTpsPWAM3hWwlkrwdOBAOJB+elAu1sq9AZQO1+Kpm6dAL5RtDhl i0pUA4fo38hzoZAsmjuZ/LQxnTFk5KHfrSoZgFRkGl97lmYWNHWIuY2yYdN+s0nwii LCysYb8flnQrvUwTE5RdjHLH9T2SEHXdOmGwixHtYsmQOS2CUcaXK3EyCpYhWJTDr6 ijJuSMv70wiKg== Date: Mon, 29 Jul 2024 16:13:08 +0000 To: Alice Ryhl , Miguel Ojeda , Andrew Morton From: Benno Lossin Cc: Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Matthew Wilcox , linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH v2] rust: mm: add abstractions for mm_struct and vm_area_struct Message-ID: <3ffd4742-7a84-434d-ad0d-962f302b977a@proton.me> In-Reply-To: <20240727-vma-v2-1-ab3e5927dc3a@google.com> References: <20240727-vma-v2-1-ab3e5927dc3a@google.com> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: 3803aa190f7afa36c652f96134003f25723ecc16 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 63C704001B X-Stat-Signature: udjxokto3ckgyiick6ru4gzpry4wbhu8 X-Rspam-User: X-HE-Tag: 1722269614-357933 X-HE-Meta: U2FsdGVkX1852a3IvEjnRqk19f8eNqftrKWtm+hpvoFRvjtULVn8fypR+BOONa+8nsscb17P0n6BQb1GMIep1t89ktZCMGWHdLGlA7dp8dbAzlDVliq1/B+e3RmFQpsXDs+XsTh1CHT/e5A1lqh8NWg/LRa6wulRVhbmWF0CQsWwtKEcHI32CGgwUj9py8RqEI301MrINE4UKNJjI6N7NyOqR2fn9LBuu75UZ7uFELvKVIddJ6dkRkClL+p52NK8MLjFkbddT2zuVercVZxGD+w4O3rk/vjLvovbXNyC8W098B8s+fqsQFvkn14VyhTah3uZNeYFNNs+B+5xJj3YDEkJjlPo5sJFv0RXs3jNZay4WMYwEXgtfE8oIitPUw9bwZePU3ZCHqsaIOZ8NgQYJgaMIj3alhFXBradwVJ05HuyvbZmaQz6wLGHuFWL4zacJhYiojGk4mQDoJU0yWYqAPWqp3JCeWFt0y5dBhmJcxbvE2aeL37y6s27P/MIAo03dpvMmzrCyjIQxRbzrAOA9SCxsrwMwd1Bv2toOk2qRYKt08NEbqQkVy9Yr17+SYhA86EeglQTHZNxX7zPNSsp4TZV0SuPnpChd4E4PbM4zSSdHqSKvL8X/xaQ/QKmBZ5UTF+NaCWeZ7toWi5Q7YixgyXbp1u3zWdap+OY50uhvHvczV+PRKPKXebLeGa7+UZVLzmGEFY3HVPy6uEv+5iBXB2SW3J197+ich04GWnjByNEzb+SPsha7CqDJQB5RCNLtufyolxFOInJDqyz2xFQqKuUFxjetWkD/aIjPZ7y/rGZbED7Km4PLlzbUC27oR05wTllqL7crvK27bPeKxX8sAv/QaXc6VbQcwq4cB3df9w0lzCuNXYi/8EoMEC9IxaFG8Z59IZfHJYM4fxt1I5FyTxRm9v3dfzLWLwQ0UGq11Vih6w5vkebb7TvbbGZZfcVqJRKYg5kUD4aEiGCdN0 WPZc95rA Jm/KkN3WE0cBn5c4sXqW4FlKvD1vmhgAtQ83XjqxjI6FmMDgI7clpEswAutx4Zs3EM8kqJKhWCTumJXguojEjSaihv4iC4qh9AfUeogZjpnf2ENXhZzuQfSWl6Ojxcbq1M3OVv+e557gzpXgHF7y3pstp3aleZaCG1AHbSjfi1vQdCv8W/VEk2sshQSQ/3/HITUX8A8UTxWp9REXfMeECNYYv3M4RBt82u0gFFv6aUS/iojhJvsZYF6ezIzxgm4SFfa/ga3ADm0zVBW/GIch/lcu0//3lZYrvH43odDvzQNUNgtf5fVlkGAxriiuM7KZHhKZ2jwdVSOuznYww4ZkvduA20NoT3DbNPnkDF4+GRJydpPzGXp6Hmi1jpkrFM/SbUlitmRx1J9rzoQch88kLBFbVOg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000002, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 27.07.24 11:03, Alice Ryhl wrote: > +/// A wrapper for the kernel's `struct mm_struct`. > +/// > +/// Since `mm_users` may be zero, the associated address space may not e= xist anymore. You must use > +/// [`mmget_not_zero`] before accessing the address space. > +/// > +/// The `ARef` smart pointer holds an `mmgrab` refcount. Its destruc= tor may sleep. > +/// > +/// # Invariants > +/// > +/// Values of this type are always refcounted. > +/// > +/// [`mmget_not_zero`]: Mm::mmget_not_zero > +pub struct Mm { > + mm: Opaque, > +} > + > +/// A wrapper for the kernel's `struct mm_struct`. > +/// > +/// This type is used only when `mm_users` is known to be non-zero at co= mpile-time. It can be used > +/// to access the associated address space. > +/// > +/// The `ARef` smart pointer holds an `mmget` refcount. Its = destructor may sleep. > +/// > +/// # Invariants > +/// > +/// Values of this type are always refcounted. The value of `mm_users` i= s non-zero. > +#[repr(transparent)] > +pub struct MmWithUser { > + mm: Mm, > +} I personally wouldn't sort it this way (so struct decls, methods and then AlwaysRefCounted impl), but I would sort it first by the struct. I find it helpful to have the `AlwaysRefCounted` impl close to the struct declaration (similarly for `Drop`). But that might just be me. > + > +/// Equivalent to `ARef` but uses `mmput_async` in destructo= r. > +/// > +/// 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, I am confused, why doesn't `mm: MM` work here? I.e. also allow usage of `ARef`. Another approach might be to have the function on `MmWithUser`: fn put_async(this: ARef) Or do you need it to be done on drop? > +} > + > +// Make all `Mm` methods available on `MmWithUser`. > +impl Deref for MmWithUser { > + type Target =3D 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? > +} > + > +// 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, > +} > + > +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 operatio= ns 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. > + #[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 muta= ble operations is okay. > + #[inline] > + pub unsafe fn from_raw_vma_mut<'a>(vma: *mut bindings::vm_area_struc= t) -> &'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 f= ield is valid for reading > + // given a shared reference. Why is the field not changed from the C side? Is this part readonly? --- Cheers, Benno