On 1/13/21 5:54 AM, Oscar Salvador wrote:
> 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.
Thanks for taking a look.
>
> 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?
Sure, we could do this. As noted, I simply patterned this after the
page flag routines in page-flags.h. If we go to single functions as
you suggest, I would perhaps change the name a bit to indicate the flags
were associated with the page. Invoking code comparison would be:
SetHPageRestoreReserve(page)
-vs-
hugetlb_set_pageflag(page, HP_Restore_Reserve)
In either case, code would be more readable and you can easily grep for
a specific flag.
If we do go with single functions as above, then they would certainly be
moved to hugetlb.h as some flags need to be accessed outside hugetlb.c.
Muchun has already suggested this movement.
--
Mike Kravetz
