Re: [PATCH v3 4/4] rust: add abstraction for `struct page`

[Matthew Wilcox]

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