Hi: On 2021/1/20 9:30, 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 the page cache and/or task page table. This is the reasoning behind
s/added/added to/ > 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.krav...@oracle.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 | 8 ++++++- > 5 files changed, 24 insertions(+), 42 deletions(-) > > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > index b8a661780c4a..ec9f03aa2738 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 be71a00ee2a0..ce3d03da0133 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, > }; > > @@ -529,6 +533,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 bc6fd1ee7dd6..04a34c08e0a6 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 8bed6b5202d2..c24da40626d3 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), > @@ -4220,7 +4196,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; > } > @@ -4457,12 +4433,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: > @@ -4773,7 +4749,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; > @@ -5591,12 +5567,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); > @@ -5607,7 +5584,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..563da803e0e0 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1260,7 +1260,13 @@ 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. > + */ Is it necessary to mention that: "offline_pages() could handle these racy cases." in the comment ? > + if (HPageMigratable(head)) > goto found; > skip = compound_nr(head) - (page - head); > pfn += skip - 1; > Looks good to me. Thanks. Reviewed-by: Miaohe Lin <linmia...@huawei.com>