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 23AA6C54E58 for ; Thu, 21 Mar 2024 14:11:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4B39B6B0082; Thu, 21 Mar 2024 10:11:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 43B736B0083; Thu, 21 Mar 2024 10:11:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2B5036B0088; Thu, 21 Mar 2024 10:11:50 -0400 (EDT) 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 166AB6B0082 for ; Thu, 21 Mar 2024 10:11:50 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 8F39C1611CA for ; Thu, 21 Mar 2024 14:11:49 +0000 (UTC) X-FDA: 81921234738.08.0E58329 Received: from mail-qt1-f170.google.com (mail-qt1-f170.google.com [209.85.160.170]) by imf02.hostedemail.com (Postfix) with ESMTP id 9C57780011 for ; Thu, 21 Mar 2024 14:11:47 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=q+omwexk; spf=pass (imf02.hostedemail.com: domain of aliceryhl@google.com designates 209.85.160.170 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=1711030307; a=rsa-sha256; cv=none; b=cjps6Ih7r/g3jf/25BOfIp8ZcdR6xFSbhRstu+MNGbrAi2ZqISZt7ETOcilh5yDIzPa8i0 Ylu1n3WgENDUs1h0i8JYWvApHRTktGQ5Oko90rzFqU1ARxagc0OqtRSbNZGbXra0xlRLAE lv4cAEnuRSWi3E7Cze2jOmTeRW1g5wo= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=q+omwexk; spf=pass (imf02.hostedemail.com: domain of aliceryhl@google.com designates 209.85.160.170 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=1711030307; 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=ck4TkMLqNJOKAoGnoaiXGuXYDSdMyWKTEEYgwReH5mM=; b=UmbJFNyIOREFyRVl8aV6crpeu/3RMcEN7GieoKPWZ11QwSxqFl13wUiU1/zwuirOPk1Bvy 3+zihQmWGz3fAs+PvmA3kM6huDzFj+pEpl87paChH8UECnp+eJp8ODmOlqHjAPOag5guY1 3M26+2PuQMsILnEk3+TZLIBdFcX2ReE= Received: by mail-qt1-f170.google.com with SMTP id d75a77b69052e-430baec7bb5so18972291cf.0 for ; Thu, 21 Mar 2024 07:11:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1711030306; x=1711635106; 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=ck4TkMLqNJOKAoGnoaiXGuXYDSdMyWKTEEYgwReH5mM=; b=q+omwexk8RO/P3iQ6mrE1giLAZOeKeikgEjIa4AfnB7oKLcYSCX7ZDVpDUhQ3yThhc TTX7Ks40095eI4SsKxjRRCkL6yDe27WNYXOocij2oXTKA4SV9Y6wEjmjYNR7GzEb6seR JKazs/t3C+iGTGLirsNGYeRAlAScZvvWHHGd1EhWbOS8Ln82+IJZoqFrLQPhcdW1iQK1 ASnWKxucbSZhpuOfVW9N4/f7tF7XLq9c7aUbnZKNSNmQILdkaT4HNvR3jRLeFcDY/F2s 0RD8aang+VX7RWnzjhcWuvOR/+E1QDrArOVESy9/w9I+xacepM61yU4JmCBAUVA5lebf puFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711030306; x=1711635106; 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=ck4TkMLqNJOKAoGnoaiXGuXYDSdMyWKTEEYgwReH5mM=; b=Z7ilKGAgKG4rR5nLl3BavAktLAbZHjvgEwBnQ1l9WYkEfN521nba1xHyh2+A1U9wme R1XE/M0yotuct9k6N+fSHxsMG5nuFygk5Bajpd2LbNTaI9+ts3T+RRtfxxCkIFCBn65e AizBlrf2XRDSbM/MotFozPDxBAsjkZoXcJ9Z7AZigJeDp9cP+RxRKNGXMurv/JeXxgtk mgl/oF2ZNUp/Uel7LMF0G5Cuw5dCI/57qV/3VBWYDz2WcBx4y7/Z6a8M2YbmdSHsGr9E DC+w5q6P1xps/XPJ5g6C/C3DRA1Xmlgq7ITFfR+D4kWbYlQG+B3mbymtll9Eq73ewLSy WnXw== X-Forwarded-Encrypted: i=1; AJvYcCWLSmZtzodz/mYorSmpBC3x8HoJVLIZhvgoaPsPnD95DiW9bRvkPQFEucpNnE5sEuUHEhbmPFo90e2qs4xxYf9tv30= X-Gm-Message-State: AOJu0YzTV3X5msoIB1aBl3pr/pgK53u6cJjZwGj80kI5MWgbBVouzlW+ WBB4+Q/cOJkDm2Q60+VxjIa1ez0KLG7Ib1F+whDMxJgTPNGRRJg3sTs84QR330C/IcdTboGgsDh 0EMjQ7+3qOBqv/0+hELtehMhu1uZlEXCURr+S X-Google-Smtp-Source: AGHT+IEJGECJT5cE+zG3WClI4hruLgbFNuhTreSMkwaj1+QR6Jyy2aFP6npQQ2UlYF+85K169vu4856UO9uV7AucCO4= X-Received: by 2002:a05:620a:4509:b0:78a:2d35:3071 with SMTP id t9-20020a05620a450900b0078a2d353071mr3931578qkp.38.1711030306353; Thu, 21 Mar 2024 07:11:46 -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:11:35 +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-Server: rspam08 X-Rspamd-Queue-Id: 9C57780011 X-Stat-Signature: cxpgh5k9agzz4b4fpn9aujpc4b6ua8dw X-Rspam-User: X-HE-Tag: 1711030307-626117 X-HE-Meta: U2FsdGVkX1/JoG/Mskks3DEVNtXZipkGgNKnL4ASMqk9LdZouk/BCkVNIn0CeV6CP1z2e37X9CnrflKUMc1mTcI4mNyw7W2CnAU9QIV7L7Kh6W5H2ljJvRcIgHsyCxmqA5bNYpP8bpyED5FT+0EGhCC3i2RlFNEUh6EwLzlV9QB1ZRwY/9b+WHdfqXH5s4uRnHx1UOok7E+9N6RlquqETkd6c/3yLlcLCwbnwsTPUyHkhouKlYf4n79MdCSqArEn5SJ1MSoOYFmx3w6REdsTjpKWR9gnmXWs9O10Xg0kzK+gCLnw2F3oX4qJvoSJqwIecoDtxyb7UQFSZPBq+Cidfj3rGRi7YCAG4U3l8uy70ow2c066SviYZFdBon2e5+Iwm9FCg4kQnfYyh501Ur6w3ZDeL6pXLpZxqWjjK6gGbLhhjGW8mnb9S3fmsFBGm0lgYQRLLWsnUQWO5wwLPY+2v00YdRM0Jr1c+fjZbqpL0+UQJB0Bu3P7XMMUltHN6vF826PYAbektOEnDqi9R6tOI7Sr8OumbJhuXktDK8aqFNDkxGyBHyhIEC9eztuI6eC8bFOQWlvA3L3/vWhnyzPUUXH2XdtTmDlrXARY+BDkJwnZuEgdvVYGZ5VePK06zHXnNR2Ss1n0JAcLo0Jd5jMLwTx0hONI5dADU/2GfVLHtcyT7vlxy+qYURXqHNpaPCOAAM2EhVErygPBBdhCe4IxUAn3lUzSuPaKfsK29lh9qukPCGCPg/3iJZzAJhlH+Q78ANa3EGLilQppof75XXlKAc+BM/NWUNy4E0x60Y4XSUCPnJv2UrZePcoN2l/FPSztc1jVLhhCzxmtrZwq3GclxUnbtou9WFLHV1aI5RoOXWB4V7svbA6B5U1GIpCQudzEtfAlOrqtHJC2bYqWdPkxp1XuOhgaoHS0U4rkSrSMbRRVqY+7HsU3nGEHwobqQYHHVCXoYWAGSUy0QonnU/K 6Hc4yYh/ jez19QI2V4ELi3KnTlNSB3wDXD9xTIxqRm8VRQAWVSl+mzOt23vOZ1nGnoa4sFxyv/W4yxDcrYGYxLTdrJ2ycSgaRv134M0005NnwHlFQPl7mqQ+zLS0lm90PMKB60miRKGVmSKtg3PldE8Zv7jAtvX2P5LKeMS+OLkTHiIfp56ml3YCIxbHfTCSH2zzJ+Ys2EoNVFxG/t3VwxrL06wKx/+iPSqABbh6/JOCm8cTHdmYsVTadd/nwgqmpzu7sW8okAse9K2ohzpTB19OcfTx26KvJi2+3Szn9P6o/xJDkX1yGsM0= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000217, 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 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 page. > >>>> > >>>> Why not "`page` is valid"? > >>>> Do you mean by ownership of the page that `page` has ownership of th= e > >>>> 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, if > > 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 cor= rectly. > >>>> > >>>> This says nothing about what 'correctly' means. What I gathered from= the > >>>> implementation is that the supplied pointer is valid for the executi= on > >>>> 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= 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 correctly. > >>> /// The pointer is valid for `PAGE_SIZE` bytes and for the duration i= n > >>> /// which the closure is called. Depending on the gfp flags and kerne= l > >>> /// configuration, the pointer may only be mapped on the current thre= ad, > >>> /// and in those cases, dereferencing it on other threads is UB. Othe= r > >>> /// than that, the usual rules for dereferencing a raw pointer apply. > >>> /// (E.g., don't cause data races, the memory may be uninitialized, a= nd > >>> /// so on.) > >> > >> I would simplify and drop "depending on the gfp flags and kernel..." a= nd > >> 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_SIZE]= ? > > > > 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 b= y 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. The= pointer is valid for reads > /// and writes for `PAGE_SIZE` bytes and for the duration in which the cl= osure is called. The > /// pointer must only be used on the current thread. The caller must also= ensure that no data races > /// occur: when mapping the same page on two threads accesses to memory w= ith 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 version = it > >> would be fine, but I think we should have a safer version (eg by takin= g > >> `&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. Alice