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

[Trevor Gross <tmgross@umich.edu>]

On Tue, Apr 16, 2024 at 5:53 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> +/// Flags for the "get free page" function that underlies all memory allocations.
> >> +pub mod flags {
> >> +    /// gfp flags.
> >
> > Uppercase acronym, maybe with a description:
> >
> >     GFP (Get Free Page) flags.
> >
> >> +    #[allow(non_camel_case_types)]
> >> +    pub type gfp_t = bindings::gfp_t;
> >
> > Why not GfpFlags, do we do this elsewhere?
> >
> >> +    /// `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;
> >
> > It feels a bit weird to have dunder constants on the rust side that
> > aren't also `#[doc(hidden)]` or just nonpublic. Makes me think they
> > are an implementation detail or not really meant to be used - could
> > you update the docs if this is the case?
>
> All of this is going away in the next version because it will be based
> on [1], which defines the gfp flags type for us.
>
> [1]: https://lore.kernel.org/rust-for-linux/20240328013603.206764-1-wedsonaf@gmail.com/

Great, thanks for the link.

> > Could you add an example of how to use this correctly?
>
> This is a private function, you're not supposed to use it directly.
> Anyone who is modifying this file directly can look at the existing
> users for examples.

Ah you're right, missed this bit.

Thanks for the followup.

Trevor