[PATCH v3 0/5] create hugetlb flags to consolidate state

[PATCH v3 0/5] create hugetlb flags to consolidate state

[Mike Kravetz]

While discussing a series of hugetlb fixes in [1], it became evident
that the hugetlb specific page state information is stored in a somewhat
haphazard manner.  Code dealing with state information would be easier
to read, understand and maintain if this information was stored in a
consistent manner.

This series uses page.private of the hugetlb head page for storing a
set of hugetlb specific page flags.  Routines are priovided for test,
set and clear of the flags.

[1] https://lore.kernel.org/r/20210106084739.63318-1-songmuchun@bytedance.com

v2 -> v3
  Fixed !CONFIG_HUGETLB_PAGE build bug and moved BUILD_BUG_ON to
  hugetlb_init (Oscar)
  Enhanced patch 3 (HPageTemporary flag) commit message (Oscar)
  Fixed typo enhanced coment in scan_movable_pages (Miaohe)
Patch -> v2
  Went back to functions similar to 'normal' page flags (Matthew/Muchun)
  Decided to leave check in unmap_and_move_huge_page and print warning
RFC -> PATCH
  Simplified to use a single set of flag manipulation routines (Oscar)
  Moved flags and routines to hugetlb.h (Muchun)
  Changed format of page flag names (Muchun)
  Changed subpool routine names (Matthew)
  More comments in code (Oscar)

Based on v5.11-rc4-mmotm-2021-01-21-20-07

Mike Kravetz (5):
  hugetlb: use page.private for hugetlb specific page flags
  hugetlb: convert page_huge_active() HPageMigratable flag
  hugetlb: only set HPageMigratable for migratable hstates
  hugetlb: convert PageHugeTemporary() to HPageTemporary flag
  hugetlb: convert PageHugeFreed to HPageFreed flag

 fs/hugetlbfs/inode.c       |  14 +---
 include/linux/hugetlb.h    |  91 ++++++++++++++++++++++
 include/linux/page-flags.h |   6 --
 mm/hugetlb.c               | 152 ++++++++++++-------------------------
 mm/memory_hotplug.c        |   9 ++-
 mm/migrate.c               |   9 +--
 6 files changed, 154 insertions(+), 127 deletions(-)

-- 
2.29.2

[PATCH v3 1/5] hugetlb: use page.private for hugetlb specific page flags

[Mike Kravetz]

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 by 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 page flags.  These flags will have test, set and clear
functions similar to those used for 'normal' page flags.  More importantly,
an enum of flag values will be created with names that actually reflect
their purpose.

In this patch,
- Create infrastructure for hugetlb specific 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 flag instead of PagePrivate

