Received-SPF: pass (google.com: domain of linux-kernel+bounces-98722-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 11 Mar 2024 10:50:56 +0000
In-Reply-To: <20240311-alice-mm-v3-4-cdf7b3a2049c@google.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240311-alice-mm-v3-4-cdf7b3a2049c@google.com>
Message-ID: <20240311105056.122734-1-aliceryhl@google.com>
Subject: Re: [PATCH v3 4/4] rust: add abstraction for `struct page`
From: Alice Ryhl <aliceryhl@google.com>
To: Matthew Wilcox <willy@infradead.org>, Andreas Hindborg <a.hindborg@samsung.com>
Cc: akpm@linux-foundation.org, alex.gaynor@gmail.com, arnd@arndb.de, 
	arve@android.com, benno.lossin@proton.me, 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, aliceryhl@google.com
Content-Type: text/plain; charset="utf-8"

Alice Ryhl <aliceryhl@google.com> writes:
> +/// Flags for the "get free page" function that underlies all memory allocations.
> +pub mod flags {
> +    pub type gfp_t = bindings::gfp_t;
> +
> +    /// `GFP_KERNEL` is typical for kernel-internal allocations. The caller requires `ZONE_NORMAL`
> +    /// or a lower zone for direct access but can direct reclaim.
> +    pub const GFP_KERNEL: gfp_t = bindings::GFP_KERNEL;
> +    /// `GFP_ZERO` returns a zeroed page on success.
> +    pub const __GFP_ZERO: gfp_t = bindings::__GFP_ZERO;
> +    /// `GFP_HIGHMEM` indicates that the allocated memory may be located in high memory.
> +    pub const __GFP_HIGHMEM: gfp_t = bindings::__GFP_HIGHMEM;
> +}
>
> [...]
>
> +impl Page {
> +    /// Allocates a new page.
> +    pub fn alloc_page(gfp_flags: flags::gfp_t) -> Result<Self, AllocError> {
> +        // SAFETY: The specified order is zero and we want one page.
> +        let page = unsafe { bindings::alloc_pages(gfp_flags, 0) };
> +        let page = NonNull::new(page).ok_or(AllocError)?;
> +        // INVARIANT: We checked that the allocation succeeded.
> +        Ok(Self { page })
> +    }

Matthew Wilcox: You suggested on a previous version that I use gfp flags
here, or that I rename it to e.g. BinderPage to make it clear that this
is specific to the kind of pages that Binder needs.

In this version I added some gfp flags, but I'm not actually sure that
the Page abstraction works for all combinations of gfp flags. For
example, I use kmap_local_page when accessing the page, but is that
correct if there's a user that doesn't pass GFP_HIGHMEM?

So perhaps it should be called HighmemPage since the methods on it
hardcode that. Or maybe it really doesn't make sense to generalize it
beyond what Binder needs.

What do you think? How broadly does these implementations generalize? I
would be happy to hear your advice on this.


Andreas Hindborg: I recall you mentioning that you also needed an
abstraction for pages. To what extent do these abstractions fit your
needs? Which gfp flags do you need?


Also, sorry for taking so long to submit this version. I spent a long
time debugging the crash that led to the submission of [1].

Alice

[1]: https://lore.kernel.org/rust-for-linux/20240305-shadow-call-stack-v2-1-c7b4a3f4d616@google.com/