Re: [RFC PATCH 1/3] hugetlb: use page.private for hugetlb specific page flags

[Oscar Salvador <osalvador@suse.de>]

On Mon, Jan 11, 2021 at 01:01:50PM -0800, Mike Kravetz wrote:
> As hugetlbfs evolved, state information about hugetlb pages was added.
> One 'convenient' way of doing this was to use available fields in tail
> pages.  Over time, it has become difficult to know the meaning or contents
> of fields simply be looking at a small bit of code.  Sometimes, the
> naming is just confusing.  For example: The PagePrivate flag indicates
> a huge page reservation was consumed and needs to be restored if an error
> is encountered and the page is freed before it is instantiated.  The
> page.private field contains the pointer to a subpool if the page is
> associated with one.
> 
> In an effort to make the code more readable, use page.private to contain
> hugetlb specific flags.  These flags will have test, set and clear functions
> similar to those used for 'normal' page flags.  More importantly, the
> flags will have names which actually reflect their purpose.
> 
> In this patch,
> - Create infrastructure for huge page flag functions
> - Move subpool pointer to page[1].private to make way for flags
>   Create routines with meaningful names to modify subpool field
> - Use new HPageRestoreReserve reserve flag instead of PagePrivate
> 
> Conversion of other state information will happen in subsequent patches.

I like this idea, it would make the code much easier to follow, and together
with Muchun's gathering indiscrete index hugetlb code will start looking less
scarier.

I do have a question below:

> +enum htlb_page_flags {
> +	HPAGE_RestoreReserve = 0,
> +};
> +
> +/*
> + * Macros to create function definitions for hpage flags
> + */
> +#define TESTHPAGEFLAG(flname)					\
> +static inline int HPage##flname(struct page *page)		\
> +	{ return test_bit(HPAGE_##flname, &(page->private)); }
> +
> +#define SETHPAGEFLAG(flname)					\
> +static inline void SetHPage##flname(struct page *page)		\
> +	{ set_bit(HPAGE_##flname, &(page->private)); }
> +
> +#define CLEARHPAGEFLAG(flname)					\
> +static inline void ClearHPage##flname(struct page *page)	\
> +	{ clear_bit(HPAGE_##flname, &(page->private)); }
> +
> +#define HPAGEFLAG(flname)					\
> +	TESTHPAGEFLAG(flname)					\
> +	SETHPAGEFLAG(flname)					\
> +	CLEARHPAGEFLAG(flname)
> +
> +HPAGEFLAG(RestoreReserve)

I have mixed feelings about this.
Could we have a single function that sets/clears the bit/flag?
e.g:

 static inline void hugetlb_set_flag(struct page *p, page_flag)
 {
         set_bit(flag, &(page->private));
 }

etc.
It would look less of an overkill?
 
-- 
Oscar Salvador
SUSE L3