[v2,1/5] hugetlb: use page.private for hugetlb specific page flags

Commit Message

Mike Kravetz Jan. 20, 2021, 1:30 a.m. UTC

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 <[e-mail]>
---
 fs/hugetlbfs/inode.c    | 12 ++-----
 include/linux/hugetlb.h | 72 +++++++++++++++++++++++++++++++++++++++++
 mm/hugetlb.c            | 45 +++++++++++++-------------
 3 files changed, 97 insertions(+), 32 deletions(-)

Comments

Miaohe Lin Jan. 20, 2021, 8:10 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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 <[e-mail]>

kernel test robot Jan. 20, 2021, 8:43 a.m. UTC | #2

Hi Mike,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20210119]
[also build test ERROR on v5.11-rc4]
[cannot apply to linux/master linus/master hnaz-linux-mm/master v5.11-rc4 v5.11-rc3 v5.11-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mike-Kravetz/create-hugetlb-flags-to-consolidate-state/20210120-142142
base:    b4bb878f3eb3e604ebfe83bbc17eb7af8d99cbf4
config: nds32-defconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ebeb3dd25715894428a5107720212b022616c02f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mike-Kravetz/create-hugetlb-flags-to-consolidate-state/20210120-142142
        git checkout ebeb3dd25715894428a5107720212b022616c02f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>):

   In file included from kernel/fork.c:51:
   include/linux/hugetlb.h: In function 'HPageRestoreReserve':
>> include/linux/hugetlb.h:512:11: error: expected ';' before '}' token
     512 |  return 0 }
         |           ^
   include/linux/hugetlb.h:524:2: note: in expansion of macro 'TESTHPAGEFLAG'
     524 |  TESTHPAGEFLAG(uname, flname)    \
         |  ^~~~~~~~~~~~~
   include/linux/hugetlb.h:531:1: note: in expansion of macro 'HPAGEFLAG'
     531 | HPAGEFLAG(RestoreReserve, restore_reserve)
         | ^~~~~~~~~
   kernel/fork.c: At top level:
   kernel/fork.c:161:13: warning: no previous prototype for 'arch_release_task_struct' [-Wmissing-prototypes]
     161 | void __weak arch_release_task_struct(struct task_struct *tsk)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~
   kernel/fork.c:746:20: warning: no previous prototype for 'arch_task_cache_init' [-Wmissing-prototypes]
     746 | void __init __weak arch_task_cache_init(void) { }
         |                    ^~~~~~~~~~~~~~~~~~~~
   kernel/fork.c:836:12: warning: no previous prototype for 'arch_dup_task_struct' [-Wmissing-prototypes]
     836 | int __weak arch_dup_task_struct(struct task_struct *dst,
         |            ^~~~~~~~~~~~~~~~~~~~
--
   In file included from kernel/sysctl.c:45:
   include/linux/hugetlb.h: In function 'HPageRestoreReserve':
>> include/linux/hugetlb.h:512:11: error: expected ';' before '}' token
     512 |  return 0 }
         |           ^
   include/linux/hugetlb.h:524:2: note: in expansion of macro 'TESTHPAGEFLAG'
     524 |  TESTHPAGEFLAG(uname, flname)    \
         |  ^~~~~~~~~~~~~
   include/linux/hugetlb.h:531:1: note: in expansion of macro 'HPAGEFLAG'
     531 | HPAGEFLAG(RestoreReserve, restore_reserve)
         | ^~~~~~~~~
--
   In file included from include/linux/migrate.h:8,
                    from kernel/sched/sched.h:53,
                    from kernel/sched/core.c:13:
   include/linux/hugetlb.h: In function 'HPageRestoreReserve':
>> include/linux/hugetlb.h:512:11: error: expected ';' before '}' token
     512 |  return 0 }
         |           ^
   include/linux/hugetlb.h:524:2: note: in expansion of macro 'TESTHPAGEFLAG'
     524 |  TESTHPAGEFLAG(uname, flname)    \
         |  ^~~~~~~~~~~~~
   include/linux/hugetlb.h:531:1: note: in expansion of macro 'HPAGEFLAG'
     531 | HPAGEFLAG(RestoreReserve, restore_reserve)
         | ^~~~~~~~~
   kernel/sched/core.c: In function 'ttwu_stat':
   kernel/sched/core.c:2955:13: warning: variable 'rq' set but not used [-Wunused-but-set-variable]
    2955 |  struct rq *rq;
         |             ^~
   kernel/sched/core.c: In function 'schedule_tail':
   kernel/sched/core.c:4298:13: warning: variable 'rq' set but not used [-Wunused-but-set-variable]
    4298 |  struct rq *rq;
         |             ^~
