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 83751C3DA4A for ; Thu, 1 Aug 2024 14:38:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 03D006B00AA; Thu, 1 Aug 2024 10:38:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F2F496B00AC; Thu, 1 Aug 2024 10:38:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DF6B96B00AD; Thu, 1 Aug 2024 10:38:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id C1C4E6B00AA for ; Thu, 1 Aug 2024 10:38:02 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id C8ADB1C420D for ; Thu, 1 Aug 2024 14:38:01 +0000 (UTC) X-FDA: 82403931162.05.D1B515B Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) by imf17.hostedemail.com (Postfix) with ESMTP id DC3FB4002D for ; Thu, 1 Aug 2024 14:37:59 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=hG6r0MTJ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf17.hostedemail.com: domain of aliceryhl@google.com designates 209.85.216.45 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1722523033; a=rsa-sha256; cv=none; b=o+lzC7jvIXBXN6SsFHyGf0BySB59p4dMJn6PwGhkjsgnAGFb1d6RCBKPe36wa6HKWr/lb5 nCUOdecXzuMoTYxT4k1nzVvIgpyt7zFzOtZVuxQ1X1HVH6of9Tyw1mSiJeGAJwZ8cFL17c OBDYNFz6LJX7vv86Goh/yHqnwrslxQY= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=hG6r0MTJ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf17.hostedemail.com: domain of aliceryhl@google.com designates 209.85.216.45 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1722523033; 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=AYrfw+mQ2Y9ZAJL6SkIKQchY5Op0ff4hPWkSjHPxYXQ=; b=wN5tTstu+LAhI0+Mhe1f2vaS/1x12gspXeTQfKKAj/7vbhIop/GZlOnIrzjUY5zx2YlScC igEPrI4oznFlXa9/vXL388Ag4NGNRWmvl2c8t+6VQEwHhLmYoTmixpEDH4CjL0y4xzYpo0 nokFPivVlwThTz4n2mvs5VpgnM2t4sM= Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-2cb4b7fef4aso5402220a91.0 for ; Thu, 01 Aug 2024 07:37:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1722523078; x=1723127878; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=AYrfw+mQ2Y9ZAJL6SkIKQchY5Op0ff4hPWkSjHPxYXQ=; b=hG6r0MTJD6wY1doQwargGhc5c0HY10QvqalrVm7KI4urVCtmvKTMRDwOxHa6NrtuWK 3m2IhyBxjcXfz0NUDnEZSrTsOnIBLiAFaEuMjo39ANFnFmf7bs4ebcWkr8e6ENYy5/Ze eiXPdJnQdGv3zfOIHUw2yjNDPLCKHXsicBlyGwsw2MtHNmiy8BT2KTsx7dcZEBGA4++r jFxm+2Q9IBWgPwrPiVcURoYwpNbpN3xNsZzyQmwGC38S8dqzGSRWCFdI16hpGE7b2d0P wj3Ar1j0xEGXpD7apzO93FAM57MH6MDFD6+D5+GVLw7B9t2AWQxQoJBFp36ypQN3q7xF hbvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722523078; x=1723127878; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=AYrfw+mQ2Y9ZAJL6SkIKQchY5Op0ff4hPWkSjHPxYXQ=; b=N7qQ01NE2AvNZCLThCjQhBpczfwpLCnYaC/t6MlHYHDgYjUOcagkRT6MNSlWEHHcso WyvDUr7GYOf+tyNOr5mhBMUTwl8xEZgRz9+U2IuQjGx9Q9m5UeuKDCsIL2E92WSrNt23 57Mc6Qqbj8Sz27dbedi51nag0pdco9u4DH0eIvN1mafI50JQLsPq431reF9DpZYOXYIt f1VNClRHpC+Bjw+l8yFkyf7jSR47QPKZQfZtpjAmziffcHOgUdSR8Kt71pirqDxaXKc0 1H9EteeyPeBkXuXurNuPlsalA9gXs+uCRR8RR8UHYUeH7bgAIlz0sXol/D0Vw2mr0V3S XHmg== X-Forwarded-Encrypted: i=1; AJvYcCUpUaWXl/SDJYipGLYwYsIVOG/UgWnwfNZ3eeOpulcH6JVlaPLjnFAy1J9DNHUWZ6mpMZ0Q1wqFpOGAnRcnwtYdWCc= X-Gm-Message-State: AOJu0YxFGvdeOt6chlD1Eem5V0BIJlL2iwFkeAJ86eLo2F7A+lW+CTwp CLlTX5R18W+RYBCVXI4BlhMUNXAtTXT2/1uMGnbUdLsURImAd/4ZXPfWvISIjb/SiUHNAOd6w8i jRaKI+CMGRUrp1dw1fV+mQYzjQCZ6EI6YU0WD X-Google-Smtp-Source: AGHT+IGM5tes2Q681ojbEa3ua0VZNkscR8OWXhPYOXiLRwQM1msyp12e3tEEXA8kn5ispLVIwVd36W3PDg6lAjJIyQs= X-Received: by 2002:a17:90b:1652:b0:2cb:55f9:b7c5 with SMTP id 98e67ed59e1d1-2cff9415bbfmr467895a91.12.1722523078128; Thu, 01 Aug 2024 07:37:58 -0700 (PDT) MIME-Version: 1.0 References: <20240801-vma-v3-1-db6c1c0afda9@google.com> <82e4816c-cada-46f3-bebf-882ae8ded118@proton.me> In-Reply-To: <82e4816c-cada-46f3-bebf-882ae8ded118@proton.me> From: Alice Ryhl Date: Thu, 1 Aug 2024 16:37:44 +0200 Message-ID: Subject: Re: [PATCH v3] rust: mm: add abstractions for mm_struct and vm_area_struct To: Benno Lossin Cc: Miguel Ojeda , Andrew Morton , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Matthew Wilcox , "Liam R. Howlett" , Vlastimil Babka , Lorenzo Stoakes , linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: DC3FB4002D X-Stat-Signature: iu3zwdd7rmf8oa43rrxrqdze7ui8cbjw X-Rspam-User: X-HE-Tag: 1722523079-717301 X-HE-Meta: U2FsdGVkX189hti8OdobMiHz6gHhZVjLYcEI3VU6mU65eNa6046nPDbLtgBjVEPKc2gjO7FUtPrv+PbTD19Wony5cYR0/Xr8IJrPcTzdiJ3zjgPJncqgR1q50GFCFMiG+WUD4yIOu1zDQA5xi3xh1rDsJYkCxCuQre1QIB8CIZfkf2VMjeczJzwxeYjeeHGkaiJ1GatOwUUMyeGMH/nSHxt/2dUx1TKnYmwuhfMkhTXh2XT1/lask1YFrhYNSEl5S+idZ6SswzzHFW7toi9Fij944TDPDga5sKtckP1LEkhvGeDQQr5OBKBResX1YuUtFMSJIpayK2+nC0Y6ZShYLODzHeana2NYtoUKTqaXOelyDjd26jUIXVsyT/qNFHOXCFcCyGKEWpYQ8HBZlz+PzE81ioLFsosYtIJep9UX6KQSz32RRkU0Kl6daLzFfzcoyEWaQOb/6qV7m1ZRzW72eMIh89gmt6JTwTw2T6vSKdXDU3IqiQHR1hSJFlzfUcbG/KYsdj6CgqcG/L8Q+yRjHQi9Krd7D2bhgO/wB8Y0KBukkN2KNBxS95nxgCQ/+2pfD6P0PXS832j/HxlvKiuQnXwfR1ZGlfH5g9rsUsfQoQw+xUQA8Ve1tshlIWiToETXzvnTHfeZnWtOv0z87g63c1lhmWtvsTn2SFGrLUI0s2H5/N5w2fw5qQI5s0NMS9S9v/TuW2oFgUB879AmJUJCWYwg956hqU8F6GSaBFWe+1wkPv1RhjdEAoKQ4kbMFSoSAr3XpQsVChemn3CEEqKOIVWd5NgSLs+Y7fpuopT1uiGaSsxuSktZg7ta67WQFVM313dAsgF0KzzQQwy+bGGEDEoFc1Y0W/Rg+fI0wyTFRJWuwKIHxfNKrXCLGhN0zYMigZlSnaTc8najnkQCglujZ6O8F4ual1TX2lnOeHFvm02WIPHTuCzzrnfVxYd2A/q8eB5KSgUB8ry7XGbyXFY dOFhlPXw OYFmI32YHSYhiZYR9OMVWpmhP6ZDbHB8+SADKQL36QcovI4J3qkmi6XbFoJqydW1fgXS5uQ7Lt7sApwlkbuaKwRI6oLJb+2Th1O8PHB0Q85A0tsKyXjiqyBCo/lqN6xQBSXn2ejVD0CfPFm+IL/7PXSJ06Y1gDxtPG8GNOh8irg+zeLXZR/Bqjp3It8pZ3MBXRJwh/dDdDi35ZQcOfc2vMt21/eoqcp/4JM4XY7uIHF098+pkzomeIJ7ODeU35WadGdd3FQEYaSgoyX5TE8Ziwoz6tQsaIKeIiUke X-Bogosity: Ham, tests=bogofilter, spamicity=0.000390, 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 Thu, Aug 1, 2024 at 4:02=E2=80=AFPM Benno Lossin wrote: > > On 01.08.24 14:58, Alice Ryhl wrote: > > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs > > new file mode 100644 > > index 000000000000..ed2db893fb79 > > --- /dev/null > > +++ b/rust/kernel/mm.rs > > @@ -0,0 +1,337 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +// Copyright (C) 2024 Google LLC. > > + > > +//! Memory management. > > +//! > > +//! C header: [`include/linux/mm.h`](../../../../include/linux/mm.h) > > + > > +use crate::{ > > + bindings, > > + types::{ARef, AlwaysRefCounted, Opaque}, > > +}; > > + > > +use core::{ > > + ops::Deref, > > + ptr::{self, NonNull}, > > +}; > > + > > +pub mod virt; > > + > > +/// A wrapper for the kernel's `struct mm_struct`. > > +/// > > +/// Since `mm_users` may be zero, the associated address space may not= exist anymore. You must use > > Can it also be the case that the space never existed to begin with? Then > I would write "the associated address space may not exist." > > Also I think it makes more sense to use "You can use [`mmget_not_zero`] > to be able to access the address space." instead of the second sentence. I'm not sure if it can be the case that the space never existed, but I'll reword. > > +/// [`mmget_not_zero`] before accessing the address space. > > +/// > > +/// The `ARef` smart pointer holds an `mmgrab` refcount. Its destr= uctor may sleep. > > +/// > > +/// # Invariants > > +/// > > +/// Values of this type are always refcounted. > > Would be good to record the refcount used in the invariant. Ok. > > +/// > > +/// [`mmget_not_zero`]: Mm::mmget_not_zero > > +pub struct Mm { > > + mm: Opaque, > > +} > > + > > +// SAFETY: It is safe to call `mmdrop` on another thread than where `m= mgrab` was called. > > +unsafe impl Send for Mm {} > > +// SAFETY: All methods on `Mm` can be called in parallel from several = threads. > > +unsafe impl Sync for Mm {} > > + > > +// SAFETY: By the type invariants, this type is always refcounted. > > +unsafe impl AlwaysRefCounted for Mm { > > + fn inc_ref(&self) { > > + // SAFETY: The pointer is valid since self is a reference. > > + unsafe { bindings::mmgrab(self.as_raw()) }; > > + } > > + > > + unsafe fn dec_ref(obj: NonNull) { > > + // SAFETY: The caller is giving up their refcount. > > + unsafe { bindings::mmdrop(obj.cast().as_ptr()) }; > > + } > > +} > > + > > +/// A wrapper for the kernel's `struct mm_struct`. > > +/// > > +/// This type is used only when `mm_users` is known to be non-zero at = compile-time. It can be used > > I find the "This type is used only when" a bit weird, what about "Like > an [`Mm`], but with non-zero `mm_users`."? Will reword. > > +/// to access the associated address space. > > +/// > > +/// The `ARef` smart pointer holds an `mmget` refcount. It= s destructor may sleep. > > +/// > > +/// # Invariants > > +/// > > +/// Values of this type are always refcounted. The value of `mm_users`= is non-zero. > > +#[repr(transparent)] > > +pub struct MmWithUser { > > + mm: Mm, > > +} > > + > > +// SAFETY: It is safe to call `mmput` on another thread than where `mm= get` was called. > > +unsafe impl Send for MmWithUser {} > > +// SAFETY: All methods on `MmWithUser` can be called in parallel from = several threads. > > +unsafe impl Sync for MmWithUser {} > > + > > +// SAFETY: By the type invariants, this type is always refcounted. > > +unsafe impl AlwaysRefCounted for MmWithUser { > > + fn inc_ref(&self) { > > + // SAFETY: The pointer is valid since self is a reference. > > + unsafe { bindings::mmget(self.as_raw()) }; > > + } > > + > > + unsafe fn dec_ref(obj: NonNull) { > > + // SAFETY: The caller is giving up their refcount. > > + unsafe { bindings::mmput(obj.cast().as_ptr()) }; > > + } > > +} > > + > > +// Make all `Mm` methods available on `MmWithUser`. > > +impl Deref for MmWithUser { > > + type Target =3D Mm; > > + > > + #[inline] > > + fn deref(&self) -> &Mm { > > + &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= ` is safe to call in atomic > > +/// context. > > Missing Invariants. Hmm. Structs will inherit invariants from their fields, no? > > +#[repr(transparent)] > > +pub struct MmWithUserAsync { > > + mm: MmWithUser, > > +} > > + > > +// SAFETY: It is safe to call `mmput_async` on another thread than whe= re `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 { > > + fn inc_ref(&self) { > > + // SAFETY: The pointer is valid since self is a reference. > > + unsafe { bindings::mmget(self.as_raw()) }; > > + } > > + > > + unsafe fn dec_ref(obj: NonNull) { > > + // 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 =3D MmWithUser; > > + > > + #[inline] > > + fn deref(&self) -> &MmWithUser { > > + &self.mm > > + } > > +} > > + > > +// These methods are safe to call even if `mm_users` is zero. > > +impl Mm { > > + /// Call `mmgrab` on `current.mm`. > > + #[inline] > > + pub fn mmgrab_current() -> Option> { > > + // SAFETY: It's safe to get the `mm` field from current. > > + let mm =3D unsafe { > > + let current =3D bindings::get_current(); > > + (*current).mm > > + }; > > + > > + let mm =3D NonNull::new(mm)?; > > + > > + // SAFETY: We just checked that `mm` is not null. > > + unsafe { bindings::mmgrab(mm.as_ptr()) }; > > + > > + // SAFETY: We just created an `mmgrab` refcount. Layouts are c= ompatible due to > > + // repr(transparent). > > + Some(unsafe { ARef::from_raw(mm.cast()) }) > > + } > > + > > + /// Returns a raw pointer to the inner `mm_struct`. > > + #[inline] > > + pub fn as_raw(&self) -> *mut bindings::mm_struct { > > + self.mm.get() > > + } > > + > > + /// Obtain a reference from a raw pointer. > > + /// > > + /// # Safety > > + /// > > + /// The caller must ensure that `ptr` points at an `mm_struct`, an= d that it is not deallocated > > + /// during the lifetime 'a. > > + #[inline] > > + pub unsafe fn from_raw_mm<'a>(ptr: *const bindings::mm_struct) -> = &'a Mm { > > Why not just `from_raw`? See response to Danilo. Alice