Messages in this thread | | | Date | Wed, 13 Jan 2021 14:54:44 +0100 | From | Oscar Salvador <> | Subject | Re: [RFC PATCH 1/3] hugetlb: use page.private for hugetlb specific page flags |
| |
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
| |