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 7B354D711CA for ; Thu, 21 Nov 2024 10:38:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8796E6B007B; Thu, 21 Nov 2024 05:38:45 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 829266B0082; Thu, 21 Nov 2024 05:38:45 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 717A36B0083; Thu, 21 Nov 2024 05:38:45 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 528B16B007B for ; Thu, 21 Nov 2024 05:38:45 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id E73451C7DB0 for ; Thu, 21 Nov 2024 10:38:44 +0000 (UTC) X-FDA: 82809751752.25.6ACF700 Received: from mail-wr1-f46.google.com (mail-wr1-f46.google.com [209.85.221.46]) by imf05.hostedemail.com (Postfix) with ESMTP id 7410910000E for ; Thu, 21 Nov 2024 10:37:03 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=PSA9Lf10; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf05.hostedemail.com: domain of aliceryhl@google.com designates 209.85.221.46 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1732185455; a=rsa-sha256; cv=none; b=vCpToCIRoID199VPdfx19CMyrwsQwWpESZaDbWkz7+GcSBQOCKKacLrfHxxhMV0xGCjmTZ sitittqHrkdElNKfrqyQN1geOYLgPPF7MJfi97tcVePU+EXknC4R+0LKW0zCGkKG3UbFHu RvF3hdRTQISfVJ3ADFgqkT1AyRvNM5k= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=PSA9Lf10; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf05.hostedemail.com: domain of aliceryhl@google.com designates 209.85.221.46 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=1732185455; 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=arZzzZcZ/LicC2e7GiApVwWtVcvvhGlrT9lVaNGL8IU=; b=zumfyNNb5Wvk73X275u8+VmAhLEafzYRwkK6ZM8/TekKzd2+5h8CDd99kmKEyjXlpEKUUY rlRvxHPMPCqKYh18dx/l9QGbgMUFqZvkNiK5KSmFyRTXkl3ADMrh31MD2WXlAhTg9lhUD7 O4zvI1b7C6j2KSWaCv7t5YasRuNvFW0= Received: by mail-wr1-f46.google.com with SMTP id ffacd0b85a97d-382588b7a5cso512860f8f.3 for ; Thu, 21 Nov 2024 02:38:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1732185521; x=1732790321; 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=arZzzZcZ/LicC2e7GiApVwWtVcvvhGlrT9lVaNGL8IU=; b=PSA9Lf10w+c4qqXi0hbIYrnPuUtn3o00G1x+IXzYm4EF5hRtBOFco74Z4bJ57N9hHS BUC2oL4jF7xv0p/JJrjsLBzsOf7tTXxKe1HPT00g39h0Wcdg1DhAYj1y5UbnZuJ+7sWE N1pc3iRDUiCpMf8OuTlnOZRbyOh1jHxs+kJPtfVt6mfh+mBz3rjdy+WK4loLnxJimPNO bpLZXKeuv6L/bVKIb8r41tjUCOccmNZK4Y9Oe38SA0T++z1Y1phnfNrgWrtAn10DTw8g ODS5aHZw8e8RFIHr4TetuWihVYlfKPClZoMOhIs72eZyEjiBWMiCHmQ3asv4kkD4GZ4r UUCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732185521; x=1732790321; 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=arZzzZcZ/LicC2e7GiApVwWtVcvvhGlrT9lVaNGL8IU=; b=V5weQ70nEupxG/pkch+4PT63rW/0WJI0wqYYbDsnLc8ES8QY7rd+z/5WWAkzWXXnSa 2RxlII09YkhZEjsYUue2rWLXCgMBoar3i6T7YKhvKB3STyjWcihUiuZrqFaxa7C314Sp plm16COmsyNi05+zCBSoiIcyejmaAgdglQcHg+jeWMSFw1OV0lP/Fqfx5CUy6jYztCjb /4E6f6agfcQTYPpqhtCsAFNK2Wqv1sQ8rJEbMlAwkSYab9AMFJ22vwVEIWK3P50OEC5W 9i9gjkkJIFN4wzKmTLqM1B14+JFGvU8eKD52ZugF0AsU4hkKWZter//ZO/kaU9HQ1SoI v06g== X-Forwarded-Encrypted: i=1; AJvYcCVi00WktB4qg7/WIAiryfxc9wx0Y0IOSSb3dRXh5yxL1RB0W1qldfEW5DhU7JlPHEK7DNzrJyEWgQ==@kvack.org X-Gm-Message-State: AOJu0Yz7k76ufHjZmguXmrMts/9iKs4zDkRF+x33QJbic4Z8YL/AdC8m 8zvLQ8g3avOwXnDCgDH7FJSE7kiGqYi6DZXb61oiNUo/1E6ZcOqRiTCLYaTDcdmvhW//WlKaeBv feKRMbec65M8y9k7vAsrONMP2tzudxspkWD2X X-Gm-Gg: ASbGncvYRBPhT7eTGezWy+cHcej80uPfjx8BZll2/rfjPHfKjyonynNRBzhPP3J1Ar9 7w7oxYrOBIGtQwk1jDvd04C7PLRJsa4N+LBxeqMp7aIeXzdaZPML+LWA7hFfsDg== X-Google-Smtp-Source: AGHT+IFKTxfTP/8V3NQnXKNt4YPbwdTYjvzZyWn+f6dBturud/k7FjFa3YKKktaiCpaNOinmPJqBmsBuDnracNpZ2O8= X-Received: by 2002:a05:6000:178a:b0:382:59c1:ccdf with SMTP id ffacd0b85a97d-38259c1ccecmr2210664f8f.46.1732185521340; Thu, 21 Nov 2024 02:38:41 -0800 (PST) MIME-Version: 1.0 References: <20241120-vma-v8-0-eb31425da66b@google.com> <20241120-vma-v8-3-eb31425da66b@google.com> In-Reply-To: From: Alice Ryhl Date: Thu, 21 Nov 2024 11:38:29 +0100 Message-ID: Subject: Re: [PATCH v8 3/7] mm: rust: add vm_insert_page To: Lorenzo Stoakes , Jann Horn Cc: Miguel Ojeda , Matthew Wilcox , Vlastimil Babka , John Hubbard , "Liam R. Howlett" , Andrew Morton , Greg Kroah-Hartman , Arnd Bergmann , Christian Brauner , Alex Gaynor , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org, Andreas Hindborg Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Queue-Id: 7410910000E X-Rspamd-Server: rspam11 X-Stat-Signature: jpmtp7z61hbm3hfb8tru1nmy6x4ox4mc X-HE-Tag: 1732185423-175556 X-HE-Meta: U2FsdGVkX19qDScBnOoeosXAaG5R3bj3+Q0KJ+bjAJCrt196M6ainbHgyPuw1pKqkQjXpMawY6N4m6kJnBiHBQ9pyQOqpTF1Mj7Ns4d4j2rT/vxJWyDV9ohazuDmcaHULNKO+paAfMofcx8FJL4W15zxXGr8EzY5wvcGeaKdi/6JaZHMOD/2v+d+/Wuk8PemOrO6DtLnwa5B147NO/qPz/DTaGUS+89fIP+v63l4qUQTXHs89blmAau9/N62eZNu9U8JenCWBd93qIYjtC3SrCT1of+3zKQnfYP+O9EluYq1Cka3lWHCfoRLBhVqLas1sVmL9PnS3k4XRjJS9PbGWOD+tcXC80Sy0P4ZzTjzNJrzIdjcwJhrjTlLw35fEmeIcybPXmclzl/k6bVoHDmrQTY7Q1nN0YyOVbKDp8/uTzNobGk3d343O4kXDEwXfE5wK5LxmAhafrFdK7bIYJWnbZKlrtVCqCcW/jO8UDaFKEBtdLfwlOhhNyPR4kpKjPF7UNO1yxQu2lcDXBfzUVQR1esEIL27gIVPGM9NKDNQWlcbIjHWYjuLnoJr7lnWG85IO67FqdcdpkvUcQhIgUBI3lizbIK4FZb1WDAJwpxNmccQ7fdTEuqjeaOuuHnpGRMpe16JcNKaHUVGExoXbmwz4I2ZkO2XVJHuQHID3CXkWN5kRQ2rwAaiapS0Q2sA2jsS64FeLzm+meoxVAlaVm/uNKyBU5iCSSsmS05Jf8AiUAWkq5ClSjrPaR3Y/OcFKXWuoJU09vT+8Nxv2NJrshtTNvTUSxW6Vj3PWejSUAPVOD2E+c4bJZUe0aCqMYgz0FfQi8HZ731y/B0gL0kMDNhjd2iauoiiwtFj0JBtxNzxgpeMlHDs6Y6DcqiLlmu7tNtXqiv69WRls7oHFspqzCR32SRmhgmw/qB1VqzE4/KrTqfRKarezXsgC16XcGpZZt7MgmkZqlaiPWXEWiP4U8V HTQ8hasF 4wHvCnLRQjnT+dyWE9d3iLjpxPTA5OXAaegd29iwmJ2CdbOpdKTS+S9PszhKHJlMuPQu/ohyLiRw/PGoiClhntHEeaeB97EUTmSMpos8rLsQS0d2q1PmF92upnjXvI0PkDG8Iz5w2iGZhlhSb1VI3w/0FqUYLyoOiEB/aCtL5RngDgAe7xDt8r8b3MfeAuhTTb73BqQ1ex137WWJV+hnH/ajwEDWerVbFKgjBZbRBJJZLVWGoK2GnKKmiPkjl1Qnc5eKW4elhyZtJ0Eaq933cearFf+JAB3SfEUBp4I8oPrzEyT54H0xznukW/S6Lq0yL9WZUrzvECsz4gwG0cy3jcvO6W76dJ0HkEQ/ySUYz7O1R6I/t6mb8SLnzzPidlWFC9GpIJm/SMUyCgmZr8+4HSzJqsQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, 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 Wed, Nov 20, 2024 at 8:20=E2=80=AFPM Lorenzo Stoakes wrote: > > On Wed, Nov 20, 2024 at 02:49:57PM +0000, Alice Ryhl wrote: > > The vm_insert_page method is only usable on vmas with the VM_MIXEDMAP > > flag, so we introduce a new type to keep track of such vmas. > > Worth mentioning that it can be used for VMAs without this flag set, but = if > so we must hold a _write_ lock to be able to do so, so it can update the > flag itself, however we intend only to use it with VMAs which already hav= e > this flag set. I got the impression from Jann that the automatically-set-VM_MIXEDMAP thing should only be used during mmap, and that VM_MIXEDMAP should not get set after initialization even with a write lock? https://lore.kernel.org/all/CAG48ez3gXicVYXiPsQDmYuPSsKMbES2KRQDk+0ANWSS0zD= kqSw@mail.gmail.com/ > > The approach used in this patch assumes that we will not need to encode > > many flag combinations in the type. I don't think we need to encode mor= e > > than VM_MIXEDMAP and VM_PFNMAP as things are now. However, if that > > becomes necessary, using generic parameters in a single type would scal= e > > better as the number of flags increases. > > > > Signed-off-by: Alice Ryhl > > --- > > rust/kernel/mm/virt.rs | 68 ++++++++++++++++++++++++++++++++++++++++++= +++++++- > > 1 file changed, 67 insertions(+), 1 deletion(-) > > > > diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs > > index 1e755dca46dd..de7f2338810a 100644 > > --- a/rust/kernel/mm/virt.rs > > +++ b/rust/kernel/mm/virt.rs > > @@ -4,7 +4,14 @@ > > > > //! Virtual memory. > > > > -use crate::{bindings, types::Opaque}; > > +use crate::{ > > + bindings, > > + error::{to_result, Result}, > > + page::Page, > > + types::Opaque, > > +}; > > + > > +use core::ops::Deref; > > > > /// A wrapper for the kernel's `struct vm_area_struct` with read acces= s. > > /// > > @@ -80,6 +87,65 @@ pub fn zap_page_range_single(&self, address: usize, = size: usize) { > > ) > > }; > > } > > + > > + /// Check whether the `VM_MIXEDMAP` flag is set. > > + #[inline] > > + pub fn check_mixedmap(&self) -> Option<&VmAreaMixedMap> { > > Nitty + a little bikesheddy (this may be my mistake as I am unfamiliar wi= th > rust idioms also) but shouldn't this be 'as_mixedmap_vma()' or something? You're probably right that this name is more consistent with Rust naming conventions. :) > > + if self.flags() & flags::MIXEDMAP !=3D 0 { > > + // SAFETY: We just checked that `VM_MIXEDMAP` is set. All = other requirements are > > + // satisfied by the type invariants of `VmAreaRef`. > > + Some(unsafe { VmAreaMixedMap::from_raw(self.as_ptr()) }) > > + } else { > > + None > > + } > > + } > > +} > > + > > +/// A wrapper for the kernel's `struct vm_area_struct` with read acces= s and `VM_MIXEDMAP` set. > > +/// > > +/// It represents an area of virtual memory. > > +/// > > +/// # Invariants > > +/// > > +/// The caller must hold the mmap read lock or the vma read lock. The = `VM_MIXEDMAP` flag must be > > +/// set. > > +#[repr(transparent)] > > +pub struct VmAreaMixedMap { > > + vma: VmAreaRef, > > +} > > + > > +// Make all `VmAreaRef` methods available on `VmAreaMixedMap`. > > +impl Deref for VmAreaMixedMap { > > + type Target =3D VmAreaRef; > > + > > + #[inline] > > + fn deref(&self) -> &VmAreaRef { > > + &self.vma > > + } > > +} > > Ah OK, thinking back to the 'impl Deref' from the other patch, I guess th= is > allows you to deref VmAreaMixedMap as a VmAreaRef, does it all sort of > automagically merge methods for you as if they were mix-ins then? Yeah, I use it to "merge" the method sets to avoid duplication. The main limitation is that you can only deref to one other type, so you can't have "diamond inheritance". > > +impl VmAreaMixedMap { > > + /// Access a virtual memory area given a raw pointer. > > + /// > > + /// # Safety > > + /// > > + /// Callers must ensure that `vma` is valid for the duration of 'a= , and that the mmap read lock > > + /// (or stronger) is held for at least the duration of 'a. The `VM= _MIXEDMAP` flag must be set. > > + #[inline] > > + pub unsafe fn from_raw<'a>(vma: *const bindings::vm_area_struct) -= > &'a Self { > > + // SAFETY: The caller ensures that the invariants are satisfie= d for the duration of 'a. > > + unsafe { &*vma.cast() } > > + } > > + > > + /// Maps a single page at the given address within the virtual mem= ory area. > > + /// > > + /// This operation does not take ownership of the page. > > + #[inline] > > + pub fn vm_insert_page(&self, address: usize, page: &Page) -> Resul= t { > > I'm guessing this 'Result' type encodes 0 vs. -Exxx error codes? In this particular case, yes. More generally, Result is a discriminated union containing either Ok(val) for success or Err(err) with some error type. But the default success type is the unit type () and the default error type is kernel::error::Error which corresponds to errno numbers. In this case, the compiler is clever enough to not use a discriminated union and instead represents Ok(()) as 0 and Err(err) as just the negative errno. This works since kernel::error::Error uses NonZeroI32 as its only field (as of 6.13). > > + // SAFETY: The caller has read access and has verified that `V= M_MIXEDMAP` is set. The page > > + // is order 0. The address is checked on the C side so it can = take any value. > > + to_result(unsafe { bindings::vm_insert_page(self.as_ptr(), add= ress as _, page.as_ptr()) }) > > + } > > } > > It's really nice to abstract this as a seprate type and then to know its > methods only apply in known circumstances... I guess in future we can use > clever generic types when it comes to combinations of characteristics tha= t > would otherwise result in a combinatorial explosion? Yeah, so the alternate approach I mention in the commit message would be to have something like this: struct VmAreaRef { ... } listing all the flags we care about. This way, we get 2^3 different types by writing just one. (There are a few different variations on how to do this, instead of bools, we may want to allow three options: true, false, unknown.) Alice