Hi:
On 2021/1/20 9:30, 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
This PagePrivate flag really confused me for a long time. :(
> 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.
Thanks for doing this. This would make life easier.
>
> 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@xxxxxxxxxx>
> ---
> fs/hugetlbfs/inode.c | 12 ++-----
> include/linux/hugetlb.h | 72 +++++++++++++++++++++++++++++++++++++++++
> mm/hugetlb.c | 45 +++++++++++++-------------
> 3 files changed, 97 insertions(+), 32 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 740693d7f255..b8a661780c4a 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -955,15 +955,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 ef5b144b8aac..be71a00ee2a0 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -472,6 +472,64 @@ 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) \
> + { BUILD_BUG_ON(sizeof_field(struct page, private) * \
> + BITS_PER_BYTE < __NR_HPAGEFLAGS); \
> + 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) \
> + { BUILD_BUG_ON(sizeof_field(struct page, private) * \
> + BITS_PER_BYTE < __NR_HPAGEFLAGS); \
> + 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 +589,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 737b2dce19e6..8bed6b5202d2 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
> @@ -2254,24 +2253,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
> @@ -2280,7 +2279,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))
> @@ -2288,7 +2287,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);
> }
> @@ -2369,7 +2368,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);
> @@ -2387,7 +2386,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)) {
> @@ -4212,7 +4211,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);
> @@ -4279,7 +4278,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
> @@ -4441,7 +4440,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);
> @@ -4755,7 +4754,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);
> }
>
>
Looks good to me.Thanks.
Reviewed-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
