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 0AF51C4828F for ; Thu, 8 Feb 2024 13:46:27 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7D2CD6B0074; Thu, 8 Feb 2024 08:46:27 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 782976B0078; Thu, 8 Feb 2024 08:46:27 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 623A56B007D; Thu, 8 Feb 2024 08:46:27 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 5368C6B0074 for ; Thu, 8 Feb 2024 08:46:27 -0500 (EST) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 196E4A25B7 for ; Thu, 8 Feb 2024 13:46:27 +0000 (UTC) X-FDA: 81768761214.07.3BD67AA Received: from mail-vs1-f45.google.com (mail-vs1-f45.google.com [209.85.217.45]) by imf10.hostedemail.com (Postfix) with ESMTP id 53CC3C0021 for ; Thu, 8 Feb 2024 13:46:25 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=MtAmmjxw; spf=pass (imf10.hostedemail.com: domain of aliceryhl@google.com designates 209.85.217.45 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707399985; 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=/L3VaVUJ969KRlxpqnrPaWtJ6K8FEE+/sV80j/XwhUA=; b=WVl3qmD/pZdX45jBozUWrNifBw0858Psm/NaT8Ew8qnqRVwJsKdfpWZGydFlqX7P5vMu47 KvPipdFFo7VnLfPxWzMXR0EM0IyUxy5Xyc+bfBVMMkrxSWj2WJD8tzJy3PmFXhw5NKOpXj tw2jzMZgyG7VnaDYmuR/LL4f9+kNXUQ= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=MtAmmjxw; spf=pass (imf10.hostedemail.com: domain of aliceryhl@google.com designates 209.85.217.45 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707399985; a=rsa-sha256; cv=none; b=V2TOwmCbRW5cQT1e3YC70GKX0vYgn6m+3aGzDz7fo11e8UgIpL8/MBNjHXR50jRPXrfk2+ ro6nCEByw7SUgRbtbWGqGwhq12qo4fWvAPfO2S9wG2GfC/nWcVIaqLvXeV+WqaA5xOuB5W Wk1JrFdfuUojjKlLDXu/NATFyGuENk8= Received: by mail-vs1-f45.google.com with SMTP id ada2fe7eead31-46d23bf501cso239549137.0 for ; Thu, 08 Feb 2024 05:46:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1707399984; x=1708004784; 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=/L3VaVUJ969KRlxpqnrPaWtJ6K8FEE+/sV80j/XwhUA=; b=MtAmmjxwTrO8a3H0wIFRy6U9lqr4oJlDlyWtd1AG0ZqKYQ5xFTM3EWlKmTBBZ9FZSh i3aiUEMamD7qNaXy5T+CIPeW53ZMmdMVVT1G202VhbCbWqFnnfaHyQWChmfiOflPzsFt zX3xAsBXfn+P6LsWF4tS9dVDeWo0e+WJ5fJqisPrDIDa+jOPotuLnASu9jX8xfmDzEZZ JvORSlCbHwwFq6hDsRdj7y0qDlhD2TKxEitgDA/gKGVgX74VVrkCmyBQtQZ12r4KgjC0 84wVRVW8qwvEwF7XaOURjqv/EAmhLJtY2QpzIYmrhxxptDUqQE6oVtTlzZ8Sb5pl2K46 fwHQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707399984; x=1708004784; 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=/L3VaVUJ969KRlxpqnrPaWtJ6K8FEE+/sV80j/XwhUA=; b=wq29J9uXLjFjnvTrn1eSg2o5S4ExjF/vWBF36JzsF1zeNxCOsAduWJG7ibnzzrYUJx ksHouZ//6FyioyBZAt1Z51FVzXbi2I4CEkO/Own0nuHPpL3Nv9ppDMF/IVI1tF5g0wt5 zXmBv/Nfi2k/ub4GIICjbqt0UV3QonTSRaRx5wdTf8k5LWW1JXBUtWqhBCQqb+0lSIBO 1n60lEOANfyHB0mrZ7NR3HeqB7TRSuji7fSCTIwgz+sqIl6h8zCaZDgPQncDQlQE64zH yPsp/k8tKMh+/lsIO7++CKygFnH9qBjp9llpJdzj5orgvKdX6ubvrr2F3tkTG4paIf4x r0Aw== X-Forwarded-Encrypted: i=1; AJvYcCUYhnOrRm1kDL+7pEdjM2MZZ3nXiiBGbRYjtggzcUIRK3koEbv3gLcJm086MZ3C1c9JntssEAwM2Nel9DMElbZmfbE= X-Gm-Message-State: AOJu0Ywa1w38rfe0o3hUl8xmCetS1LGwO1SRzd91GPoqqSA/FdUPBrSe ZpUP0k/glSLQA781mEOOC07O2rZr55oDYD7q1pJjI/HNZXh0uQChJRR46T2nhY7BN8wRyAXvONI ZtAr/I1vH8Zi+qV4Rg/pqDHQJHv5Bx65JIi9M X-Google-Smtp-Source: AGHT+IH6287k1PPBR4BUh12Zj7QCoeDvAPd2j0KqozKpvxtJQ61zdQBdx/BUaxVAT8L4YQHLbT9e6QxMKbfrbXZy9C4= X-Received: by 2002:a67:f508:0:b0:46d:6339:b9b0 with SMTP id u8-20020a67f508000000b0046d6339b9b0mr1204693vsn.1.1707399984309; Thu, 08 Feb 2024 05:46:24 -0800 (PST) MIME-Version: 1.0 References: <20240124-alice-mm-v1-0-d1abcec83c44@google.com> <20240124-alice-mm-v1-3-d1abcec83c44@google.com> In-Reply-To: From: Alice Ryhl Date: Thu, 8 Feb 2024 14:46:13 +0100 Message-ID: Subject: Re: [PATCH 3/3] rust: add abstraction for `struct page` To: Trevor Gross Cc: Miguel Ojeda , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Andreas Hindborg , Kees Cook , Al Viro , Andrew Morton , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Todd Kjos , Martijn Coenen , Joel Fernandes , Carlos Llamas , Suren Baghdasaryan , Arnd Bergmann , linux-mm@kvack.org, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, Christian Brauner Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 53CC3C0021 X-Rspam-User: X-Stat-Signature: oq4uoejcjfdp89ko7puuu3x7gofgbzxo X-Rspamd-Server: rspam01 X-HE-Tag: 1707399985-96832 X-HE-Meta: U2FsdGVkX1+J239lzKHLcHvyz6FZtJUpeL2xVe9z/YYEgcKKAGYHoFbWFbM59xwakBoRXm0RqhNyXmphM6A1ym0NIjwOI/WBeSYRTQJ2AH7EnUdLJteqEUqLb+XaAwqOWzBNCcm2SAFJJR5i63PojFtwInAhfTy6XTcBoFuvxajGlOEFN9/BAYmplQjSfAKjQf9+OciO0Di2oQH6ivNN1XQQDP3ruh8/gQqSf41Tb7Li24sjue5fuEqDJTiQhHYFUnSMevXrSpVvD6xXtm9LLae2miRlMb40/eHp4l07HVeUy9ejzKHM80uosM8gOHXplsjKPoMhzp9bvI5RakwU6EoCipFO2KP7FmQf6aPfqY/60Q7gUc+M/kRp8ccXZSHmBJi67w69NtD+Tb4V4Lar2hFiN8sTeUZ8+iDA4CxFglW0HkJ8hK5QGGwPcztajYc5g2kxtbGkMGIqDmOtw34jn4UcGJNOsCZibvvdfcztExYDI/Jxs2ATeASgDT0VlJJ7lzn6h0g67OEOmuxLwbLlsgvBQbOmaehBWjOkV14xNf4GALYRyI1pLnB3x9ppfNVsj8kzclhxC/mPvMeLlFOpIRV5g3SOP5KfXjRI7XGORLllYpA/1tnnYUlbuxFWPB2ZTy3PLLl7UoPGzwxIJL3lF7JKSoCsPtGgQWv3YlrMQhoH5Iy2enbAtHDztSSZjJ/G9z9o0AkVIlCWf6uxB1ANVDtH7MBsgVckjD00HblHNuGc68PnbyEnqEqvSk05upfKGLtwVis6jVz1l86TWFOtooi5Fbk9Eali5lyANklGjqN/w8wFNiNYvDDMR0lWcnafvgOBrvY1mNhWid/sMsUbThVs8auLeU9j/Oytu8qpekyR0WLOuRa7Sm88qEbXAGGJ+NwDZzAgKetnmtaS2vemrEIk/28E3XBJZgx19poHY7ozxR8WcI5kHoH3KHyFiZmWp07cckmGUUqZGlsSAVk IUk0fLZ+ +ZENxxOZmp3ySVK2tKDU3/lZuxRdn0BT1OQtqIadv34jJDmS7RqyvrH8lqW+XaKtboJlBsOCWxdmbZBskER1x0+dwwUbMVzR2Ha5G4QGV58NjoNnoDB790xSFuoe82ES2CZAD8PAU1z50RTEI9wK4wkDqbnmm6pVAHYDjelu7ngXGQawn2Ly+u84algrjw4M0s7BbTBtoM5DriRgDwFDdyEhctiu6QZywcc8tIB7gFxFQaX4Wu9KpsvBTwlMI/INVu3a/8KO5cEXQ/pdWnSrI15Jr6zU5HIIAuASjfQGCKTuMq54cJ4TdoYcN2JeMCqdEtAC5 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 Thu, Feb 1, 2024 at 7:02=E2=80=AFAM Trevor Gross wro= te: > > On Wed, Jan 24, 2024 at 6:22=E2=80=AFAM Alice Ryhl = wrote: > > +/// A pointer to a page that owns the page allocation. > > +/// > > +/// # Invariants > > +/// > > +/// The pointer points at a page, and has ownership over the page. > > +pub struct Page { > > + page: NonNull, > > +} > > Shouldn't this be UnsafeCell / Opaque? Since `struct page` contains locks= . That only matters when we use a reference. Here, it's behind a raw pointer. > > +// SAFETY: It is safe to transfer page allocations between threads. > > +unsafe impl Send for Page {} > > + > > +// SAFETY: Calling `&self` methods on this type in parallel is safe. I= t might > > +// allow you to perform a data race on bytes stored in the page, but w= e treat > > +// this like data races on user pointers. > > +unsafe impl Sync for Page {} > > These races should probably be in the Page docs, rather than pointing > to user pointers. New safety comment: SAFETY: As long as the safety requirements for `&self` methods on this type are followed, there is no problem with calling them in parallel. > > +impl Page { > > + /// Allocates a new set of contiguous pages. > > "set of contiguous page" -> "page"? Thanks, done. > > + pub fn new() -> Result { > > + // SAFETY: These are the correct arguments to allocate a singl= e page. > > + let page =3D unsafe { > > + bindings::alloc_pages( > > + bindings::GFP_KERNEL | bindings::__GFP_ZERO | bindings= ::__GFP_HIGHMEM, > > + 0, > > + ) > > + }; > > + > > + match NonNull::new(page) { > > + // INVARIANT: We checked that the allocation above succeed= ed. > > + Some(page) =3D> Ok(Self { page }), > > + None =3D> Err(AllocError), > > + } > > Optionally: > > let page =3D NonNull::new(page).ok_or(AllocError)?; > Ok(Self { page }) Done. > > + } > > + > > + /// Returns a raw pointer to the page. > > Maybe add ", valid for PAGE_SIZE" or similar to make this obvious. This is a pointer to the `struct page`, not the actual page data. > > + pub fn as_ptr(&self) -> *mut bindings::page { > > + self.page.as_ptr() > > + } > > + > > + /// Runs a piece of code with this page mapped to an address. > > Maybe ", then immediately unmaps the page" to make the entire operation c= lear. Ok. > > + /// It is up to the caller to use the provided raw pointer correct= ly. > > + pub fn with_page_mapped(&self, f: impl FnOnce(*mut c_void) -> T= ) -> T { > > If there is exclusive access into the page, this signature could be: > > FnOnce(&mut [u8; PAGE_SIZE]) -> T > > Otherwise possibly > > FnOnce(*mut [u8; PAGE_SIZE]) -> T > > But based on the thread with Boqun it seems there is no synchronized > access here. In this case, "use the provided raw pointer correctly" or > the type level docs should clarify what you can and can't rely on with > pointers into a page. > > E.g. if I'm understanding correctly, you can never construct a &T or > &mut T anywhere in this page unless T is Sync. We discussed this in the meeting and concluded that we should use *mut u8 h= ere. > > + /// Runs a piece of code with a raw pointer to a slice of this pag= e, with > > + /// bounds checking. > > + /// > > + /// If `f` is called, then it will be called with a pointer that p= oints at > > + /// `off` bytes into the page, and the pointer will be valid for a= t least > > + /// `len` bytes. The pointer is only valid on this task, as this m= ethod uses > > + /// a local mapping./ > > + /// > > + /// If `off` and `len` refers to a region outside of this page, th= en this > > + /// method returns `EINVAL` and does not call `f`. > > + pub fn with_pointer_into_page( > > + &self, > > + off: usize, > > + len: usize, > > + f: impl FnOnce(*mut u8) -> Result, > > + ) -> Result { > > Same question about exclusive access > > impl FnOnce(&mut [u8]) -> Result We discussed this in the meeting. Slices raise all sorts of cans of worms with uninit and exclusivity, so the raw methods won't use them. > > + let bounds_ok =3D off <=3D PAGE_SIZE && len <=3D PAGE_SIZE && = (off + len) <=3D PAGE_SIZE; > > + > > + if bounds_ok { > > + self.with_page_mapped(move |page_addr| { > > + // SAFETY: The `off` integer is at most `PAGE_SIZE`, s= o this pointer offset will > > + // result in a pointer that is in bounds or one off th= e end of the page. > > + f(unsafe { page_addr.cast::().add(off) }) > > + }) > > + } else { > > + Err(EINVAL) > > + } > > + } > > + > > + /// Maps the page and reads from it into the given buffer. > > + /// > > + /// # Safety > > + /// > > + /// Callers must ensure that `dest` is valid for writing `len` byt= es. > > + pub unsafe fn read(&self, dest: *mut u8, offset: usize, len: usize= ) -> Result { > > Is there a reason not to use a slice just for a destination to read into? Ditto. > > + self.with_pointer_into_page(offset, len, move |from_ptr| { > > Nit: do the names from_ptr/to_ptr come from existing binder? src/dst > seems more common (also dst vs. dest). Renamed everything to use src/dst > > + self.with_pointer_into_page(offset, len, move |to_ptr| { > > + // SAFETY: If `with_pointer_into_page` calls into this clo= sure, then > > + // it has performed a bounds check and guarantees that `to= _ptr` is > > + // valid for `len` bytes. > > + unsafe { ptr::copy(src, to_ptr, len) }; > > + Ok(()) > > + }) > > + } > > + > > + /// Maps the page and zeroes the given slice. > > Mention that this will error with the same conditions as with_pointer_int= o_page. That method is private. I will add documentation for this that doesn't reference with_pointer_into_page. > > + pub fn fill_zero(&self, offset: usize, len: usize) -> Result { > > + self.with_pointer_into_page(offset, len, move |to_ptr| { > > + // SAFETY: If `with_pointer_into_page` calls into this clo= sure, then > > + // it has performed a bounds check and guarantees that `to= _ptr` is > > + // valid for `len` bytes. > > + unsafe { ptr::write_bytes(to_ptr, 0u8, len) }; > > + Ok(()) > > + }) > > + } > > + > > + /// Copies data from userspace into this page. > > + pub fn copy_into_page( > > + &self, > > + reader: &mut UserSlicePtrReader, > > + offset: usize, > > + len: usize, > > + ) -> Result { > > Maybe copy_from_user_slice or something that includes "user", since > as-is it sounds like copying a page into another page. > > Also, docs should point out the error condition. Done. Thanks, Alice