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 394A2C54E58 for ; Thu, 21 Mar 2024 13:56:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B08016B0082; Thu, 21 Mar 2024 09:56:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AB75D6B0083; Thu, 21 Mar 2024 09:56:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9A5C86B0087; Thu, 21 Mar 2024 09:56:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 8BFD16B0082 for ; Thu, 21 Mar 2024 09:56:29 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 6046E1603BB for ; Thu, 21 Mar 2024 13:56:29 +0000 (UTC) X-FDA: 81921196098.07.1941F6B Received: from mail-4322.protonmail.ch (mail-4322.protonmail.ch [185.70.43.22]) by imf08.hostedemail.com (Postfix) with ESMTP id 89067160014 for ; Thu, 21 Mar 2024 13:56:27 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=CwO5JKKg; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf08.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.22 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=1711029387; 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=+DZVzwGA3DNwg1DykuRDpqsopt/Gmedd14krbU6mhdE=; b=0PczXEuJ9q6yKKktKh2Z9ZDUZ6m1h+4tOs4QXLQaWn3DIWCvGrcDmHN4XJKQXunUPK2Lm3 UdJjVgh4RZOGp9LMWudpaAH85DRjVVIJuEzCJKR8E2Sr0SHWDFJG8XV6LABr4w67lwH2gg 3ulbigxPD7breigIwpI9BogAUOuJHUU= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=CwO5JKKg; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf08.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.22 as permitted sender) smtp.mailfrom=benno.lossin@proton.me ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711029387; a=rsa-sha256; cv=none; b=lDbIwQkSXqmqgqiMqtn7n2Zy3epzJ2W37G3UZEn5EAInS6zT5KtruJ8jcbaaXgAt5K4dFl KepdVRmoOxqk6XZwMpGeq0DEKV0PJ2UJnqq/rOEOIh4/eIT4YzSTBCU8v4v6cQAwkTDAwJ rOREjlfXlput+2k2r2tFow5dQySIrQo= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1711029385; x=1711288585; bh=+DZVzwGA3DNwg1DykuRDpqsopt/Gmedd14krbU6mhdE=; 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=CwO5JKKgXVn+JoGs/oxfHnLuOyuxpDFkQiKIHtgtT472HlueJhv0iSK590fc+QdMr 4ZZvdyJfubOA8rbBKo2cbbYcbxRANSBUeL3kxYYKUMv1Rk/BeIsHh1JwT4ZstA6VRF czmYQzARvgK0c39fgn0Dfq27/7/VF+zelB8iskbVAoCPdmaNc7zJlgYCpl1S8hBUir /dq64fhB4jQaqrCMX+tH/8OPGLRcHV5qfhVLhl90sh9WXwfSFDWPARDMrXdFqb0CQR adGovePRsjpcc92pFMbrvFVfk7tKdAkzrmDzUX/9LAFBzAkYwRGSZE3gfvKg6rA/XI /kG4xT5Y5ceOQ== Date: Thu, 21 Mar 2024 13:56:11 +0000 To: Alice Ryhl From: 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 Subject: Re: [PATCH v3 4/4] rust: add abstraction for `struct page` Message-ID: In-Reply-To: References: <20240320084630.2727355-1-aliceryhl@google.com> <9ed03148-5ceb-40f2-9c2d-31e2b8918888@proton.me> Feedback-ID: 71780778:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 89067160014 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: y7jcf6943t734uaa9m7ysxtmh5x3yahn X-HE-Tag: 1711029387-220070 X-HE-Meta: U2FsdGVkX18LaBI+2Q+72dv4QgF6EqNn/AnKgBEyRiHt90vLVGrp2Ae6milsOZKnDYthvozZN/6cPkwmutbfc8baphdC3n8Rd/zlpXjTh3L/Jw1IRndUockBR0sc+j/inyexnrmAgqTNeltgEtUNAboB7Rtvcc90uBlmzxANmXMxmoWhRR5VvcJvSda1W03myYxYXJS6IrOlwT/QI0QBT86Ce8m5drFZ0WsKidtMlrULb1RQP7fkGekv1U3rzP3UXBztm6oeCBT546WZwrOyrsFAydF83SLam4o4TtaDcoNnyJ8zkN8XUA+HJpofSSghwrh1GiofKkwboznObsTWd6saYRrm7kmXRP62GCKSnZs03MhQJqsmbJMWTsB/Lq9luvlikyepaa3u0rQjdfmaNwtfAWjRDS6zwz9hgem3FXy6Eh6bTzjux07My5Ztp1N81adwE/chSdmSFuhU9TyrCK8wg2olQQTyk5zaeRhUQmHs1whmD/OKJyCberqTJS3ZDc2/Y/UmwKZCkNl1NNVP3yq87ybuKfMNinYUC2Ouk4r9k5PxjoYjlI+1NUolmdhKiSy9FE1d4fD3SwA22leypGw4wt6G2xrKq6l37mQDHZ7fCx3t1O9Dk1V06LKtTLTtnKhgVrcAl7fUoWv2mVyRxJllmIKlVTUhh0a7nC9MwO03XCFVqtaSdtv+fk+1UEg88NX6IYpOz3RndqkGhAX6+CyeFMoR9w9RuCPleyWWofleyBH93vq0eqfikYrtWov1XcYPTomuGvn1iv4GXHADJPc/EB+Lpc2DOUJe/b6K8aesK9QnIW6fg5e3xDy515QSow1shASX/WDb8RKAg/tX1t68Y9P5oGh21f5nQLuH4UskL52XEyeEcAgdb7Dwi08j559XnTzUifWcKQ+lEj5l8eqnqsyXoXZT5OQNiWtFwG077flAgnItEIsaFxEwQiYk17b47SUuVhzLP1Drz6M 2S0Yg72x nosiI+jsx2qG1VPUrJP/W9fK2/z5jG3u9IM7LYm5DI/hUj8HCzpgN6tHY7b2+4pBhjl0/CtPisAas3YT7SDEDTf/3dlRaw2QW3O9VfWNWraDcs8S6j7lrvtu/CvVpX1uo/P3c9yPu9jc2ltKr+CVrtjNTOCFQUL3JubbNHc0xoCzBlCYXzckYQ1o1Cx2h6bl885UMOyimrA/ZSNM4bUv3efoVQU4jLP5ALqyI+0jw5+W6JOVVRUWveWvo3dD+lu7Lrhx2y+knH9Ad3/UQHY5TRvXn/nIJrlay7/6/SMPJUg60hGJCgKwiQiNS5L+AmwNx3gWa 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 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 page. >>>> >>>> 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. >=20 > To me "points at a page" implies that the pointer is valid. I mean, if > it was dangling, it would not point at a page? >=20 > 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 corre= ctly. >>>> >>>> This says nothing about what 'correctly' means. What I gathered from t= he >>>> implementation is that the supplied pointer is valid for the execution >>>> 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 threads? >>>> Could that not result in the same page being mapped multiple times? If >>>> that is fine, what about potential data races when two threads write t= o >>>> 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 correctly. >>> /// The pointer is valid for `PAGE_SIZE` bytes and for the duration in >>> /// which the closure is called. Depending on the gfp flags and kernel >>> /// configuration, the pointer may only be mapped on the current thread= , >>> /// and in those cases, dereferencing it on other threads is UB. Other >>> /// than that, the usual rules for dereferencing a raw pointer apply. >>> /// (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. >=20 > Sure, that works for me. >=20 >> Also would it make sense to make the pointer type *mut [u8; PAGE_SIZE]? >=20 > 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() }); >> } >> }); >=20 > Like I said, the usual pointer rules apply. Two threads can access it > in parallel as long as one of the following are satisfied: >=20 > * Both accesses are reads. > * Both accesses are atomic. > * They access disjoint byte ranges. >=20 > 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. The p= ointer is valid for reads /// and writes for `PAGE_SIZE` bytes and for the duration in which the clos= ure is called. The /// pointer must only be used on the current thread. The caller must also e= nsure that no data races /// occur: when mapping the same page on two threads accesses to memory wit= h the same offset must be /// synchronized. >=20 >> If this is not allowed, I don't really like the API. As a raw version it >> would be fine, but I think we should have a safer version (eg by taking >> `&mut self`). >=20 > 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. --=20 Cheers, Benno