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 3DC51C54E58 for ; Thu, 21 Mar 2024 14:16:22 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9C1126B0088; Thu, 21 Mar 2024 10:16:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 970DC6B0089; Thu, 21 Mar 2024 10:16:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 838B26B008A; Thu, 21 Mar 2024 10:16:21 -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 709656B0088 for ; Thu, 21 Mar 2024 10:16:21 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 0E93FA051E for ; Thu, 21 Mar 2024 14:16:21 +0000 (UTC) X-FDA: 81921246162.04.70C3D4E Received: from mail-oi1-f171.google.com (mail-oi1-f171.google.com [209.85.167.171]) by imf14.hostedemail.com (Postfix) with ESMTP id 3EA1D100023 for ; Thu, 21 Mar 2024 14:16:19 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=JbNSZLfh; spf=pass (imf14.hostedemail.com: domain of aliceryhl@google.com designates 209.85.167.171 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=1711030579; 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=9PbBzEMEXXAeq4LLgNricDK4jILoebAZr7BEjc950wk=; b=Vwqpc7NpWBYLpOieor4dBaJ+Hk3KK5E5vdQ23FxPHhtXz09TN7ssz1+O+u45lh15+vw95U CABKKo8kUB80PSzkypFXDBpFojOGFvW/Ye/GRW6sV4EjA0gaBuXj4jflOULkWzcODXczPb LUtOkyxeM1XQnlOQ3HI0Kq4ZqiqfjCk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711030579; a=rsa-sha256; cv=none; b=MkS+R4qnreQMIUJTqibD7LRRwy9iOn8nwDyU/V1TGb29/SHt0YPKnjq9M3HUyYKfhRde7A TYXh5s0zxgR9lVLTf/XOMh4NIHV4k14EIwEHcF+fehev7lLbYSTT3P4Vb/85ac7T60Q++I wb5NbEl7ctUfHGAROMpr6jUBkYm/w6M= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=JbNSZLfh; spf=pass (imf14.hostedemail.com: domain of aliceryhl@google.com designates 209.85.167.171 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-oi1-f171.google.com with SMTP id 5614622812f47-3c3a4101721so650894b6e.1 for ; Thu, 21 Mar 2024 07:16:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1711030578; x=1711635378; 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=9PbBzEMEXXAeq4LLgNricDK4jILoebAZr7BEjc950wk=; b=JbNSZLfhz+co3Qq9+mxCIqFSnWU8kQz1aOrx+tULMzRwZ/y4o+kvGkUEOgigzywOeT RmTzzXgg97mYFUyxbyTvf1LdbbTH/kzZTR6HNu4mibSLlnjASUiIJgUioyiqRHJkoXju Eo8Q/kdFxXqAZjfaCNkc11twIGPNWcO7u9ymyB5EAI9nvP6jJuFNegRPvM+t0c3UI3gM u+G7gg7fASokx/W4CjhGbWd1V58bSoxS1X57rkKdrFEG9/pBj8DDIZLWI2Qqpa3AnoY1 J4VVpB6Lxo4i9eQbrRTe4TSS2Udngqb3mAproGW/Mr1at1Ob2EtXfozrFLYznLESpQMM DcnA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711030578; x=1711635378; 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=9PbBzEMEXXAeq4LLgNricDK4jILoebAZr7BEjc950wk=; b=HmgiRZgabEq8UVCG7aTCYNNaciHtW7IakQNpYMNVPnC+85OHuV1xsWN3CFYD1u8Eu0 4yH3BkH0YzxwxTphwU1JdMaXGb5OxaWkZwA3a9Y0oERZyqPWbRfH7A6ZLM4SKl17anZ9 qfpF7kxU69XCZ53X9f1ZHvMvOwOxtX0HVs0rzfDCz0OlRVz5AiRO53C1zGV7lgd/juth jPk7WvvVMbx/Yj7kv5829FMQ0WQICdH1BiwCxzqVV2sMMtxxTzBa8E6exqrqsVww4ATa 56eB5EvaMFEAXoPBe3ojOuE9xInBjbd2wVwzDCQ7zSX6f/EV9eFDr+XVzZJ2SUMe9e68 hAjw== X-Forwarded-Encrypted: i=1; AJvYcCW3yDk0n6B2WUSIkPh5IdX8Q3aJKpM7O/EYkcSCtgU3za8n1wpzRgTHgLmiNJwbRgvux3l5mLA8+VCn8JualQkYLCs= X-Gm-Message-State: AOJu0YyeXj2lhksG4zMZZn4+xuFTahiWDYtYSvJ4Ndx5Fx/tZJJyp/gS CWWkL+iTeKjVPtMwhPQSz3CR2Vzf8OcsBNKgfS947JGLOUNeJvED7dCIfsqwMinyVVcEAPwTiCl KzVYvPV9KhUMTCtYGYoELOhxNY5i1M49IwYow X-Google-Smtp-Source: AGHT+IH4TEVVFQK/YJPG19j7ZpbabVGsjRX1xScFkHE3KNMxeLkKxELqwdZFyKY+cYQcAqD1S7so3f6XqUodsJZwoyk= X-Received: by 2002:a05:6808:1292:b0:3c3:865b:5311 with SMTP id a18-20020a056808129200b003c3865b5311mr11092810oiw.48.1711030577997; Thu, 21 Mar 2024 07:16:17 -0700 (PDT) MIME-Version: 1.0 References: <20240320084630.2727355-1-aliceryhl@google.com> <9ed03148-5ceb-40f2-9c2d-31e2b8918888@proton.me> In-Reply-To: From: Alice Ryhl Date: Thu, 21 Mar 2024 15:16:07 +0100 Message-ID: Subject: Re: [PATCH v3 4/4] rust: add abstraction for `struct page` To: Benno Lossin Cc: a.hindborg@samsung.com, akpm@linux-foundation.org, alex.gaynor@gmail.com, arnd@arndb.de, arve@android.com, bjorn3_gh@protonmail.com, boqun.feng@gmail.com, brauner@kernel.org, cmllamas@google.com, gary@garyguo.net, gregkh@linuxfoundation.org, joel@joelfernandes.org, keescook@chromium.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, maco@android.com, ojeda@kernel.org, rust-for-linux@vger.kernel.org, surenb@google.com, tkjos@android.com, viro@zeniv.linux.org.uk, wedsonaf@gmail.com, willy@infradead.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 3EA1D100023 X-Rspam-User: X-Stat-Signature: pd4p9kcqwcdd7yhncm6xbosgepxraw8k X-Rspamd-Server: rspam03 X-HE-Tag: 1711030579-180767 X-HE-Meta: U2FsdGVkX18uWb6LAZtJevMnbCbSwpQsrcstKf4I+BCBk4eGc7zYeyslTwpfW1HO5JAEZXrfH3Y3IGQXmCgUNcWk/ImlCsKKhQT8ZlRq0lx5B4Yu2R0/saLN5iHwasRoNtf6aUhbz1E2WQtiTVNhAx6BI/7kBoGUiwgTgM7WDUi6kEiYMwTIVK5h5enGrIuWmqVN4i2Kqb87MW8OW6uk5Ok+xr8zwjNuKqVV77qNiV+hsNaoEcZ2/P/By8NHYQU064l+SoO778mU3By69WeWG08T8EAm3OhvbLz3UfJ8ERvCyXD6zn1rQT2C1YAkXf+SY6RXsXUBqXcVvVNRq8LGFZq/g7rW2ODdH9wUt6Gg98F73hmytcAopSicvykKhIDbQJUBdcCmeIqp65pTU3ksEfU4/yNDSVKzKxT0FqmVNGsxPXiLr/XY/D83QglYlfyo1Ff0IOj362EH9emew68q7yot/TxnE6cUpXtV6Vjf/wHmqO1Jorw44UpLuakywQdSsGSsuSpt/lfbXTBisuzedNKhvHdxPG9l30HLfOIyg87aiykg2XzRoTkRhJR3BHs3BYa+b0NKjeyG3j6rXH0jSWKj2HBY6VFh5bkWdusJ2ZeXTQKVgYSMBBwpMfKZRRPyYyVvUhT1xvEJMIxVRiN7ZHnqEO/8y4dzD5v1tK6p2mLX3BCWBUOv3FXsiypfAWlLNLSSbz4pteftCKVQNVJ/VzKPpvChnj2vMkxEQeyFES0CgC7zKatGML4agWxDinEa58U0iNMt3vwfnujkX37hAIv/1PBK9w8/nCFldDSbjiZsGpJdMFFHJYDt+0pNpf+SP6e6DSZCeTnxTc6Q2bWtkOX8PCKQUTOEXUv+nhmAs6roYTlCFjsXej0X4dLkJ/qlgIOdiqtL5p15CzwtEg3/7Vcx/uskcngvb2HtgIwH0UdamRmHMsuyIg1Pmc5Vh2e62UwfCFHIttKnxtxkP2K XWPmFk5R iLlXR9FWZEeiLDKGJcCaPUhXdUsJGPO58wLeWaHxpyNvHAk3FGaEeR/uW3juqbDhnIMAlO8Xl6VggrnMENRDGhR04H72MuUJZRZ/y17rRzb7IW1pazPl0lnvDBtUcLDTn2EWixGkDuU9nK8bfrOWKEV7cFlyYfiYutpAbzS2wDQSioe0YT8vtqEgDzGfd7j5Tj5n1WVRvbu2H73oQBa9eSFWMstdypzdee7zKO+HIiRki6mrTk2ccbWOahYIxrtoUCZJm+p16fqgrbUT0IloQCGp5bgSZEl5hfQdcoH37KmS5mVSLlTMp2cjQzpOgXLWQIPKX 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, Mar 21, 2024 at 3:11=E2=80=AFPM Alice Ryhl w= rote: > > On Thu, Mar 21, 2024 at 2:56=E2=80=AFPM Benno Lossin wrote: > > > > On 3/21/24 14:42, Alice Ryhl wrote: > > > On Thu, Mar 21, 2024 at 2:16=E2=80=AFPM Benno Lossin wrote: > > >> > > >> On 3/20/24 09:46, Alice Ryhl wrote: > > >>>> On 3/11/24 11:47, 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 pag= e. > > >>>> > > >>>> Why not "`page` is valid"? > > >>>> Do you mean by ownership of the page that `page` has ownership of = the > > >>>> allocation, or does that entail any other property/privilege? > > >>> > > >>> I can add "at a valid page". > > >> > > >> I don't think that helps, what you need as an invariant is that the > > >> pointer is valid. > > > > > > To me "points at a page" implies that the pointer is valid. I mean, i= f > > > it was dangling, it would not point at a page? > > > > > > But I can reword to something else if you have a preferred phrasing. > > > > I would just say "`page` is valid" or "`self.page` is valid". > > > > >>>>> + /// Runs a piece of code with this page mapped to an address= . > > >>>>> + /// > > >>>>> + /// The page is unmapped when this call returns. > > >>>>> + /// > > >>>>> + /// It is up to the caller to use the provided raw pointer c= orrectly. > > >>>> > > >>>> This says nothing about what 'correctly' means. What I gathered fr= om the > > >>>> implementation is that the supplied pointer is valid for the execu= tion > > >>>> of `f` for `PAGE_SIZE` bytes. > > >>>> What other things are you allowed to rely upon? > > >>>> > > >>>> Is it really OK for this function to be called from multiple threa= ds? > > >>>> Could that not result in the same page being mapped multiple times= ? If > > >>>> that is fine, what about potential data races when two threads wri= te to > > >>>> the pointer given to `f`? > > >>>> > > >>>>> + pub fn with_page_mapped(&self, f: impl FnOnce(*mut u8) ->= T) -> T { > > >>> > > >>> I will say: > > >>> > > >>> /// It is up to the caller to use the provided raw pointer correctl= y. > > >>> /// The pointer is valid for `PAGE_SIZE` bytes and for the duration= in > > >>> /// which the closure is called. Depending on the gfp flags and ker= nel > > >>> /// configuration, the pointer may only be mapped on the current th= read, > > >>> /// and in those cases, dereferencing it on other threads is UB. Ot= her > > >>> /// than that, the usual rules for dereferencing a raw pointer appl= y. > > >>> /// (E.g., don't cause data races, the memory may be uninitialized,= and > > >>> /// so on.) > > >> > > >> I would simplify and drop "depending on the gfp flags and kernel..."= and > > >> just say that the pointer is only valid on the current thread. > > > > > > Sure, that works for me. > > > > > >> Also would it make sense to make the pointer type *mut [u8; PAGE_SIZ= E]? > > > > > > I think it's a trade-off. That makes the code more error-prone, since > > > `pointer::add` now doesn't move by a number of bytes, but a number of > > > pages. > > > > Yeah. As long as you document that the pointer is valid for r/w with > > offsets in `0..PAGE_SIZE` bytes, leaving the type as is, is fine by me. > > > > > > >>> It's okay to map it multiple times from different threads. > > >> > > >> Do you still need to take care of data races? > > >> So would it be fine to execute this code on two threads in parallel? > > >> > > >> static PAGE: Page =3D ...; // assume we have a page accessible= by both threads > > >> > > >> PAGE.with_page_mapped(|ptr| { > > >> loop { > > >> unsafe { ptr.write(0) }; > > >> pr_info!("{}", unsafe { ptr.read() }); > > >> } > > >> }); > > > > > > Like I said, the usual pointer rules apply. Two threads can access it > > > in parallel as long as one of the following are satisfied: > > > > > > * Both accesses are reads. > > > * Both accesses are atomic. > > > * They access disjoint byte ranges. > > > > > > Other than the fact that it uses a thread-local mapping on machines > > > that can't address all of their memory at the same time, it's > > > completely normal memory. It's literally just a PAGE_SIZE-aligned > > > allocation of PAGE_SIZE bytes. > > > > Thanks for the info, what do you think of this?: > > > > /// It is up to the caller to use the provided raw pointer correctly. T= he pointer is valid for reads > > /// and writes for `PAGE_SIZE` bytes and for the duration in which the = closure is called. The > > /// pointer must only be used on the current thread. The caller must al= so ensure that no data races > > /// occur: when mapping the same page on two threads accesses to memory= with the same offset must be > > /// synchronized. > > I would much rather phrase it in terms of "the usual pointer" rules. I > mean, the memory could also be uninitialized if you don't pass > __GFP_ZERO when you create it, so you also have to make sure to follow > the rules about uninitialized memory. I don't want to be in the > business of listing all requirements for accessing memory here. > > > >> If this is not allowed, I don't really like the API. As a raw versio= n it > > >> would be fine, but I think we should have a safer version (eg by tak= ing > > >> `&mut self`). > > > > > > I don't understand what you mean. It is the *most* raw API that `Page= ` > > > has. I can make them private if you want me to. The API cannot take > > > `&mut self` because I need to be able to unsafely perform concurrent > > > writes to disjoint byte ranges. > > > > If you don't need these functions to be public, I think we should > > definitely make them private. > > Also we could add a `raw` suffix to the functions to make it clear that > > it is a primitive API. If you think that it is highly unlikely that we > > get a safer version, then I don't think there is value in adding the > > suffix. > > The old code on the Rust branch didn't have these functions, but > that's because the old `read_raw` and `write_raw` methods did all of > these things directly in their implementation: > > * Map the memory so we can get a pointer. > * Get a pointer to a subslice (with bounds checks!) > * Do the actual read/write. > > I thought that doing this many things in a single function was > convoluted, so I decided to refactor the code by extracting the "get a > pointer to the page" logic into `with_page_mapped` and the "point to > subslice with bounds check" logic into `with_pointer_into_page`. That > way, each function has only one responsibility, instead of mixing > three responsibilities into one. > > So even if we get a safer version, I would not want to get rid of this > method. I don't want to inline its implementation into more > complicated functions. The safer method would call the raw method, and > then do whatever additional logic it wants to do on top of that. Adding to this: To me, we *do* already have safer versions of this method. Those are the read_raw and write_raw and fill_zero and copy_from_user_slice methods. Alice