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