--
   In file included from include/linux/migrate.h:8,
                    from kernel/sched/sched.h:53,
                    from kernel/sched/fair.c:23:
   include/linux/hugetlb.h: In function 'HPageRestoreReserve':
>> include/linux/hugetlb.h:512:11: error: expected ';' before '}' token
     512 |  return 0 }
         |           ^
   include/linux/hugetlb.h:524:2: note: in expansion of macro 'TESTHPAGEFLAG'
     524 |  TESTHPAGEFLAG(uname, flname)    \
         |  ^~~~~~~~~~~~~
   include/linux/hugetlb.h:531:1: note: in expansion of macro 'HPAGEFLAG'
     531 | HPAGEFLAG(RestoreReserve, restore_reserve)
         | ^~~~~~~~~
   kernel/sched/fair.c: At top level:
   kernel/sched/fair.c:5399:6: warning: no previous prototype for 'init_cfs_bandwidth' [-Wmissing-prototypes]
    5399 | void init_cfs_bandwidth(struct cfs_bandwidth *cfs_b) {}
         |      ^~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11218:6: warning: no previous prototype for 'free_fair_sched_group' [-Wmissing-prototypes]
   11218 | void free_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11220:5: warning: no previous prototype for 'alloc_fair_sched_group' [-Wmissing-prototypes]
   11220 | int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
         |     ^~~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11225:6: warning: no previous prototype for 'online_fair_sched_group' [-Wmissing-prototypes]
   11225 | void online_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11227:6: warning: no previous prototype for 'unregister_fair_sched_group' [-Wmissing-prototypes]
   11227 | void unregister_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/migrate.h:8,
                    from kernel/sched/sched.h:53,
                    from kernel/sched/rt.c:6:
   include/linux/hugetlb.h: In function 'HPageRestoreReserve':
>> include/linux/hugetlb.h:512:11: error: expected ';' before '}' token
     512 |  return 0 }
         |           ^
   include/linux/hugetlb.h:524:2: note: in expansion of macro 'TESTHPAGEFLAG'
     524 |  TESTHPAGEFLAG(uname, flname)    \
         |  ^~~~~~~~~~~~~
   include/linux/hugetlb.h:531:1: note: in expansion of macro 'HPAGEFLAG'
     531 | HPAGEFLAG(RestoreReserve, restore_reserve)
         | ^~~~~~~~~
   kernel/sched/rt.c: At top level:
   kernel/sched/rt.c:253:6: warning: no previous prototype for 'free_rt_sched_group' [-Wmissing-prototypes]
     253 | void free_rt_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~
   kernel/sched/rt.c:255:5: warning: no previous prototype for 'alloc_rt_sched_group' [-Wmissing-prototypes]
     255 | int alloc_rt_sched_group(struct task_group *tg, struct task_group *parent)
         |     ^~~~~~~~~~~~~~~~~~~~
   kernel/sched/rt.c:669:6: warning: no previous prototype for 'sched_rt_bandwidth_account' [-Wmissing-prototypes]
     669 | bool sched_rt_bandwidth_account(struct rt_rq *rt_rq)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/migrate.h:8,
                    from mm/page_alloc.c:61:
   include/linux/hugetlb.h: In function 'HPageRestoreReserve':
