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 6B029C54E58 for ; Wed, 20 Mar 2024 08:46:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 099A26B0082; Wed, 20 Mar 2024 04:46:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 04A3C6B0083; Wed, 20 Mar 2024 04:46:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E55BF6B0085; Wed, 20 Mar 2024 04:46:37 -0400 (EDT) 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 D4EE96B0082 for ; Wed, 20 Mar 2024 04:46:37 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 7B96E81318 for ; Wed, 20 Mar 2024 08:46:37 +0000 (UTC) X-FDA: 81916786434.20.D501BEA Received: from mail-yw1-f202.google.com (mail-yw1-f202.google.com [209.85.128.202]) by imf25.hostedemail.com (Postfix) with ESMTP id A7BACA001E for ; Wed, 20 Mar 2024 08:46:34 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=xj+eYHqm; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of 3aaL6ZQkKCM4u52wyBI1508805y.w86527EH-664Fuw4.8B0@flex--aliceryhl.bounces.google.com designates 209.85.128.202 as permitted sender) smtp.mailfrom=3aaL6ZQkKCM4u52wyBI1508805y.w86527EH-664Fuw4.8B0@flex--aliceryhl.bounces.google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1710924394; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=UK1n2ZUFasdNJS4tL/dUBZ9HzwBNAKoJUXbp2GIlPGA=; b=v9Dv4EbMMW6A8z3pv/YDbsEdHs1xzNzozC+HL2drj/e+oJjYRA8AJZxVnOSgJYOWTgK3XF m/VbXlm+44dNhNuKso/mUEFAFS5QQu1Y5140jQEnSbfg6MY8D81v8Z/OBDV0aAnTnN+4w1 xggPkRMEM+edWvcvr8eGQLfCYpvKri4= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=xj+eYHqm; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf25.hostedemail.com: domain of 3aaL6ZQkKCM4u52wyBI1508805y.w86527EH-664Fuw4.8B0@flex--aliceryhl.bounces.google.com designates 209.85.128.202 as permitted sender) smtp.mailfrom=3aaL6ZQkKCM4u52wyBI1508805y.w86527EH-664Fuw4.8B0@flex--aliceryhl.bounces.google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710924394; a=rsa-sha256; cv=none; b=ugrk0m9LoBY3iMwzYmFT4QBB3+n+NyHLeKDW3IBgbjtwX8f9R/fc/k+VM6sL66DabFqJRQ 9K/6V/ghLttS1l+2ipqf3fGondy51JO3QLGILbQNo3gxJLRchHkTHOc0ncCIkXN3EkPjDo 8WKstuc8UuKF+SsV+ky+8KEnn3Du1w8= Received: by mail-yw1-f202.google.com with SMTP id 00721157ae682-60cd041665bso111898787b3.0 for ; Wed, 20 Mar 2024 01:46:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1710924393; x=1711529193; darn=kvack.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=UK1n2ZUFasdNJS4tL/dUBZ9HzwBNAKoJUXbp2GIlPGA=; b=xj+eYHqmgOJbSTRQNRsNs3cnToRwTtZPNbP88cZC+nI/2M9t7oJDxLFAayk/AhKbsn AZ8oR5It6Bsshjl2WhZwYEDGECiPs1jaq1IuzK/IBz71X0xk8ofHKcJWgoPymav49G3e SlEhJIbbHAUDn7jmBlHBdmyfZ+9AfAuLK3Jf0ZtOBY3TdBcmU5EwzG6LoB8hwJxO/Dul BS5O3Zb2zQ1NgAuBsFRaNtsRj5E0mlJMxu9mkS7cUHA/GmVYRatJIoKmVTNRvxJy7/EL Ig+OrXEvMeKf88oSuhYjVx2ZTc59S5cr8thI51EMFVZnx2ujxtMtKFKDgcTxmqb2g4Yh 9Jyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710924393; x=1711529193; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=UK1n2ZUFasdNJS4tL/dUBZ9HzwBNAKoJUXbp2GIlPGA=; b=Dzl+S1qFNu8ZYijjgwYrZHlJv3mB1U1rpjbrplDovyiADnt6i6T+t+VUXwXRJM6aRX mfpUV+dRGjRfzlQ17QT5JCgvNuh6TWVMMi18H7eW9O64lkSUPLQTaTR9jMiMrbJcdW6D xDrbEI7zqsG4rWRDRMN5YALWayIV0/FM30m88RMwJiUTPPhEZKj2ETmc7oy9hDeH7k3n 7sCni/KOGIExgUzbpxtJLiLK4QSHY68MMWq7MGJsb6oeL59rZrrK2VyjJoJUjLkSqhqX bspV3hTDQZ2T5jyB92lU7qa3LxWa+kUzeRl24zg4nN0F3AEF8YKiStvCot5T+PCjjWb3 w+KQ== X-Forwarded-Encrypted: i=1; AJvYcCVuKc5Y9wzPN1/c807k4EGeeCst2CGq8AMa6aWPidkxAl9Lvbr8RfZ+FM3bk42EvFQWMtAxawFZPpdZT+Gv/Kc8LOI= X-Gm-Message-State: AOJu0YxaJE2s2Uj8zrEmYNyRoJ5GLlX13H+yDGPWEETmpms5vxvWMS8d 6wr/iUcPXnbC6SyKTIponSjRXW6P/7t+I6hwv4smh+QEGYYFjhiN2mOG67V8EIEiDzCDSbncvAu 4aU7mkW3fhgZq3g== X-Google-Smtp-Source: AGHT+IHhO69PTaN4boKOvVHCOJk+p5noJ7sGfzArQ8mOhKPuKZfczEJL2sh4BtJrZ5uOiOp76ApjmtaKNMUWfnM= X-Received: from aliceryhl2.c.googlers.com ([fda3:e722:ac3:cc00:68:949d:c0a8:572]) (user=aliceryhl job=sendgmr) by 2002:a05:6902:124b:b0:dc2:398d:a671 with SMTP id t11-20020a056902124b00b00dc2398da671mr4614173ybu.10.1710924393527; Wed, 20 Mar 2024 01:46:33 -0700 (PDT) Date: Wed, 20 Mar 2024 08:46:30 +0000 In-Reply-To: Mime-Version: 1.0 References: X-Mailer: git-send-email 2.44.0.291.gc1ea87d7ee-goog Message-ID: <20240320084630.2727355-1-aliceryhl@google.com> Subject: Re: [PATCH v3 4/4] rust: add abstraction for `struct page` From: Alice Ryhl To: benno.lossin@proton.me Cc: a.hindborg@samsung.com, akpm@linux-foundation.org, alex.gaynor@gmail.com, aliceryhl@google.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" X-Rspam-User: X-Stat-Signature: nscit1kr89ehhotuxxeixh6c1o6j5inq X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: A7BACA001E X-HE-Tag: 1710924394-417234 X-HE-Meta: U2FsdGVkX18w3gA204VtxnSLA9uOHsHppixYjj80phwXzIsFBSDeKV+Mku0gQw3sGZ/LeG2k3zrS07SRdIOXCMK53fFNx3lJGuwMor9ky+vSzG9UuoVJLLF09pNvKOgoth4u9JOTyVXkDOq9BOc3Wj9Ys66AFZEoLM/OwfQpn7qzq3sehnmshtJOr9BrnIG4NLrNzURpVHtpRUKccklZvK+50MX2xYBrtwpnKnteI1oIoIwo4oujwbYU9lQVgI5pvT1wMfILbtkIQivWErJri+Mkgim1x0KEw1h0t9WmVcwfVt2sbofrvF01MmMPV+nUHthYjjGOkAT9b4p/rlMWk/XuQ8RSbUsBweGSPiBI+UDn5tm3fAo10G/I6ARNorA/ArXWDHdS2XYrkhP6mRaJUcElCRuo/rA+wZEueSwEi+5IYU56HogpC8qdOXi7W7qqpuYoFvks/OlY5BQLvNuQAYFtmfUCLwKfMQ9/bt6UdZ5joYNf0eIM3n8rD+lVTau7WQ2EOkolxsTbbQos7V9eatqjfR6hU6M0h/HlljHR3RSPRmrtC3ZKhaU5XU04rw0wkvNoVTMttwP6Recf0Z1tazyZYd5vQ6U2iYMvkcaZ/nnp9RxJUjDy4+wr7vpT8ppsneRvAEhkHFQOLmcEEbWd0CYjDdKYMk7s+i0DYF2vksWrOKh8gHv2P3O1Y5koAu1JlUSGXfSS0xdQdDTmDtq8W4goSsAfbi2Nd4w6KJvYIn+ehh8Lglz/ElMdFadoND7PAbViGtYwFXDyxp0x96UV5RX5a0SllM0wd6JvlQb3QZFOhiqiBSitEexNuzA3iZx6ahOhBHIl7PhYNdjce4qdckfOwGi9Z3re03xSmJC/KQPHV+Uh+5EaySOtTglX4xGB7nerEppvkQ7fuHt1qgeFZIhy5WZLvvfa4bILxAUGxvFPyk8hxRU95vfawr7yeD4AvRYr1LLV8DzJRbsZ9Dm 01Gkg2y2 3go/OV2eWlS93WTZiBk7J1USWbEk8OHxsLGGcYimJPfKh9MMgIvjNvXWtV/T079Aay4H7euY5nPyFNCr9IY9NrCxPFMSeS9F1FlTwhd6uSCQwdtiJWxcuk4RXImJar9fSSOrFh7nlc+DrJ+lO6e7jz3Jxa+mZtV8p+wcW0tvY8cyC1KKm6y/+jU90NxzbErNESGg+cX/cHP/z2g4AZCUeqa8G+vZ11uviGqBuIQiRp81hIeifZSm1Wa/2Btco6dWg0vRlBD5fuwK9leSj0oCLmcdoGeLLYaWbGCu+1A65x+jlO/X3fn3QjKyywEkVkjGgB6JoWMI1NHbcI6Zdl2QWhQZbBFz985LQClRsKVhoqFQx4x+JCqnr5Hijvr28bGVPACq2nKtgII+nXkdpCPRw34bASGzg1xK7deBhopz2qlTfP6KALyRkds0erHcWEDOOHlAGL11tvB21UYnvp9Hyo9MsgFqKRL70X7+S5HXn73cfcfiCv1sjT1TSEfTwMf43Gnsbt32NaX5GnM25oygl0nz4mnyxQqcL6VsQMzoYtAU3gmWJ3C95wRHkb7JyYLv96AXxB5OuIzj0JPEKAUzDQp65TNjy7Vrhp5nUkAtidiSxqxlXkM2vDEvX9vkMWBuMYamNRTNnexu80QNLxzC4OvzTW3Ih5tMPT8W2/2ozhzBIwdZpAjWWLKqxvg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, 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/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". By ownership I mean that we are allowed to pass it to __free_page and that until we do, we can access the page. If you want me to reword this, please tell me what you want it to say. > > +// SAFETY: It is safe to transfer page allocations between threads. > > Why? > > > +unsafe impl Send for Page {} How about: // SAFETY: Pages have no logic that relies on them staying on a given // thread, so moving them across threads is safe. > > +// SAFETY: As long as the safety requirements for `&self` methods on this type > > +// are followed, there is no problem with calling them in parallel. > > Why? > > > +unsafe impl Sync for Page {} How about: // SAFETY: Pages have no logic that relies on them not being accessed // concurrently, so accessing them concurrently is safe. > > + // SAFETY: The specified order is zero and we want one page. > > This doesn't explain why it is sound to call the function. I expect that > it is always sound to call this function with valid arguments. > > > + let page = unsafe { bindings::alloc_pages(gfp_flags, 0) }; How about: // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. // Other than that, it is always safe to call this method. > > + // INVARIANT: We checked that the allocation succeeded. > > Doesn't mention ownership. > > > + Ok(Self { page }) How about: // INVARIANT: We just successfully allocated a page, so we now have // ownership of the newly allocated page. We transfer that ownership to // the new `Page` object. > > + /// 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 correctly. > > This says nothing about what 'correctly' means. What I gathered from the > 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 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 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.) It's okay to map it multiple times from different threads. > > + // SAFETY: This unmaps the page mapped above. > > This doesn't explain why it is sound. > > > + // > > + // Since this API takes the user code as a closure, it can only be used > > + // in a manner where the pages are unmapped in reverse order. This is as > > + // required by `kunmap_local`. > > + // > > + // In other words, if this call to `kunmap_local` happens when a > > + // different page should be unmapped first, then there must necessarily > > + // be a call to `kmap_local_page` other than the call just above in > > + // `with_page_mapped` that made that possible. In this case, it is the > > + // unsafe block that wraps that other call that is incorrect. > > + unsafe { bindings::kunmap_local(mapped_addr) }; Why do you say that? The kunmap_local method requires that the address being unmapped is currently mapped, and that pages are unmapped in reverse order. The safety comment explains that the page is currently mapped and that this method cannot be used to unmap them in anything other than reverse order. > > + /// Runs a piece of code with a raw pointer to a slice of this page, with > > + /// bounds checking. > > + /// > > + /// If `f` is called, then it will be called with a pointer that points at > > + /// `off` bytes into the page, and the pointer will be valid for at least > > + /// `len` bytes. The pointer is only valid on this task, as this method uses > > + /// a local mapping. > > This information about the pointer only being valid on this task should > also apply to `with_page_mapped`, right? > > > + /// > > + /// If `off` and `len` refers to a region outside of this page, then this > > + /// method returns `EINVAL` and does not call `f`. > > + /// > > + /// It is up to the caller to use the provided raw pointer correctly. > > Again, please specify what 'correctly' means. I will remove the "The pointer is only valid on this task, as this method uses a local mapping." sentence and copy the same paragraph as previously (without the `PAGE_SIZE` remark). Alice