Received-SPF: pass (google.com: domain of linux-kernel+bounces-105212-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sat, 16 Mar 2024 15:30:47 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Alice Ryhl <aliceryhl@google.com>
Cc: Andreas Hindborg <a.hindborg@samsung.com>, 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
Subject: Re: [PATCH v3 4/4] rust: add abstraction for `struct page`
Message-ID: <ZfW7J2uS28hlFWcd@casper.infradead.org>
References: <20240311-alice-mm-v3-4-cdf7b3a2049c@google.com>
 <20240311105056.122734-1-aliceryhl@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240311105056.122734-1-aliceryhl@google.com>

On Mon, Mar 11, 2024 at 10:50:56AM +0000, Alice Ryhl wrote:
> 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.

I think what you have here is good.

> 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?

Yes, kmap_local_page() works for non-highmem pages (it's essentially a
no-op)