>> include/linux/hugetlb.h:512:11: error: expected ';' before '}' token
     512 |  return 0 }
         |           ^
   include/linux/hugetlb.h:524:2: note: in expansion of macro 'TESTHPAGEFLAG'
     524 |  TESTHPAGEFLAG(uname, flname)    \
         |  ^~~~~~~~~~~~~
   include/linux/hugetlb.h:531:1: note: in expansion of macro 'HPAGEFLAG'
     531 | HPAGEFLAG(RestoreReserve, restore_reserve)
         | ^~~~~~~~~
   mm/page_alloc.c: At top level:
   mm/page_alloc.c:2618:5: warning: no previous prototype for 'find_suitable_fallback' [-Wmissing-prototypes]
    2618 | int find_suitable_fallback(struct free_area *area, unsigned int order,
         |     ^~~~~~~~~~~~~~~~~~~~~~
   mm/page_alloc.c:3597:15: warning: no previous prototype for 'should_fail_alloc_page' [-Wmissing-prototypes]
    3597 | noinline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
         |               ^~~~~~~~~~~~~~~~~~~~~~
   mm/page_alloc.c:6257:23: warning: no previous prototype for 'memmap_init' [-Wmissing-prototypes]
    6257 | void __meminit __weak memmap_init(unsigned long size, int nid,
         |                       ^~~~~~~~~~~
--
   In file included from fs/proc/meminfo.c:6:
   include/linux/hugetlb.h: In function 'HPageRestoreReserve':
>> include/linux/hugetlb.h:512:11: error: expected ';' before '}' token
     512 |  return 0 }
         |           ^
   include/linux/hugetlb.h:524:2: note: in expansion of macro 'TESTHPAGEFLAG'
     524 |  TESTHPAGEFLAG(uname, flname)    \
         |  ^~~~~~~~~~~~~
   include/linux/hugetlb.h:531:1: note: in expansion of macro 'HPAGEFLAG'
     531 | HPAGEFLAG(RestoreReserve, restore_reserve)
         | ^~~~~~~~~
   fs/proc/meminfo.c: At top level:
   fs/proc/meminfo.c:22:28: warning: no previous prototype for 'arch_report_meminfo' [-Wmissing-prototypes]
      22 | void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
         |                            ^~~~~~~~~~~~~~~~~~~
--
   In file included from kernel/events/core.c:31:
   include/linux/hugetlb.h: In function 'HPageRestoreReserve':
>> include/linux/hugetlb.h:512:11: error: expected ';' before '}' token
     512 |  return 0 }
         |           ^
   include/linux/hugetlb.h:524:2: note: in expansion of macro 'TESTHPAGEFLAG'
     524 |  TESTHPAGEFLAG(uname, flname)    \
         |  ^~~~~~~~~~~~~
   include/linux/hugetlb.h:531:1: note: in expansion of macro 'HPAGEFLAG'
     531 | HPAGEFLAG(RestoreReserve, restore_reserve)
         | ^~~~~~~~~
   kernel/events/core.c: At top level:
   kernel/events/core.c:6539:6: warning: no previous prototype for 'perf_pmu_snapshot_aux' [-Wmissing-prototypes]
    6539 | long perf_pmu_snapshot_aux(struct perf_buffer *rb,
         |      ^~~~~~~~~~~~~~~~~~~~~


vim +512 include/linux/hugetlb.h

   488	
   489	/*
   490	 * Macros to create test, set and clear function definitions for
   491	 * hugetlb specific page flags.
   492	 */
   493	#ifdef CONFIG_HUGETLB_PAGE
   494	#define TESTHPAGEFLAG(uname, flname)				\
   495	static inline int HPage##uname(struct page *page)		\
   496		{ BUILD_BUG_ON(sizeof_field(struct page, private) *	\
   497				BITS_PER_BYTE < __NR_HPAGEFLAGS);	\
   498		return test_bit(HPG_##flname, &(page->private)); }
   499	
   500	#define SETHPAGEFLAG(uname, flname)				\
   501	static inline void SetHPage##uname(struct page *page)		\
   502		{ set_bit(HPG_##flname, &(page->private)); }
   503	
   504	#define CLEARHPAGEFLAG(uname, flname)				\
   505	static inline void ClearHPage##uname(struct page *page)		\
   506		{ clear_bit(HPG_##flname, &(page->private)); }
   507	#else
   508	#define TESTHPAGEFLAG(uname, flname)				\
   509	static inline int HPage##uname(struct page *page)		\
   510		{ BUILD_BUG_ON(sizeof_field(struct page, private) *	\
   511				BITS_PER_BYTE < __NR_HPAGEFLAGS);	\
 > 512		return 0 }
   513	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Oscar Salvador Jan. 20, 2021, 9:30 a.m. UTC | #3

On Tue, Jan 19, 2021 at 05:30:45PM -0800, Mike Kravetz wrote:
> + * 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 }

You missed a ";" right there.

I might be missing something, but I do not think we need a BUILD_BUG_ON there
when CONFIG_HUGETLB_PAGE is not set?
Actually, would make more sense to move the BUILD_BUG_ON from above to
hugetlb_init?

Other than that, looks good to me, and I think it is a great improvment
towards readability and maintability.

Mike Kravetz Jan. 20, 2021, 6:12 p.m. UTC | #4

On 1/20/21 1:30 AM, Oscar Salvador wrote:
> On Tue, Jan 19, 2021 at 05:30:45PM -0800, Mike Kravetz wrote:
>> + * 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 }
> 
> You missed a ";" right there.

Thanks.  I made that typo when fixing up some trivial checkpatch warnings.
Lesson learned (again), nothing is too trivial not to introduce erros.

> I might be missing something, but I do not think we need a BUILD_BUG_ON there
> when CONFIG_HUGETLB_PAGE is not set?
> Actually, would make more sense to move the BUILD_BUG_ON from above to
> hugetlb_init?

Yes, hugetlb_init is a better location for the BUILD_BUG_ON.  I initially
put it there before creating the !CONFIG_HUGETLB_PAGE version of the macros.
With the !CONFIG_HUGETLB_PAGE versions, none of the flags are ever used so
it is OK if the BUILD_BUG_ON is not included if !CONFIG_HUGETLB_PAGE.

Patch

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);
 	}