Conversion of other state information will happen in subsequent patches.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c    | 12 ++------
 include/linux/hugetlb.h | 68 +++++++++++++++++++++++++++++++++++++++++
 mm/hugetlb.c            | 48 +++++++++++++++--------------
 3 files changed, 96 insertions(+), 32 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 21c20fd5f9ee..b00801fd6002 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -966,15 +966,9 @@ static int hugetlbfs_migrate_page(struct address_space *mapping,
 	if (rc != MIGRATEPAGE_SUCCESS)
 		return rc;
 
-	/*
-	 * page_private is subpool pointer in hugetlb pages.  Transfer to
-	 * new page.  PagePrivate is not associated with page_private for
-	 * hugetlb pages and can not be set here as only page_huge_active
-	 * pages can be migrated.
-	 */
-	if (page_private(page)) {
-		set_page_private(newpage, page_private(page));
-		set_page_private(page, 0);
+	if (hugetlb_page_subpool(page)) {
+		hugetlb_set_page_subpool(newpage, hugetlb_page_subpool(page));
+		hugetlb_set_page_subpool(page, NULL);
 	}
 
 	if (mode != MIGRATE_SYNC_NO_COPY)
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index b5807f23caf8..a7eb05315c6e 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -472,6 +472,60 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
 					unsigned long flags);
 #endif /* HAVE_ARCH_HUGETLB_UNMAPPED_AREA */
 
+/*
+ * huegtlb page specific state flags.  These flags are located in page.private
+ * of the hugetlb head page.  Functions created via the below macros should be
+ * used to manipulate these flags.
+ *
+ * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at
+ *	allocation time.  Cleared when page is fully instantiated.  Free
+ *	routine checks flag to restore a reservation on error paths.
+ */
+enum hugetlb_page_flags {
+	HPG_restore_reserve = 0,
+	__NR_HPAGEFLAGS,
+};
+
+/*
+ * Macros to create test, set and clear function definitions for
+ * hugetlb specific page flags.
+ */
+#ifdef CONFIG_HUGETLB_PAGE
+#define TESTHPAGEFLAG(uname, flname)				\
+static inline int HPage##uname(struct page *page)		\
+	{ return test_bit(HPG_##flname, &(page->private)); }
+
+#define SETHPAGEFLAG(uname, flname)				\
+static inline void SetHPage##uname(struct page *page)		\
+	{ set_bit(HPG_##flname, &(page->private)); }
+
+#define CLEARHPAGEFLAG(uname, flname)				\
+static inline void ClearHPage##uname(struct page *page)		\
+	{ clear_bit(HPG_##flname, &(page->private)); }
+#else
+#define TESTHPAGEFLAG(uname, flname)				\
+static inline int HPage##uname(struct page *page)		\
+	{ return 0; }
+
+#define SETHPAGEFLAG(uname, flname)				\
+static inline void SetHPage##uname(struct page *page)		\
+	{ }
+
+#define CLEARHPAGEFLAG(uname, flname)				\
+static inline void ClearHPage##uname(struct page *page)		\
+	{ }
+#endif
+
+#define HPAGEFLAG(uname, flname)				\
+	TESTHPAGEFLAG(uname, flname)				\
+	SETHPAGEFLAG(uname, flname)				\
+	CLEARHPAGEFLAG(uname, flname)				\
+
+/*
+ * Create functions associated with hugetlb page flags
+ */
+HPAGEFLAG(RestoreReserve, restore_reserve)
+
 #ifdef CONFIG_HUGETLB_PAGE
 
 #define HSTATE_NAME_LEN 32
@@ -531,6 +585,20 @@ extern unsigned int default_hstate_idx;
 
 #define default_hstate (hstates[default_hstate_idx])
 
+/*
+ * hugetlb page subpool pointer located in hpage[1].private
+ */
+static inline struct hugepage_subpool *hugetlb_page_subpool(struct page *hpage)
+{
+	return (struct hugepage_subpool *)(hpage+1)->private;
+}
+
+static inline void hugetlb_set_page_subpool(struct page *hpage,
+					struct hugepage_subpool *subpool)
+{
+	set_page_private(hpage+1, (unsigned long)subpool);
+}
+
 static inline struct hstate *hstate_file(struct file *f)
 {
 	return hstate_inode(file_inode(f));
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d78111f0fa2c..8ec6138ca81b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1133,7 +1133,7 @@ static struct page *dequeue_huge_page_vma(struct hstate *h,
 	nid = huge_node(vma, address, gfp_mask, &mpol, &nodemask);
 	page = dequeue_huge_page_nodemask(h, gfp_mask, nid, nodemask);
 	if (page && !avoid_reserve && vma_has_reserves(vma, chg)) {
-		SetPagePrivate(page);
+		SetHPageRestoreReserve(page);
 		h->resv_huge_pages--;
 	}
 
@@ -1407,20 +1407,19 @@ static void __free_huge_page(struct page *page)
 	 */
 	struct hstate *h = page_hstate(page);
 	int nid = page_to_nid(page);
-	struct hugepage_subpool *spool =
-		(struct hugepage_subpool *)page_private(page);
+	struct hugepage_subpool *spool = hugetlb_page_subpool(page);
 	bool restore_reserve;
 
 	VM_BUG_ON_PAGE(page_count(page), page);
 	VM_BUG_ON_PAGE(page_mapcount(page), page);
 
-	set_page_private(page, 0);
+	hugetlb_set_page_subpool(page, NULL);
 	page->mapping = NULL;
-	restore_reserve = PagePrivate(page);
-	ClearPagePrivate(page);
+	restore_reserve = HPageRestoreReserve(page);
+	ClearHPageRestoreReserve(page);
 
 	/*
-	 * If PagePrivate() was set on page, page allocation consumed a
+	 * If HPageRestoreReserve was set on page, page allocation consumed a
 	 * reservation.  If the page was associated with a subpool, there
 	 * would have been a page reserved in the subpool before allocation
 	 * via hugepage_subpool_get_pages().  Since we are 'restoring' the
@@ -2251,24 +2250,24 @@ static long vma_add_reservation(struct hstate *h,
  * This routine is called to restore a reservation on error paths.  In the
  * specific error paths, a huge page was allocated (via alloc_huge_page)
  * and is about to be freed.  If a reservation for the page existed,
- * alloc_huge_page would have consumed the reservation and set PagePrivate
- * in the newly allocated page.  When the page is freed via free_huge_page,
- * the global reservation count will be incremented if PagePrivate is set.
- * However, free_huge_page can not adjust the reserve map.  Adjust the
- * reserve map here to be consistent with global reserve count adjustments
- * to be made by free_huge_page.
+ * alloc_huge_page would have consumed the reservation and set
+ * HPageRestoreReserve in the newly allocated page.  When the page is freed
+ * via free_huge_page, the global reservation count will be incremented if
+ * HPageRestoreReserve is set.  However, free_huge_page can not adjust the
+ * reserve map.  Adjust the reserve map here to be consistent with global
+ * reserve count adjustments to be made by free_huge_page.
  */
 static void restore_reserve_on_error(struct hstate *h,
 			struct vm_area_struct *vma, unsigned long address,
 			struct page *page)
 {
-	if (unlikely(PagePrivate(page))) {
+	if (unlikely(HPageRestoreReserve(page))) {
 		long rc = vma_needs_reservation(h, vma, address);
 
 		if (unlikely(rc < 0)) {
 			/*
 			 * Rare out of memory condition in reserve map
-			 * manipulation.  Clear PagePrivate so that
+			 * manipulation.  Clear HPageRestoreReserve so that
 			 * global reserve count will not be incremented
 			 * by free_huge_page.  This will make it appear
 			 * as though the reservation for this page was
@@ -2277,7 +2276,7 @@ static void restore_reserve_on_error(struct hstate *h,
 			 * is better than inconsistent global huge page
 			 * accounting of reserve counts.
 			 */
-			ClearPagePrivate(page);
+			ClearHPageRestoreReserve(page);
 		} else if (rc) {
 			rc = vma_add_reservation(h, vma, address);
 			if (unlikely(rc < 0))
@@ -2285,7 +2284,7 @@ static void restore_reserve_on_error(struct hstate *h,
 				 * See above comment about rare out of
 				 * memory condition.
 				 */
-				ClearPagePrivate(page);
+				ClearHPageRestoreReserve(page);
 		} else
 			vma_end_reservation(h, vma, address);
 	}
@@ -2366,7 +2365,7 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
 		if (!page)
 			goto out_uncharge_cgroup;
 		if (!avoid_reserve && vma_has_reserves(vma, gbl_chg)) {
-			SetPagePrivate(page);
+			SetHPageRestoreReserve(page);
 			h->resv_huge_pages--;
 		}
 		spin_lock(&hugetlb_lock);
@@ -2384,7 +2383,7 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
 
 	spin_unlock(&hugetlb_lock);
 
-	set_page_private(page, (unsigned long)spool);
+	hugetlb_set_page_subpool(page, spool);
 
 	map_commit = vma_commit_reservation(h, vma, addr);
 	if (unlikely(map_chg > map_commit)) {
@@ -3156,6 +3155,9 @@ static int __init hugetlb_init(void)
 {
 	int i;
 
+	BUILD_BUG_ON(sizeof_field(struct page, private) * BITS_PER_BYTE <
+			__NR_HPAGEFLAGS);
+
 	if (!hugepages_supported()) {
 		if (hugetlb_max_hstate || default_hstate_max_huge_pages)
 			pr_warn("HugeTLB: huge pages not supported, ignoring associated command-line parameters\n");
@@ -4207,7 +4209,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
 	spin_lock(ptl);
 	ptep = huge_pte_offset(mm, haddr, huge_page_size(h));
 	if (likely(ptep && pte_same(huge_ptep_get(ptep), pte))) {
-		ClearPagePrivate(new_page);
+		ClearHPageRestoreReserve(new_page);
 
 		/* Break COW */
 		huge_ptep_clear_flush(vma, haddr, ptep);
@@ -4274,7 +4276,7 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
 
 	if (err)
 		return err;
-	ClearPagePrivate(page);
+	ClearHPageRestoreReserve(page);
 
 	/*
 	 * set page dirty so that it will not be removed from cache/file
@@ -4436,7 +4438,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 		goto backout;
 
 	if (anon_rmap) {
-		ClearPagePrivate(page);
+		ClearHPageRestoreReserve(page);
 		hugepage_add_new_anon_rmap(page, vma, haddr);
 	} else
 		page_dup_rmap(page, true);
@@ -4750,7 +4752,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	if (vm_shared) {
 		page_dup_rmap(page, true);
 	} else {
-		ClearPagePrivate(page);
+		ClearHPageRestoreReserve(page);
 		hugepage_add_new_anon_rmap(page, dst_vma, dst_addr);
 	}
 
-- 
2.29.2

Re: [PATCH v3 1/5] hugetlb: use page.private for hugetlb specific page flags

[Oscar Salvador]

On Fri, Jan 22, 2021 at 11:52:27AM -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 by 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 page flags.  These flags will have test, set and clear
> functions similar to those used for 'normal' page flags.  More importantly,
> an enum of flag values will be created with names that actually reflect
> their purpose.
> 
> In this patch,
> - Create infrastructure for hugetlb specific 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 flag instead of PagePrivate
> 
> Conversion of other state information will happen in subsequent patches.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>

-- 
Oscar Salvador
SUSE L3

Re: [PATCH v3 1/5] hugetlb: use page.private for hugetlb specific page flags

[Michal Hocko]

[sorry for jumping in late]

On Fri 22-01-21 11:52:27, 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 by 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.

OK, I thought the page.private was abused more than for this very
specific case.

> In an effort to make the code more readable, use page.private to contain
> hugetlb specific page flags.  These flags will have test, set and clear
> functions similar to those used for 'normal' page flags.  More importantly,
> an enum of flag values will be created with names that actually reflect
> their purpose.

This is definitely a step into the right direction!

> In this patch,
> - Create infrastructure for hugetlb specific page flag functions
> - Move subpool pointer to page[1].private to make way for flags
>   Create routines with meaningful names to modify subpool field

This makes some sense as well. It is really important that the primary
state is stored in the head page. The respective data can be in tail
pages.

> - Use new HPageRestoreReserve flag instead of PagePrivate

Much better! Although wouldn't HPageReserve be sufficient? The flag name
doesn't really need to tell explicitly what to do with the reserve,
right? Or would that be too confusing?

> Conversion of other state information will happen in subsequent patches.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

I do not see any problems with the patch. I hope I haven't overlooked
anything...

Acked-by: Michal Hocko <mhocko@suse.com>
> ---
>  fs/hugetlbfs/inode.c    | 12 ++------
>  include/linux/hugetlb.h | 68 +++++++++++++++++++++++++++++++++++++++++
>  mm/hugetlb.c            | 48 +++++++++++++++--------------
>  3 files changed, 96 insertions(+), 32 deletions(-)
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 1/5] hugetlb: use page.private for hugetlb specific page flags

[Mike Kravetz]

On 1/27/21 2:20 AM, Michal Hocko wrote:
> [sorry for jumping in late]
> 
> On Fri 22-01-21 11:52:27, 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 by 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.
> 
> OK, I thought the page.private was abused more than for this very
> specific case.
> 
>> In an effort to make the code more readable, use page.private to contain
>> hugetlb specific page flags.  These flags will have test, set and clear
>> functions similar to those used for 'normal' page flags.  More importantly,
>> an enum of flag values will be created with names that actually reflect
>> their purpose.
> 
> This is definitely a step into the right direction!
> 
>> In this patch,
>> - Create infrastructure for hugetlb specific page flag functions
>> - Move subpool pointer to page[1].private to make way for flags
>>   Create routines with meaningful names to modify subpool field
> 
> This makes some sense as well. It is really important that the primary
> state is stored in the head page. The respective data can be in tail
> pages.
> 
>> - Use new HPageRestoreReserve flag instead of PagePrivate
> 
> Much better! Although wouldn't HPageReserve be sufficient? The flag name
> doesn't really need to tell explicitly what to do with the reserve,
> right? Or would that be too confusing?

Thanks for taking a look.

HPageReserve could be sufficient.  I don't have a strong opinion and was
just trying to add as much meaning to the name as possible.  If you do not
have a strong opinion, I would just leave it as is.

-- 
Mike Kravetz

Re: [PATCH v3 1/5] hugetlb: use page.private for hugetlb specific page flags

[Michal Hocko]

Sorry forgot one thing.

On Fri 22-01-21 11:52:27, Mike Kravetz wrote:
> + * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at
> + *	allocation time.  Cleared when page is fully instantiated.  Free
> + *	routine checks flag to restore a reservation on error paths.

Can we document that the pool is stored in head+1->private here please?

> + */
> +enum hugetlb_page_flags {
> +	HPG_restore_reserve = 0,
> +	__NR_HPAGEFLAGS,
> +};

-- 
Michal Hocko
SUSE Labs

[PATCH v3 2/5] hugetlb: convert page_huge_active() HPageMigratable flag

[Mike Kravetz]

Use the new hugetlb page specific flag HPageMigratable to replace the
page_huge_active interfaces.  By it's name, page_huge_active implied
that a huge page was on the active list.  However, that is not really
what code checking the flag wanted to know.  It really wanted to determine
if the huge page could be migrated.  This happens when the page is actually
added to the page cache and/or task page table.  This is the reasoning
behind the name change.

The VM_BUG_ON_PAGE() calls in the *_huge_active() interfaces are not
really necessary as we KNOW the page is a hugetlb page.  Therefore, they
are removed.

The routine page_huge_active checked for PageHeadHuge before testing the
active bit.  This is unnecessary in the case where we hold a reference or
lock and know it is a hugetlb head page.  page_huge_active is also called
without holding a reference or lock (scan_movable_pages), and can race with
code freeing the page.  The extra check in page_huge_active shortened the
race window, but did not prevent the race.  Offline code calling
scan_movable_pages already deals with these races, so removing the check
is acceptable.  Add comment to racy code.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
---
 fs/hugetlbfs/inode.c       |  2 +-
 include/linux/hugetlb.h    |  5 +++++
 include/linux/page-flags.h |  6 -----
 mm/hugetlb.c               | 45 ++++++++++----------------------------
 mm/memory_hotplug.c        |  9 +++++++-
 5 files changed, 25 insertions(+), 42 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index b00801fd6002..e1d7ed2a53a9 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 
 		mutex_unlock(&hugetlb_fault_mutex_table[hash]);
 
-		set_page_huge_active(page);
+		SetHPageMigratable(page);
 		/*
 		 * unlock_page because locked by add_to_page_cache()
 		 * put_page() due to reference from alloc_huge_page()
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index a7eb05315c6e..58be44a915d1 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -480,9 +480,13 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
  * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at
  *	allocation time.  Cleared when page is fully instantiated.  Free
  *	routine checks flag to restore a reservation on error paths.
+ * HPG_migratable  - Set after a newly allocated page is added to the page
+ *	cache and/or page tables.  Indicates the page is a candidate for
+ *	migration.
  */
 enum hugetlb_page_flags {
 	HPG_restore_reserve = 0,
+	HPG_migratable,
 	__NR_HPAGEFLAGS,
 };
 
@@ -525,6 +529,7 @@ static inline void ClearHPage##uname(struct page *page)		\
  * Create functions associated with hugetlb page flags
  */
 HPAGEFLAG(RestoreReserve, restore_reserve)
+HPAGEFLAG(Migratable, migratable)
 
 #ifdef CONFIG_HUGETLB_PAGE
 
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index ec5d0290e0ee..db914477057b 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -592,15 +592,9 @@ static inline void ClearPageCompound(struct page *page)
 #ifdef CONFIG_HUGETLB_PAGE
 int PageHuge(struct page *page);
 int PageHeadHuge(struct page *page);
-bool page_huge_active(struct page *page);
 #else
 TESTPAGEFLAG_FALSE(Huge)
 TESTPAGEFLAG_FALSE(HeadHuge)
-
-static inline bool page_huge_active(struct page *page)
-{
-	return 0;
-}
 #endif
 
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 8ec6138ca81b..f1a3c8230dbf 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1353,30 +1353,6 @@ struct hstate *size_to_hstate(unsigned long size)
 	return NULL;
 }
 
-/*
- * Test to determine whether the hugepage is "active/in-use" (i.e. being linked
- * to hstate->hugepage_activelist.)
- *
- * This function can be called for tail pages, but never returns true for them.
- */
-bool page_huge_active(struct page *page)
-{
-	return PageHeadHuge(page) && PagePrivate(&page[1]);
-}
-
-/* never called for tail page */
-void set_page_huge_active(struct page *page)
-{
-	VM_BUG_ON_PAGE(!PageHeadHuge(page), page);
-	SetPagePrivate(&page[1]);
-}
-
-static void clear_page_huge_active(struct page *page)
-{
-	VM_BUG_ON_PAGE(!PageHeadHuge(page), page);
-	ClearPagePrivate(&page[1]);
-}
-
 /*
  * Internal hugetlb specific page flag. Do not use outside of the hugetlb
  * code
@@ -1438,7 +1414,7 @@ static void __free_huge_page(struct page *page)
 	}
 
 	spin_lock(&hugetlb_lock);
-	clear_page_huge_active(page);
+	ClearHPageMigratable(page);
 	hugetlb_cgroup_uncharge_page(hstate_index(h),
 				     pages_per_huge_page(h), page);
 	hugetlb_cgroup_uncharge_page_rsvd(hstate_index(h),
@@ -4218,7 +4194,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
 				make_huge_pte(vma, new_page, 1));
 		page_remove_rmap(old_page, true);
 		hugepage_add_new_anon_rmap(new_page, vma, haddr);
-		set_page_huge_active(new_page);
+		SetHPageMigratable(new_page);
 		/* Make the old page be freed below */
 		new_page = old_page;
 	}
@@ -4455,12 +4431,12 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 	spin_unlock(ptl);
 
 	/*
-	 * Only make newly allocated pages active.  Existing pages found
-	 * in the pagecache could be !page_huge_active() if they have been
-	 * isolated for migration.
+	 * Only set HPageMigratable in newly allocated pages.  Existing pages
+	 * found in the pagecache may not have HPageMigratableset if they have
+	 * been isolated for migration.
 	 */
 	if (new_page)
-		set_page_huge_active(page);
+		SetHPageMigratable(page);
 
 	unlock_page(page);
 out:
@@ -4771,7 +4747,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	update_mmu_cache(dst_vma, dst_addr, dst_pte);
 
 	spin_unlock(ptl);
-	set_page_huge_active(page);
+	SetHPageMigratable(page);
 	if (vm_shared)
 		unlock_page(page);
 	ret = 0;
@@ -5596,12 +5572,13 @@ bool isolate_huge_page(struct page *page, struct list_head *list)
 	bool ret = true;
 
 	spin_lock(&hugetlb_lock);
-	if (!PageHeadHuge(page) || !page_huge_active(page) ||
+	if (!PageHeadHuge(page) ||
+	    !HPageMigratable(page) ||
 	    !get_page_unless_zero(page)) {
 		ret = false;
 		goto unlock;
 	}
-	clear_page_huge_active(page);
+	ClearHPageMigratable(page);
 	list_move_tail(&page->lru, list);
 unlock:
 	spin_unlock(&hugetlb_lock);
@@ -5612,7 +5589,7 @@ void putback_active_hugepage(struct page *page)
 {
 	VM_BUG_ON_PAGE(!PageHead(page), page);
 	spin_lock(&hugetlb_lock);
-	set_page_huge_active(page);
+	SetHPageMigratable(page);
 	list_move_tail(&page->lru, &(page_hstate(page))->hugepage_activelist);
 	spin_unlock(&hugetlb_lock);
 	put_page(page);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f9d57b9be8c7..465164be4d2d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1260,7 +1260,14 @@ static int scan_movable_pages(unsigned long start, unsigned long end,
 		if (!PageHuge(page))
 			continue;
 		head = compound_head(page);
-		if (page_huge_active(head))
+		/*
+		 * This test is racy as we hold no reference or lock.  The
+		 * hugetlb page could have been free'ed and head is no longer
+		 * a hugetlb page before the following check.  In such unlikely
+		 * cases false positives and negatives are possible.  Calling
+		 * code must deal with these scenarios.
+		 */
+		if (HPageMigratable(head))
 			goto found;
 		skip = compound_nr(head) - (page - head);
 		pfn += skip - 1;
-- 
2.29.2

Re: [PATCH v3 2/5] hugetlb: convert page_huge_active() HPageMigratable flag

[Michal Hocko]

On Fri 22-01-21 11:52:28, Mike Kravetz wrote:
> Use the new hugetlb page specific flag HPageMigratable to replace the
> page_huge_active interfaces.  By it's name, page_huge_active implied
> that a huge page was on the active list.  However, that is not really
> what code checking the flag wanted to know.  It really wanted to determine
> if the huge page could be migrated.  This happens when the page is actually
> added to the page cache and/or task page table.  This is the reasoning
> behind the name change.

yeah, definitely less confusing!

> The VM_BUG_ON_PAGE() calls in the *_huge_active() interfaces are not
> really necessary as we KNOW the page is a hugetlb page.  Therefore, they
> are removed.
> 
> The routine page_huge_active checked for PageHeadHuge before testing the
> active bit.  This is unnecessary in the case where we hold a reference or
> lock and know it is a hugetlb head page.  page_huge_active is also called
> without holding a reference or lock (scan_movable_pages), and can race with
> code freeing the page.  The extra check in page_huge_active shortened the
> race window, but did not prevent the race.  Offline code calling
> scan_movable_pages already deals with these races, so removing the check
> is acceptable.  Add comment to racy code.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Michal Hocko <mhocko@suse.com>

One nit below
[...]
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index a7eb05315c6e..58be44a915d1 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -480,9 +480,13 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>   * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at
>   *	allocation time.  Cleared when page is fully instantiated.  Free
>   *	routine checks flag to restore a reservation on error paths.
> + * HPG_migratable  - Set after a newly allocated page is added to the page
> + *	cache and/or page tables.  Indicates the page is a candidate for
> + *	migration.

The state change is synchronized by hugetlb_lock.

Pls.

>   */
>  enum hugetlb_page_flags {
>  	HPG_restore_reserve = 0,
> +	HPG_migratable,
>  	__NR_HPAGEFLAGS,
>  };
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 2/5] hugetlb: convert page_huge_active() HPageMigratable flag

[Mike Kravetz]

On 1/27/21 2:25 AM, Michal Hocko wrote:
> On Fri 22-01-21 11:52:28, Mike Kravetz wrote:
>> Use the new hugetlb page specific flag HPageMigratable to replace the
>> page_huge_active interfaces.  By it's name, page_huge_active implied
>> that a huge page was on the active list.  However, that is not really
>> what code checking the flag wanted to know.  It really wanted to determine
>> if the huge page could be migrated.  This happens when the page is actually
>> added to the page cache and/or task page table.  This is the reasoning
>> behind the name change.
> 
> yeah, definitely less confusing!
> 
>> The VM_BUG_ON_PAGE() calls in the *_huge_active() interfaces are not
>> really necessary as we KNOW the page is a hugetlb page.  Therefore, they
>> are removed.
>>
>> The routine page_huge_active checked for PageHeadHuge before testing the
>> active bit.  This is unnecessary in the case where we hold a reference or
>> lock and know it is a hugetlb head page.  page_huge_active is also called
>> without holding a reference or lock (scan_movable_pages), and can race with
>> code freeing the page.  The extra check in page_huge_active shortened the
>> race window, but did not prevent the race.  Offline code calling
>> scan_movable_pages already deals with these races, so removing the check
>> is acceptable.  Add comment to racy code.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> Reviewed-by: Oscar Salvador <osalvador@suse.de>
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
>> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> One nit below
> [...]
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index a7eb05315c6e..58be44a915d1 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -480,9 +480,13 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>>   * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at
>>   *	allocation time.  Cleared when page is fully instantiated.  Free
>>   *	routine checks flag to restore a reservation on error paths.
>> + * HPG_migratable  - Set after a newly allocated page is added to the page
>> + *	cache and/or page tables.  Indicates the page is a candidate for
>> + *	migration.
> 
> The state change is synchronized by hugetlb_lock.
> 
> Pls.

I will update/explain this.  But ... hugetlb_lock does not synchronize all
changes of this flag.  The flag is set without holding the lock for newly
allocated pages after being added to page cache and/or page tables.  This
'signals' the page is a candidate for migration.  When the migration code
(isolation, putback, move state) deals with the flag, it does hold the
hugetlb_lock.

This patch did not change any of the synchronization.
-- 
Mike Kravetz

[PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Mike Kravetz]

The HP_Migratable flag indicates a page is a candidate for migration.
Only set the flag if the page's hstate supports migration.  This allows
the migration paths to detect non-migratable pages earlier.  If migration
is not supported for the hstate, HP_Migratable will not be set, the page
will not be isolated and no attempt will be made to migrate.  We should
never get to unmap_and_move_huge_page for a page where migration is not
supported, so throw a warning if we do.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c    | 2 +-
 include/linux/hugetlb.h | 9 +++++++++
 mm/hugetlb.c            | 8 ++++----
 mm/migrate.c            | 9 ++++-----
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index e1d7ed2a53a9..93f7b8d3c5fd 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 
 		mutex_unlock(&hugetlb_fault_mutex_table[hash]);
 
-		SetHPageMigratable(page);
+		SetHPageMigratableIfSupported(page);
 		/*
 		 * unlock_page because locked by add_to_page_cache()
 		 * put_page() due to reference from alloc_huge_page()
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 58be44a915d1..cd1960541f2a 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h)
 	return arch_hugetlb_migration_supported(h);
 }
 
+/*
+ * Only set HPageMigratable if migration supported for page
+ */
+static inline void SetHPageMigratableIfSupported(struct page *page)
+{
+	if (hugepage_migration_supported(page_hstate(page)))
+		SetHPageMigratable(page);
+}
+
 /*
  * Movability check is different as compared to migration check.
  * It determines whether or not a huge page should be placed on
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f1a3c8230dbf..4da1a29ac5e2 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4194,7 +4194,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
 				make_huge_pte(vma, new_page, 1));
 		page_remove_rmap(old_page, true);
 		hugepage_add_new_anon_rmap(new_page, vma, haddr);
-		SetHPageMigratable(new_page);
+		SetHPageMigratableIfSupported(new_page);
 		/* Make the old page be freed below */
 		new_page = old_page;
 	}
@@ -4436,7 +4436,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 	 * been isolated for migration.
 	 */
 	if (new_page)
-		SetHPageMigratable(page);
+		SetHPageMigratableIfSupported(page);
 
 	unlock_page(page);
 out:
@@ -4747,7 +4747,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	update_mmu_cache(dst_vma, dst_addr, dst_pte);
 
 	spin_unlock(ptl);
-	SetHPageMigratable(page);
+	SetHPageMigratableIfSupported(page);
 	if (vm_shared)
 		unlock_page(page);
 	ret = 0;
@@ -5589,7 +5589,7 @@ void putback_active_hugepage(struct page *page)
 {
 	VM_BUG_ON_PAGE(!PageHead(page), page);
 	spin_lock(&hugetlb_lock);
-	SetHPageMigratable(page);
+	SetHPageMigratableIfSupported(page);
 	list_move_tail(&page->lru, &(page_hstate(page))->hugepage_activelist);
 	spin_unlock(&hugetlb_lock);
 	put_page(page);
diff --git a/mm/migrate.c b/mm/migrate.c
index a3e1acc72ad7..c8d19e83f372 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1275,13 +1275,12 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 	struct address_space *mapping = NULL;
 
 	/*
-	 * Migratability of hugepages depends on architectures and their size.
-	 * This check is necessary because some callers of hugepage migration
-	 * like soft offline and memory hotremove don't walk through page
-	 * tables or check whether the hugepage is pmd-based or not before
-	 * kicking migration.
+	 * Support for migration should be checked at isolation time.
+	 * Therefore, we should never get here if migration is not supported
+	 * for the page.
 	 */
 	if (!hugepage_migration_supported(page_hstate(hpage))) {
+		VM_WARN_ON(1);
 		list_move_tail(&hpage->lru, ret);
 		return -ENOSYS;
 	}
-- 
2.29.2

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Oscar Salvador]

On Fri, Jan 22, 2021 at 11:52:29AM -0800, Mike Kravetz wrote:
> The HP_Migratable flag indicates a page is a candidate for migration.
> Only set the flag if the page's hstate supports migration.  This allows
> the migration paths to detect non-migratable pages earlier.  If migration
> is not supported for the hstate, HP_Migratable will not be set, the page
> will not be isolated and no attempt will be made to migrate.  We should
> never get to unmap_and_move_huge_page for a page where migration is not
> supported, so throw a warning if we do.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

I wish there was a better way to do this like opencode it at fault path,
but as you mentioned that is troublesome.

I am not a big fan of the name either, too long for my taste but I cannot
come up with anything better so:

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  fs/hugetlbfs/inode.c    | 2 +-
>  include/linux/hugetlb.h | 9 +++++++++
>  mm/hugetlb.c            | 8 ++++----
>  mm/migrate.c            | 9 ++++-----
>  4 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index e1d7ed2a53a9..93f7b8d3c5fd 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  
>  		mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>  
> -		SetHPageMigratable(page);
> +		SetHPageMigratableIfSupported(page);
>  		/*
>  		 * unlock_page because locked by add_to_page_cache()
>  		 * put_page() due to reference from alloc_huge_page()
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 58be44a915d1..cd1960541f2a 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h)
>  	return arch_hugetlb_migration_supported(h);
>  }
>  
> +/*
> + * Only set HPageMigratable if migration supported for page
> + */
> +static inline void SetHPageMigratableIfSupported(struct page *page)
> +{
> +	if (hugepage_migration_supported(page_hstate(page)))
> +		SetHPageMigratable(page);
> +}
> +
>  /*
>   * Movability check is different as compared to migration check.
>   * It determines whether or not a huge page should be placed on
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f1a3c8230dbf..4da1a29ac5e2 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4194,7 +4194,7 @@ static vm_fault_t hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
>  				make_huge_pte(vma, new_page, 1));
>  		page_remove_rmap(old_page, true);
>  		hugepage_add_new_anon_rmap(new_page, vma, haddr);
> -		SetHPageMigratable(new_page);
> +		SetHPageMigratableIfSupported(new_page);
>  		/* Make the old page be freed below */
>  		new_page = old_page;
>  	}
> @@ -4436,7 +4436,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
>  	 * been isolated for migration.
>  	 */
>  	if (new_page)
> -		SetHPageMigratable(page);
> +		SetHPageMigratableIfSupported(page);
>  
>  	unlock_page(page);
>  out:
> @@ -4747,7 +4747,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>  	update_mmu_cache(dst_vma, dst_addr, dst_pte);
>  
>  	spin_unlock(ptl);
> -	SetHPageMigratable(page);
> +	SetHPageMigratableIfSupported(page);
>  	if (vm_shared)
>  		unlock_page(page);
>  	ret = 0;
> @@ -5589,7 +5589,7 @@ void putback_active_hugepage(struct page *page)
>  {
>  	VM_BUG_ON_PAGE(!PageHead(page), page);
>  	spin_lock(&hugetlb_lock);
> -	SetHPageMigratable(page);
> +	SetHPageMigratableIfSupported(page);
>  	list_move_tail(&page->lru, &(page_hstate(page))->hugepage_activelist);
>  	spin_unlock(&hugetlb_lock);
>  	put_page(page);
> diff --git a/mm/migrate.c b/mm/migrate.c
> index a3e1acc72ad7..c8d19e83f372 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1275,13 +1275,12 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
>  	struct address_space *mapping = NULL;
>  
>  	/*
> -	 * Migratability of hugepages depends on architectures and their size.
> -	 * This check is necessary because some callers of hugepage migration
> -	 * like soft offline and memory hotremove don't walk through page
> -	 * tables or check whether the hugepage is pmd-based or not before
> -	 * kicking migration.
> +	 * Support for migration should be checked at isolation time.
> +	 * Therefore, we should never get here if migration is not supported
> +	 * for the page.
>  	 */
>  	if (!hugepage_migration_supported(page_hstate(hpage))) {
> +		VM_WARN_ON(1);
>  		list_move_tail(&hpage->lru, ret);
>  		return -ENOSYS;
>  	}
> -- 
> 2.29.2
> 
> 

-- 
Oscar Salvador
SUSE L3

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Michal Hocko]

On Fri 22-01-21 11:52:29, Mike Kravetz wrote:
> The HP_Migratable flag indicates a page is a candidate for migration.
> Only set the flag if the page's hstate supports migration.  This allows
> the migration paths to detect non-migratable pages earlier.  If migration
> is not supported for the hstate, HP_Migratable will not be set, the page
> will not be isolated and no attempt will be made to migrate.  We should
> never get to unmap_and_move_huge_page for a page where migration is not
> supported, so throw a warning if we do.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  fs/hugetlbfs/inode.c    | 2 +-
>  include/linux/hugetlb.h | 9 +++++++++
>  mm/hugetlb.c            | 8 ++++----
>  mm/migrate.c            | 9 ++++-----
>  4 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index e1d7ed2a53a9..93f7b8d3c5fd 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  
>  		mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>  
> -		SetHPageMigratable(page);
> +		SetHPageMigratableIfSupported(page);
>  		/*
>  		 * unlock_page because locked by add_to_page_cache()
>  		 * put_page() due to reference from alloc_huge_page()
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 58be44a915d1..cd1960541f2a 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h)
>  	return arch_hugetlb_migration_supported(h);
>  }
>  
> +/*
> + * Only set HPageMigratable if migration supported for page
> + */
> +static inline void SetHPageMigratableIfSupported(struct page *page)

This is really mouthful...

> +{
> +	if (hugepage_migration_supported(page_hstate(page)))
> +		SetHPageMigratable(page);

and it is really a trivial wrapper. I do understand why you want to
prevent from the code duplication and potentially a missing check but
this all is just an internal hugetlb code. Even if the flag is set on
non-migrateable hugetlb page then this will not be fatal. The migration
can fail even on those pages for which migration is supported right?

So I am not really sure this is an improvement in the end. But up to you
I do not really have a strong opinion here.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Mike Kravetz]

On 1/27/21 2:35 AM, Michal Hocko wrote:
> On Fri 22-01-21 11:52:29, Mike Kravetz wrote:
>> The HP_Migratable flag indicates a page is a candidate for migration.
>> Only set the flag if the page's hstate supports migration.  This allows
>> the migration paths to detect non-migratable pages earlier.  If migration
>> is not supported for the hstate, HP_Migratable will not be set, the page
>> will not be isolated and no attempt will be made to migrate.  We should
>> never get to unmap_and_move_huge_page for a page where migration is not
>> supported, so throw a warning if we do.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  fs/hugetlbfs/inode.c    | 2 +-
>>  include/linux/hugetlb.h | 9 +++++++++
>>  mm/hugetlb.c            | 8 ++++----
>>  mm/migrate.c            | 9 ++++-----
>>  4 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index e1d7ed2a53a9..93f7b8d3c5fd 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>>  
>>  		mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>>  
>> -		SetHPageMigratable(page);
>> +		SetHPageMigratableIfSupported(page);
>>  		/*
>>  		 * unlock_page because locked by add_to_page_cache()
>>  		 * put_page() due to reference from alloc_huge_page()
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index 58be44a915d1..cd1960541f2a 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h)
>>  	return arch_hugetlb_migration_supported(h);
>>  }
>>  
>> +/*
>> + * Only set HPageMigratable if migration supported for page
>> + */
>> +static inline void SetHPageMigratableIfSupported(struct page *page)
> 
> This is really mouthful...
> 
>> +{
>> +	if (hugepage_migration_supported(page_hstate(page)))
>> +		SetHPageMigratable(page);
> 
> and it is really a trivial wrapper. I do understand why you want to
> prevent from the code duplication and potentially a missing check but
> this all is just an internal hugetlb code. Even if the flag is set on
> non-migrateable hugetlb page then this will not be fatal. The migration
> can fail even on those pages for which migration is supported right?
> 
> So I am not really sure this is an improvement in the end. But up to you
> I do not really have a strong opinion here.

Yes, this patch is somewhat optional.  It should be a minor improvement
in cases where we are dealing with hpages in a non-migratable hstate.
Although, I do not believe this is the common case.

The real reason for even looking into this was a comment by Oscar.  With
the name change to HPageMigratable, it implies that the page is migratable.
However, this is not the case if the page's hstate does not support migration.
So, if we check the hstate when setting the flag we can eliminate those
cases where the page is certainly not migratable.

I don't really love this patch.  It has minimal functional value.

Oscar, what do you think about dropping this?
-- 
Mike Kravetz

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Oscar Salvador]

On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote:
> Yes, this patch is somewhat optional.  It should be a minor improvement
> in cases where we are dealing with hpages in a non-migratable hstate.
> Although, I do not believe this is the common case.
> 
> The real reason for even looking into this was a comment by Oscar.  With
> the name change to HPageMigratable, it implies that the page is migratable.
> However, this is not the case if the page's hstate does not support migration.
> So, if we check the hstate when setting the flag we can eliminate those
> cases where the page is certainly not migratable.
> 
> I don't really love this patch.  It has minimal functional value.
> 
> Oscar, what do you think about dropping this?

Yeah, I remember this topic arose during a discussion of patch#2 in the
early versions, about whether the renaming to HPageMigratable made
sense.

Back then I thought that we could have this in one place at fault-path [1],
which should have made this prettier, but it is not the case.
True is that the optimization is little, so I am fine with dropping this
patch.

unmap_and_move_huge_page() fences off pages belonging to non-migratable
hstates.

[1] https://patchwork.kernel.org/project/linux-mm/patch/20210120013049.311822-3-mike.kravetz@oracle.com/#23914033

Thanks

-- 
Oscar Salvador
SUSE L3

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Andrew Morton]

On Thu, 28 Jan 2021 06:52:21 +0100 Oscar Salvador <osalvador@suse.de> wrote:

> On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote:
> > Yes, this patch is somewhat optional.  It should be a minor improvement
> > in cases where we are dealing with hpages in a non-migratable hstate.
> > Although, I do not believe this is the common case.
> > 
> > The real reason for even looking into this was a comment by Oscar.  With
> > the name change to HPageMigratable, it implies that the page is migratable.
> > However, this is not the case if the page's hstate does not support migration.
> > So, if we check the hstate when setting the flag we can eliminate those
> > cases where the page is certainly not migratable.
> > 
> > I don't really love this patch.  It has minimal functional value.
> > 
> > Oscar, what do you think about dropping this?
> 
> Yeah, I remember this topic arose during a discussion of patch#2 in the
> early versions, about whether the renaming to HPageMigratable made
> sense.
> 
> Back then I thought that we could have this in one place at fault-path [1],
> which should have made this prettier, but it is not the case.
> True is that the optimization is little, so I am fine with dropping this
> patch.

I've dropped it.

> unmap_and_move_huge_page() fences off pages belonging to non-migratable
> hstates.
> 
> [1] https://patchwork.kernel.org/project/linux-mm/patch/20210120013049.311822-3-mike.kravetz@oracle.com/#23914033

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Mike Kravetz]

On 1/28/21 1:37 PM, Andrew Morton wrote:
> On Thu, 28 Jan 2021 06:52:21 +0100 Oscar Salvador <osalvador@suse.de> wrote:
> 
>> On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote:
>>> Yes, this patch is somewhat optional.  It should be a minor improvement
>>> in cases where we are dealing with hpages in a non-migratable hstate.
>>> Although, I do not believe this is the common case.
>>>
>>> The real reason for even looking into this was a comment by Oscar.  With
>>> the name change to HPageMigratable, it implies that the page is migratable.
>>> However, this is not the case if the page's hstate does not support migration.
>>> So, if we check the hstate when setting the flag we can eliminate those
>>> cases where the page is certainly not migratable.
>>>
>>> I don't really love this patch.  It has minimal functional value.
>>>
>>> Oscar, what do you think about dropping this?
>>
>> Yeah, I remember this topic arose during a discussion of patch#2 in the
>> early versions, about whether the renaming to HPageMigratable made
>> sense.
>>
>> Back then I thought that we could have this in one place at fault-path [1],
>> which should have made this prettier, but it is not the case.
>> True is that the optimization is little, so I am fine with dropping this
>> patch.
> 
> I've dropped it.

Thanks Andrew.

Michal suggested that comments describing synchronization be added for each
flag.  Since I did 'one patch per flag', that would be an update to each patch.
Or, I could simply add a patch to update the comment block based on what you
already have.

Let me know what is best/easiest for you.
-- 
Mike Kravetz

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Andrew Morton]

On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> On 1/28/21 1:37 PM, Andrew Morton wrote:
> > On Thu, 28 Jan 2021 06:52:21 +0100 Oscar Salvador <osalvador@suse.de> wrote:
> > 
> >> On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote:
> >>> Yes, this patch is somewhat optional.  It should be a minor improvement
> >>> in cases where we are dealing with hpages in a non-migratable hstate.
> >>> Although, I do not believe this is the common case.
> >>>
> >>> The real reason for even looking into this was a comment by Oscar.  With
> >>> the name change to HPageMigratable, it implies that the page is migratable.
> >>> However, this is not the case if the page's hstate does not support migration.
> >>> So, if we check the hstate when setting the flag we can eliminate those
> >>> cases where the page is certainly not migratable.
> >>>
> >>> I don't really love this patch.  It has minimal functional value.
> >>>
> >>> Oscar, what do you think about dropping this?
> >>
> >> Yeah, I remember this topic arose during a discussion of patch#2 in the
> >> early versions, about whether the renaming to HPageMigratable made
> >> sense.
> >>
> >> Back then I thought that we could have this in one place at fault-path [1],
> >> which should have made this prettier, but it is not the case.
> >> True is that the optimization is little, so I am fine with dropping this
> >> patch.
> > 
> > I've dropped it.
> 
> Thanks Andrew.
> 
> Michal suggested that comments describing synchronization be added for each
> flag.  Since I did 'one patch per flag', that would be an update to each patch.
> Or, I could simply add a patch to update the comment block based on what you
> already have.
> 
> Let me know what is best/easiest for you.

I guess just one patch is best for reviewers.  Then I'll split up into
a sprinkle of -fix patches if I'm feeling energetic ;)

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Michal Hocko]

On Thu 28-01-21 14:15:31, Andrew Morton wrote:
> On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:
> 
> > On 1/28/21 1:37 PM, Andrew Morton wrote:
> > > On Thu, 28 Jan 2021 06:52:21 +0100 Oscar Salvador <osalvador@suse.de> wrote:
> > > 
> > >> On Wed, Jan 27, 2021 at 03:36:41PM -0800, Mike Kravetz wrote:
> > >>> Yes, this patch is somewhat optional.  It should be a minor improvement
> > >>> in cases where we are dealing with hpages in a non-migratable hstate.
> > >>> Although, I do not believe this is the common case.
> > >>>
> > >>> The real reason for even looking into this was a comment by Oscar.  With
> > >>> the name change to HPageMigratable, it implies that the page is migratable.
> > >>> However, this is not the case if the page's hstate does not support migration.
> > >>> So, if we check the hstate when setting the flag we can eliminate those
> > >>> cases where the page is certainly not migratable.
> > >>>
> > >>> I don't really love this patch.  It has minimal functional value.
> > >>>
> > >>> Oscar, what do you think about dropping this?
> > >>
> > >> Yeah, I remember this topic arose during a discussion of patch#2 in the
> > >> early versions, about whether the renaming to HPageMigratable made
> > >> sense.
> > >>
> > >> Back then I thought that we could have this in one place at fault-path [1],
> > >> which should have made this prettier, but it is not the case.
> > >> True is that the optimization is little, so I am fine with dropping this
> > >> patch.
> > > 
> > > I've dropped it.
> > 
> > Thanks Andrew.
> > 
> > Michal suggested that comments describing synchronization be added for each
> > flag.  Since I did 'one patch per flag', that would be an update to each patch.
> > Or, I could simply add a patch to update the comment block based on what you
> > already have.
> > 
> > Let me know what is best/easiest for you.
> 
> I guess just one patch is best for reviewers.  Then I'll split up into
> a sprinkle of -fix patches if I'm feeling energetic ;)

A single patch on top should be just fine. It is not like this would be
a bisectability breaker.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Mike Kravetz]

On 1/28/21 2:15 PM, Andrew Morton wrote:
> On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>
>> Michal suggested that comments describing synchronization be added for each
>> flag.  Since I did 'one patch per flag', that would be an update to each patch.
>> Or, I could simply add a patch to update the comment block based on what you
>> already have.
>>
>> Let me know what is best/easiest for you.
> 
> I guess just one patch is best for reviewers.  Then I'll split up into
> a sprinkle of -fix patches if I'm feeling energetic ;)

Here is a patch to update the comments for all those flags.  It should
apply on top of what is in your tree.

From: Mike Kravetz <mike.kravetz@oracle.com>
Date: Fri, 29 Jan 2021 10:36:12 -0800
Subject: [PATCH] huegtlb: add synchronization information for new hugetlb
 specific flags

Adding comments, no functional change.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 include/linux/hugetlb.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index e5e6ffd55392..cf70795d2209 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -480,14 +480,24 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
  * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at
  *	allocation time.  Cleared when page is fully instantiated.  Free
  *	routine checks flag to restore a reservation on error paths.
+ *	Synchronization:  Examined or modified by code that knows it has
+ *	the only reference to page.  i.e. After allocation but before use
+ *	or when the page is being freed.
  * HPG_migratable  - Set after a newly allocated page is added to the page
  *	cache and/or page tables.  Indicates the page is a candidate for
  *	migration.
+ *	Synchronization:  Initially set after new page allocation with no
+ *	locking.  When examined and modified during migration processing
+ *	(isolate, migrate, putback) the hugetlb_lock is held.
  * HPG_temporary - - Set on a page that is temporarily allocated from the buddy
  *	allocator.  Typically used for migration target pages when no pages
  *	are available in the pool.  The hugetlb free page path will
  *	immediately free pages with this flag set to the buddy allocator.
+ *	Synchronization: Can be set after huge page allocation from buddy when
+ *	code knows it has only reference.  All other examinations and
+ *	modifications require hugetlb_lock.
  * HPG_freed - Set when page is on the free lists.
+ *	Synchronization: hugetlb_lock held for examination and modification.
  */
 enum hugetlb_page_flags {
 	HPG_restore_reserve = 0,
-- 
2.29.2

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Michal Hocko]

On Fri 29-01-21 10:46:15, Mike Kravetz wrote:
> On 1/28/21 2:15 PM, Andrew Morton wrote:
> > On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:
> >>
> >> Michal suggested that comments describing synchronization be added for each
> >> flag.  Since I did 'one patch per flag', that would be an update to each patch.
> >> Or, I could simply add a patch to update the comment block based on what you
> >> already have.
> >>
> >> Let me know what is best/easiest for you.
> > 
> > I guess just one patch is best for reviewers.  Then I'll split up into
> > a sprinkle of -fix patches if I'm feeling energetic ;)
> 
> Here is a patch to update the comments for all those flags.  It should
> apply on top of what is in your tree.
> 
> From: Mike Kravetz <mike.kravetz@oracle.com>
> Date: Fri, 29 Jan 2021 10:36:12 -0800
> Subject: [PATCH] huegtlb: add synchronization information for new hugetlb
>  specific flags
> 
> Adding comments, no functional change.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks Mike!

> ---
>  include/linux/hugetlb.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index e5e6ffd55392..cf70795d2209 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -480,14 +480,24 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>   * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at
>   *	allocation time.  Cleared when page is fully instantiated.  Free
>   *	routine checks flag to restore a reservation on error paths.
> + *	Synchronization:  Examined or modified by code that knows it has
> + *	the only reference to page.  i.e. After allocation but before use
> + *	or when the page is being freed.
>   * HPG_migratable  - Set after a newly allocated page is added to the page
>   *	cache and/or page tables.  Indicates the page is a candidate for
>   *	migration.
> + *	Synchronization:  Initially set after new page allocation with no
> + *	locking.  When examined and modified during migration processing
> + *	(isolate, migrate, putback) the hugetlb_lock is held.
>   * HPG_temporary - - Set on a page that is temporarily allocated from the buddy
>   *	allocator.  Typically used for migration target pages when no pages
>   *	are available in the pool.  The hugetlb free page path will
>   *	immediately free pages with this flag set to the buddy allocator.
> + *	Synchronization: Can be set after huge page allocation from buddy when
> + *	code knows it has only reference.  All other examinations and
> + *	modifications require hugetlb_lock.
>   * HPG_freed - Set when page is on the free lists.
> + *	Synchronization: hugetlb_lock held for examination and modification.
>   */
>  enum hugetlb_page_flags {
>  	HPG_restore_reserve = 0,
> -- 
> 2.29.2
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Mike Kravetz]

On 2/1/21 3:49 AM, Michal Hocko wrote:
> On Fri 29-01-21 10:46:15, Mike Kravetz wrote:
>> On 1/28/21 2:15 PM, Andrew Morton wrote:
>>> On Thu, 28 Jan 2021 14:00:29 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>>>
>>>> Michal suggested that comments describing synchronization be added for each
>>>> flag.  Since I did 'one patch per flag', that would be an update to each patch.
>>>> Or, I could simply add a patch to update the comment block based on what you
>>>> already have.
>>>>
>>>> Let me know what is best/easiest for you.
>>>
>>> I guess just one patch is best for reviewers.  Then I'll split up into
>>> a sprinkle of -fix patches if I'm feeling energetic ;)
>>
>> Here is a patch to update the comments for all those flags.  It should
>> apply on top of what is in your tree.
>>
>> From: Mike Kravetz <mike.kravetz@oracle.com>
>> Date: Fri, 29 Jan 2021 10:36:12 -0800
>> Subject: [PATCH] huegtlb: add synchronization information for new hugetlb
>>  specific flags
>>
>> Adding comments, no functional change.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks Mike!

Hi Andrew,

You may have missed this due to the depth of email thread.  Can you pick
this up, or would you like me to resend?

Thanks,
-- 
Mike Kravetz

> 
>> ---
>>  include/linux/hugetlb.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index e5e6ffd55392..cf70795d2209 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -480,14 +480,24 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>>   * HPG_restore_reserve - Set when a hugetlb page consumes a reservation at
>>   *	allocation time.  Cleared when page is fully instantiated.  Free
>>   *	routine checks flag to restore a reservation on error paths.
>> + *	Synchronization:  Examined or modified by code that knows it has
>> + *	the only reference to page.  i.e. After allocation but before use
>> + *	or when the page is being freed.
>>   * HPG_migratable  - Set after a newly allocated page is added to the page
>>   *	cache and/or page tables.  Indicates the page is a candidate for
>>   *	migration.
>> + *	Synchronization:  Initially set after new page allocation with no
>> + *	locking.  When examined and modified during migration processing
>> + *	(isolate, migrate, putback) the hugetlb_lock is held.
>>   * HPG_temporary - - Set on a page that is temporarily allocated from the buddy
>>   *	allocator.  Typically used for migration target pages when no pages
>>   *	are available in the pool.  The hugetlb free page path will
>>   *	immediately free pages with this flag set to the buddy allocator.
>> + *	Synchronization: Can be set after huge page allocation from buddy when
>> + *	code knows it has only reference.  All other examinations and
>> + *	modifications require hugetlb_lock.
>>   * HPG_freed - Set when page is on the free lists.
>> + *	Synchronization: hugetlb_lock held for examination and modification.
>>   */
>>  enum hugetlb_page_flags {
>>  	HPG_restore_reserve = 0,
>> -- 
>> 2.29.2
>>
> 

[PATCH v3 4/5] hugetlb: convert PageHugeTemporary() to HPageTemporary flag

[Mike Kravetz]

Use new hugetlb specific HPageTemporary flag to replace the
PageHugeTemporary() interfaces.  PageHugeTemporary does contain
a PageHuge() check.  However, this interface is only used within
hugetlb code where we know we are dealing with a hugetlb page.
Therefore, the check can be eliminated.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
---
 include/linux/hugetlb.h |  6 ++++++
 mm/hugetlb.c            | 36 +++++++-----------------------------
 2 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index cd1960541f2a..3c86c3a0e144 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -483,10 +483,15 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
  * HPG_migratable  - Set after a newly allocated page is added to the page
  *	cache and/or page tables.  Indicates the page is a candidate for
  *	migration.
+ * HPG_temporary - - Set on a page that is temporarily allocated from the buddy
+ *	allocator.  Typically used for migration target pages when no pages
+ *	are available in the pool.  The hugetlb free page path will
+ *	immediately free pages with this flag set to the buddy allocator.
  */
 enum hugetlb_page_flags {
 	HPG_restore_reserve = 0,
 	HPG_migratable,
+	HPG_temporary,
 	__NR_HPAGEFLAGS,
 };
 
@@ -530,6 +535,7 @@ static inline void ClearHPage##uname(struct page *page)		\
  */
 HPAGEFLAG(RestoreReserve, restore_reserve)
 HPAGEFLAG(Migratable, migratable)
+HPAGEFLAG(Temporary, temporary)
 
 #ifdef CONFIG_HUGETLB_PAGE
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 4da1a29ac5e2..70ffa1027988 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1353,28 +1353,6 @@ struct hstate *size_to_hstate(unsigned long size)
 	return NULL;
 }
 
-/*
- * Internal hugetlb specific page flag. Do not use outside of the hugetlb
- * code
- */
-static inline bool PageHugeTemporary(struct page *page)
-{
-	if (!PageHuge(page))
-		return false;
-
-	return (unsigned long)page[2].mapping == -1U;
-}
-
-static inline void SetPageHugeTemporary(struct page *page)
-{
-	page[2].mapping = (void *)-1U;
-}
-
-static inline void ClearPageHugeTemporary(struct page *page)
-{
-	page[2].mapping = NULL;
-}
-
 static void __free_huge_page(struct page *page)
 {
 	/*
@@ -1422,9 +1400,9 @@ static void __free_huge_page(struct page *page)
 	if (restore_reserve)
 		h->resv_huge_pages++;
 
-	if (PageHugeTemporary(page)) {
+	if (HPageTemporary(page)) {
 		list_del(&page->lru);
-		ClearPageHugeTemporary(page);
+		ClearHPageTemporary(page);
 		update_and_free_page(h, page);
 	} else if (h->surplus_huge_pages_node[nid]) {
 		/* remove the page from active list */
@@ -1860,7 +1838,7 @@ static struct page *alloc_surplus_huge_page(struct hstate *h, gfp_t gfp_mask,
 	 * codeflow
 	 */
 	if (h->surplus_huge_pages >= h->nr_overcommit_huge_pages) {
-		SetPageHugeTemporary(page);
+		SetHPageTemporary(page);
 		spin_unlock(&hugetlb_lock);
 		put_page(page);
 		return NULL;
@@ -1891,7 +1869,7 @@ static struct page *alloc_migrate_huge_page(struct hstate *h, gfp_t gfp_mask,
 	 * We do not account these pages as surplus because they are only
 	 * temporary and will be released properly on the last reference
 	 */
-	SetPageHugeTemporary(page);
+	SetHPageTemporary(page);
 
 	return page;
 }
@@ -5612,12 +5590,12 @@ void move_hugetlb_state(struct page *oldpage, struct page *newpage, int reason)
 	 * here as well otherwise the global surplus count will not match
 	 * the per-node's.
 	 */
-	if (PageHugeTemporary(newpage)) {
+	if (HPageTemporary(newpage)) {
 		int old_nid = page_to_nid(oldpage);
 		int new_nid = page_to_nid(newpage);
 
-		SetPageHugeTemporary(oldpage);
-		ClearPageHugeTemporary(newpage);
+		SetHPageTemporary(oldpage);
+		ClearHPageTemporary(newpage);
 
 		spin_lock(&hugetlb_lock);
 		if (h->surplus_huge_pages_node[old_nid]) {
-- 
2.29.2

Re: [External] [PATCH v3 4/5] hugetlb: convert PageHugeTemporary() to HPageTemporary flag

[Muchun Song]

On Sat, Jan 23, 2021 at 3:55 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> X-Gm-Spam: 0
> X-Gm-Phishy: 0
>
> Use new hugetlb specific HPageTemporary flag to replace the
> PageHugeTemporary() interfaces.  PageHugeTemporary does contain
> a PageHuge() check.  However, this interface is only used within
> hugetlb code where we know we are dealing with a hugetlb page.
> Therefore, the check can be eliminated.
>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

> ---
>  include/linux/hugetlb.h |  6 ++++++
>  mm/hugetlb.c            | 36 +++++++-----------------------------
>  2 files changed, 13 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index cd1960541f2a..3c86c3a0e144 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -483,10 +483,15 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>   * HPG_migratable  - Set after a newly allocated page is added to the page
>   *     cache and/or page tables.  Indicates the page is a candidate for
>   *     migration.
> + * HPG_temporary - - Set on a page that is temporarily allocated from the buddy
> + *     allocator.  Typically used for migration target pages when no pages
> + *     are available in the pool.  The hugetlb free page path will
> + *     immediately free pages with this flag set to the buddy allocator.
>   */
>  enum hugetlb_page_flags {
>         HPG_restore_reserve = 0,
>         HPG_migratable,
> +       HPG_temporary,
>         __NR_HPAGEFLAGS,
>  };
>
> @@ -530,6 +535,7 @@ static inline void ClearHPage##uname(struct page *page)             \
>   */
>  HPAGEFLAG(RestoreReserve, restore_reserve)
>  HPAGEFLAG(Migratable, migratable)
> +HPAGEFLAG(Temporary, temporary)
>
>  #ifdef CONFIG_HUGETLB_PAGE
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 4da1a29ac5e2..70ffa1027988 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1353,28 +1353,6 @@ struct hstate *size_to_hstate(unsigned long size)
>         return NULL;
>  }
>
> -/*
> - * Internal hugetlb specific page flag. Do not use outside of the hugetlb
> - * code
> - */
> -static inline bool PageHugeTemporary(struct page *page)
> -{
> -       if (!PageHuge(page))
> -               return false;
> -
> -       return (unsigned long)page[2].mapping == -1U;
> -}
> -
> -static inline void SetPageHugeTemporary(struct page *page)
> -{
> -       page[2].mapping = (void *)-1U;
> -}
> -
> -static inline void ClearPageHugeTemporary(struct page *page)
> -{
> -       page[2].mapping = NULL;
> -}
> -
>  static void __free_huge_page(struct page *page)
>  {
>         /*
> @@ -1422,9 +1400,9 @@ static void __free_huge_page(struct page *page)
>         if (restore_reserve)
>                 h->resv_huge_pages++;
>
> -       if (PageHugeTemporary(page)) {
> +       if (HPageTemporary(page)) {
>                 list_del(&page->lru);
> -               ClearPageHugeTemporary(page);
> +               ClearHPageTemporary(page);
>                 update_and_free_page(h, page);
>         } else if (h->surplus_huge_pages_node[nid]) {
>                 /* remove the page from active list */
> @@ -1860,7 +1838,7 @@ static struct page *alloc_surplus_huge_page(struct hstate *h, gfp_t gfp_mask,
>          * codeflow
>          */
>         if (h->surplus_huge_pages >= h->nr_overcommit_huge_pages) {
> -               SetPageHugeTemporary(page);
> +               SetHPageTemporary(page);
>                 spin_unlock(&hugetlb_lock);
>                 put_page(page);
>                 return NULL;
> @@ -1891,7 +1869,7 @@ static struct page *alloc_migrate_huge_page(struct hstate *h, gfp_t gfp_mask,
>          * We do not account these pages as surplus because they are only
>          * temporary and will be released properly on the last reference
>          */
> -       SetPageHugeTemporary(page);
> +       SetHPageTemporary(page);
>
>         return page;
>  }
> @@ -5612,12 +5590,12 @@ void move_hugetlb_state(struct page *oldpage, struct page *newpage, int reason)
>          * here as well otherwise the global surplus count will not match
>          * the per-node's.
>          */
> -       if (PageHugeTemporary(newpage)) {
> +       if (HPageTemporary(newpage)) {
>                 int old_nid = page_to_nid(oldpage);
>                 int new_nid = page_to_nid(newpage);
>
> -               SetPageHugeTemporary(oldpage);
> -               ClearPageHugeTemporary(newpage);
> +               SetHPageTemporary(oldpage);
> +               ClearHPageTemporary(newpage);
>
>                 spin_lock(&hugetlb_lock);
>                 if (h->surplus_huge_pages_node[old_nid]) {
> --
> 2.29.2
>

Re: [PATCH v3 4/5] hugetlb: convert PageHugeTemporary() to HPageTemporary flag

[Michal Hocko]

On Fri 22-01-21 11:52:30, Mike Kravetz wrote:
> Use new hugetlb specific HPageTemporary flag to replace the
> PageHugeTemporary() interfaces.  PageHugeTemporary does contain
> a PageHuge() check.  However, this interface is only used within
> hugetlb code where we know we are dealing with a hugetlb page.
> Therefore, the check can be eliminated.

A dedicated flag is definitely something I would go for if that was
available back then when I introduced it.

> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!
> ---
>  include/linux/hugetlb.h |  6 ++++++
>  mm/hugetlb.c            | 36 +++++++-----------------------------
>  2 files changed, 13 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index cd1960541f2a..3c86c3a0e144 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -483,10 +483,15 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>   * HPG_migratable  - Set after a newly allocated page is added to the page
>   *	cache and/or page tables.  Indicates the page is a candidate for
>   *	migration.
> + * HPG_temporary - - Set on a page that is temporarily allocated from the buddy
> + *	allocator.  Typically used for migration target pages when no pages
> + *	are available in the pool.  The hugetlb free page path will
> + *	immediately free pages with this flag set to the buddy allocator.
>   */
>  enum hugetlb_page_flags {
>  	HPG_restore_reserve = 0,
>  	HPG_migratable,
> +	HPG_temporary,
>  	__NR_HPAGEFLAGS,
>  };
>  
> @@ -530,6 +535,7 @@ static inline void ClearHPage##uname(struct page *page)		\
>   */
>  HPAGEFLAG(RestoreReserve, restore_reserve)
>  HPAGEFLAG(Migratable, migratable)
> +HPAGEFLAG(Temporary, temporary)
>  
>  #ifdef CONFIG_HUGETLB_PAGE
>  
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 4da1a29ac5e2..70ffa1027988 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1353,28 +1353,6 @@ struct hstate *size_to_hstate(unsigned long size)
>  	return NULL;
>  }
>  
> -/*
> - * Internal hugetlb specific page flag. Do not use outside of the hugetlb
> - * code
> - */
> -static inline bool PageHugeTemporary(struct page *page)
> -{
> -	if (!PageHuge(page))
> -		return false;
> -
> -	return (unsigned long)page[2].mapping == -1U;
> -}
> -
> -static inline void SetPageHugeTemporary(struct page *page)
> -{
> -	page[2].mapping = (void *)-1U;
> -}
> -
> -static inline void ClearPageHugeTemporary(struct page *page)
> -{
> -	page[2].mapping = NULL;
> -}
> -
>  static void __free_huge_page(struct page *page)
>  {
>  	/*
> @@ -1422,9 +1400,9 @@ static void __free_huge_page(struct page *page)
>  	if (restore_reserve)
>  		h->resv_huge_pages++;
>  
> -	if (PageHugeTemporary(page)) {
> +	if (HPageTemporary(page)) {
>  		list_del(&page->lru);
> -		ClearPageHugeTemporary(page);
> +		ClearHPageTemporary(page);
>  		update_and_free_page(h, page);
>  	} else if (h->surplus_huge_pages_node[nid]) {
>  		/* remove the page from active list */
> @@ -1860,7 +1838,7 @@ static struct page *alloc_surplus_huge_page(struct hstate *h, gfp_t gfp_mask,
>  	 * codeflow
>  	 */
>  	if (h->surplus_huge_pages >= h->nr_overcommit_huge_pages) {
> -		SetPageHugeTemporary(page);
> +		SetHPageTemporary(page);
>  		spin_unlock(&hugetlb_lock);
>  		put_page(page);
>  		return NULL;
> @@ -1891,7 +1869,7 @@ static struct page *alloc_migrate_huge_page(struct hstate *h, gfp_t gfp_mask,
>  	 * We do not account these pages as surplus because they are only
>  	 * temporary and will be released properly on the last reference
>  	 */
> -	SetPageHugeTemporary(page);
> +	SetHPageTemporary(page);
>  
>  	return page;
>  }
> @@ -5612,12 +5590,12 @@ void move_hugetlb_state(struct page *oldpage, struct page *newpage, int reason)
>  	 * here as well otherwise the global surplus count will not match
>  	 * the per-node's.
>  	 */
> -	if (PageHugeTemporary(newpage)) {
> +	if (HPageTemporary(newpage)) {
>  		int old_nid = page_to_nid(oldpage);
>  		int new_nid = page_to_nid(newpage);
>  
> -		SetPageHugeTemporary(oldpage);
> -		ClearPageHugeTemporary(newpage);
> +		SetHPageTemporary(oldpage);
> +		ClearHPageTemporary(newpage);
>  
>  		spin_lock(&hugetlb_lock);
>  		if (h->surplus_huge_pages_node[old_nid]) {
> -- 
> 2.29.2

-- 
Michal Hocko
SUSE Labs

[PATCH v3 5/5] hugetlb: convert PageHugeFreed to HPageFreed flag

[Mike Kravetz]

Use new hugetlb specific HPageFreed flag to replace the
PageHugeFreed interfaces.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Reviewed-by: Muchun Song <songmuchun@bytedance.com>
---
 include/linux/hugetlb.h |  3 +++
 mm/hugetlb.c            | 23 ++++-------------------
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 3c86c3a0e144..f09085c0afea 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -487,11 +487,13 @@ unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
  *	allocator.  Typically used for migration target pages when no pages
  *	are available in the pool.  The hugetlb free page path will
  *	immediately free pages with this flag set to the buddy allocator.
+ * HPG_freed - Set when page is on the free lists.
  */
 enum hugetlb_page_flags {
 	HPG_restore_reserve = 0,
 	HPG_migratable,
 	HPG_temporary,
+	HPG_freed,
 	__NR_HPAGEFLAGS,
 };
 
@@ -536,6 +538,7 @@ static inline void ClearHPage##uname(struct page *page)		\
 HPAGEFLAG(RestoreReserve, restore_reserve)
 HPAGEFLAG(Migratable, migratable)
 HPAGEFLAG(Temporary, temporary)
+HPAGEFLAG(Freed, freed)
 
 #ifdef CONFIG_HUGETLB_PAGE
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 70ffa1027988..c1f283cee0a9 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -79,21 +79,6 @@ DEFINE_SPINLOCK(hugetlb_lock);
 static int num_fault_mutexes;
 struct mutex *hugetlb_fault_mutex_table ____cacheline_aligned_in_smp;
 
-static inline bool PageHugeFreed(struct page *head)
-{
-	return page_private(head + 4) == -1UL;
-}
-
-static inline void SetPageHugeFreed(struct page *head)
-{
-	set_page_private(head + 4, -1UL);
-}
-
-static inline void ClearPageHugeFreed(struct page *head)
-{
-	set_page_private(head + 4, 0);
-}
-
 /* Forward declaration */
 static int hugetlb_acct_memory(struct hstate *h, long delta);
 
@@ -1043,7 +1028,7 @@ static void enqueue_huge_page(struct hstate *h, struct page *page)
 	list_move(&page->lru, &h->hugepage_freelists[nid]);
 	h->free_huge_pages++;
 	h->free_huge_pages_node[nid]++;
-	SetPageHugeFreed(page);
+	SetHPageFreed(page);
 }
 
 static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid)
@@ -1060,7 +1045,7 @@ static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid)
 
 		list_move(&page->lru, &h->hugepage_activelist);
 		set_page_refcounted(page);
-		ClearPageHugeFreed(page);
+		ClearHPageFreed(page);
 		h->free_huge_pages--;
 		h->free_huge_pages_node[nid]--;
 		return page;
@@ -1474,7 +1459,7 @@ static void prep_new_huge_page(struct hstate *h, struct page *page, int nid)
 	spin_lock(&hugetlb_lock);
 	h->nr_huge_pages++;
 	h->nr_huge_pages_node[nid]++;
-	ClearPageHugeFreed(page);
+	ClearHPageFreed(page);
 	spin_unlock(&hugetlb_lock);
 }
 
@@ -1747,7 +1732,7 @@ int dissolve_free_huge_page(struct page *page)
 		 * We should make sure that the page is already on the free list
 		 * when it is dissolved.
 		 */
-		if (unlikely(!PageHugeFreed(head))) {
+		if (unlikely(!HPageFreed(head))) {
 			spin_unlock(&hugetlb_lock);
 			cond_resched();
 
-- 
2.29.2

Re: [PATCH v3 5/5] hugetlb: convert PageHugeFreed to HPageFreed flag

[Michal Hocko]

On Fri 22-01-21 11:52:31, Mike Kravetz wrote:
> Use new hugetlb specific HPageFreed flag to replace the
> PageHugeFreed interfaces.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Again I would love to see locking that is required to gain a stable
value.

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 5/5] hugetlb: convert PageHugeFreed to HPageFreed flag

[Mike Kravetz]

On 1/27/21 2:41 AM, Michal Hocko wrote:
> On Fri 22-01-21 11:52:31, Mike Kravetz wrote:
>> Use new hugetlb specific HPageFreed flag to replace the
>> PageHugeFreed interfaces.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> Reviewed-by: Oscar Salvador <osalvador@suse.de>
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Again I would love to see locking that is required to gain a stable
> value.

Will do

-- 
Mike Kravetz