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