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 5C99DC47DB3 for ; Mon, 29 Jan 2024 18:00:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DF9536B0093; Mon, 29 Jan 2024 13:00:09 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DA9B76B0095; Mon, 29 Jan 2024 13:00:09 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C993A6B0096; Mon, 29 Jan 2024 13:00:09 -0500 (EST) 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 B8D296B0093 for ; Mon, 29 Jan 2024 13:00:09 -0500 (EST) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 4F7E81A0592 for ; Mon, 29 Jan 2024 18:00:09 +0000 (UTC) X-FDA: 81733112538.19.E349F8E Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf16.hostedemail.com (Postfix) with ESMTP id C84ED180004 for ; Mon, 29 Jan 2024 18:00:06 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=Z6sg7Flo; dmarc=none; spf=none (imf16.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706551207; 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=nUaCpWIQKZnRWHaoO9ygLf4yYLLy6m9z7KXvSuxXHYs=; b=Fi+NOTbRf6+jSpPjMQGJZ1iF3ZitH2Nqswz2Vv2jSgfxCYvGqPEh81ExXQSsnEPDP5pG7d eMfsxv7qzyJeWJlMWu7eSyN0Ls7a5Nh7iuJgINbJNDKXdsf5a9wMSRpc2BFAeirZoOQbtz 6RkbL3BlnyRCjw7KbQr7XdAikY1y7O4= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=Z6sg7Flo; dmarc=none; spf=none (imf16.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706551207; a=rsa-sha256; cv=none; b=EWc+RVO8YCcC2tlofu1iOothgYT10RKZx+2aN/Kd5hm/ZKy7EwAGewvj9qcuvMBLbKE7VG IwfnNWJPdzuBRfHX8MDESd/ZD4e4jNVkaMlLJlfqprcM/wbtyASS+m/rW6HKacyu7Z1EPr EQQJ+PegNGMPRPSltKrBU0n8sANIPxM= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=nUaCpWIQKZnRWHaoO9ygLf4yYLLy6m9z7KXvSuxXHYs=; b=Z6sg7FloIK7TT9Uz6gOs/cXEGW MMhZ3DDDlapkaIxgXoEcD+P/f28ZbpQ7wILt7c9kRuRbNeVVrc/047UE5fRz11/o1Lj/gldY7Pghq 8rjcuvNZqxzRuvUgW2TuCb1haYVv2twq4AB0gntHlUhuI1KjLN6F9Stg7lotDcLOm/e5WSDFGI3N1 93wMVUNYt0f1334r0l3wgENnU21gMFliEVVVWGGyY+rW8HfY+qDHDthxTN4Iqp/naKq+LhkehNgS6 4MVS55mImQKFWR59qTlAR/ArUf2tk2cQb1u4XkJ8yTpWnh5QKjBBFFoNePYHxMUlG/YVHIJZKz6KD KTCfELAA==; Received: from willy by casper.infradead.org with local (Exim 4.97.1 #2 (Red Hat Linux)) id 1rUVvF-00000007KGd-3pzC; Mon, 29 Jan 2024 17:59:53 +0000 Date: Mon, 29 Jan 2024 17:59:53 +0000 From: Matthew Wilcox To: Alice Ryhl Cc: Miguel Ojeda , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?iso-8859-1?Q?Bj=F6rn?= Roy Baron , Benno Lossin , Andreas Hindborg , Kees Cook , Al Viro , Andrew Morton , Greg Kroah-Hartman , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , 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 Subject: Re: [PATCH 3/3] rust: add abstraction for `struct page` Message-ID: References: <20240124-alice-mm-v1-0-d1abcec83c44@google.com> <20240124-alice-mm-v1-3-d1abcec83c44@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240124-alice-mm-v1-3-d1abcec83c44@google.com> X-Rspamd-Queue-Id: C84ED180004 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: 99kq3ciy8nwu7jpk5edgtf8mg7353s17 X-HE-Tag: 1706551206-112256 X-HE-Meta: U2FsdGVkX182lpJkn73JrCLGCGwa8jaJ66IWsgiD+LlCEBt4QJsxnkqSouQseCj0bmCsVx0GVqlUNTUCppFkfatYDwfoy5hoEahCEt3cg9iu6rLGMG5Trh5s0bhg0qEf9ZDPJY6SaeDJ0bDdiOLkmwAXmNI+bccFOdNuexGjRC35rKAsTBgxz2d4g4ubTRe+npd/6Mv2SnDLra6FHK/TLD1r8n15iOWvTX8N/03ytHGePM+aO+Af/Mp7DYGtBs10wwkwwbPpBvhJnm5Vbv+mUzyM+8bCWpduGhBBzvK/O6OBxYtUvMp2RorIlO5yACUl7kNsSM/8Hmn8nYq1bye1VJfaFs2mBUvhybMd6E45Fftf43q1l7aDSmdQthAobxnwyXmRZhKG0XkmVFDWpSQBcv3cIAUzmVlJGOX3PJGkAK+H78QkpkKnUiD6bm9UazeA+C+zna/aSN0bv/lMTBBpgtSiJ5Z8W87yuApHWhnd/LYjQooXAf3DplTUijjkw6jl7PHpaYqqYtG0k97oByrzDq5nbyWl664fAeheDO+IZkUQnNI6FTyDIoOzKxJoMRKxWIxdytmRzCrwyAzxNeQD6QJ7TrTahjwFe+ExsuuEM10/wFkSYbM0xSxiMNThFA9xNuMvxQdera4bT3NbMWODPXxTw2jyz1ksZ3T0SIxdrb/iIqwc1Um8VrCx+hNlIv3zDVHaGFbfrHWfUQVXkH8K8zwrG+9HrNUVPZ63Zwbie8JJUQQuEOvB+pzowiQFQ6oOjPRE7ZCPLhhUzXsi9zuqb7uoImo+Wkyp/XZnep9DgI/7SxbHE44DRwwkksMjjneJZYP80jwXyqC6ONBXS+JWaE7qSKH/Q7ADAFVO/6aO59Tdpdi5jPWOm/1K1sgZIGo2BH3RM9ayKiPYB0I35Y7ZC4+CKI6KTFzRoRv1AT6Z6pqRU47WlO3aeBg8iWmQFaAjWZKyH/C73VLdwntPZBW yDF5YqLQ +qWMjBnX6wRFBnjxCWP+p9Gk2VkUR4YR6QI91gUN4Hqe7tD7Dm9In7c7l/LGniPOw2Fx8SKcoqDxcIRzFPxaEDqZuiZGqm3Qmjk1UfOYM3S5x+DD+lOfnXXBWOYg+jczRKrPmwPOx2zVa05jZAdbEFncnBbVcrVqzN4GnXKbin+Opxi4ffm7Y3iC7fmxdjzRY+/t/J7XsKgt16aO5P//6iIxUNw== 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 Wed, Jan 24, 2024 at 11:20:23AM +0000, Alice Ryhl wrote: > Adds a new struct called `Page` that wraps a pointer to `struct page`. > This struct is assumed to hold ownership over the page, so that Rust > code can allocate and manage pages directly. OK ... > This patch only adds support for pages of order zero, as that is all > Rust Binder needs. However, it is written to make it easy to add support > for higher-order pages in the future. To do that, you would add a const > generic parameter to `Page` that specifies the order. Most of the > methods do not need to be adjusted, as the logic for dealing with > mapping multiple pages at once can be isolated to just the > `with_pointer_into_page` method. Finally, the struct can be renamed to > `Pages`, and the type alias `Page = Pages<0>` can be introduced. This description concerns me because it reads like you're not keeping up with the current thinking in MM about what pages are and how we're improving the type hierarchy. As in, we're creating one instead of allowing the current mish-mash of absolutely everything to continue. Are you the right person to ask about the operations that Binder does with a page so we can figure out where it fits in the type hierarchy? > Rust Binder needs to manage pages directly as that is how transactions > are delivered: Each process has an mmap'd region for incoming > transactions. When an incoming transaction arrives, the Binder driver > will choose a region in the mmap, allocate and map the relevant pages > manually, and copy the incoming transaction directly into the page. This > architecture allows the driver to copy transactions directly from the > address space of one process to another, without an intermediate copy > to a kernel buffer. Everything about this says "This is what a first year comp sci student thinks will be fast". Oh well, the thinking here isn't your fault. > @@ -127,6 +129,24 @@ int rust_helper_signal_pending(struct task_struct *t) > } > EXPORT_SYMBOL_GPL(rust_helper_signal_pending); > > +struct page *rust_helper_alloc_pages(gfp_t gfp_mask, unsigned int order) > +{ > + return alloc_pages(gfp_mask, order); > +} > +EXPORT_SYMBOL_GPL(rust_helper_alloc_pages); > + > +void *rust_helper_kmap_local_page(struct page *page) > +{ > + return kmap_local_page(page); > +} > +EXPORT_SYMBOL_GPL(rust_helper_kmap_local_page); > + > +void rust_helper_kunmap_local(const void *addr) > +{ > + kunmap_local(addr); > +} > +EXPORT_SYMBOL_GPL(rust_helper_kunmap_local); I remain opposed to all these fidgetty little helpers. Particularly when they're noops on machines without HIGHMEM, which is ~all of them. > +/// A bitwise shift for the page size. > +pub const PAGE_SHIFT: usize = bindings::PAGE_SHIFT as usize; Does PAGE_SHIFT really need to be as large as 'usize'? If it's more than 63 by the time I retire, I'll be shocked. If it's more than 127 by the time I die, I'll be even more shocked. And it won't get to 255 by the heat death of the universe. > +/// The number of bytes in a page. > +pub const PAGE_SIZE: usize = 1 << PAGE_SHIFT; This is appropriately usize. > +/// A bitwise mask for the page size. > +pub const PAGE_MASK: usize = PAGE_SIZE - 1; Are you trying to get somebody killed? include/asm-generic/page.h:#define PAGE_MASK (~(PAGE_SIZE-1)) Defining PAGE_MASK to be the opposite set of bits in C and Rust is going to bite us all day every day for a decade. > +impl Page { > + /// Allocates a new set of contiguous pages. > + pub fn new() -> Result { > + // SAFETY: These are the correct arguments to allocate a single page. > + let page = unsafe { > + bindings::alloc_pages( > + bindings::GFP_KERNEL | bindings::__GFP_ZERO | bindings::__GFP_HIGHMEM, > + 0, > + ) > + }; This feels too Binder-specific to be 'Page'. Pages are not necessarily allocated with GFP_HIGHMEM, nor are they necessarily zeroed. Maybe you want a BinderPage type?