[PATCH] hmm: Suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set

[PATCH] hmm: Suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set

[Jason Gunthorpe]

gcc reports that several variables are defined but not used.

For the first hunk CONFIG_HUGETLB_PAGE the entire if block is already
protected by pud_huge() which is forced to 0. None of the stuff under
the ifdef causes compilation problems as it is already stubbed out in
the header files.

For the second hunk the dummy huge_page_shift macro doesn't touch the
argument, so just inline the argument.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 mm/hmm.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index 0db8491090b888..816c2356f2449f 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -797,7 +797,6 @@ static int hmm_vma_walk_pud(pud_t *pudp,
 			return hmm_vma_walk_hole_(addr, end, fault,
 						write_fault, walk);
 
-#ifdef CONFIG_HUGETLB_PAGE
 		pfn = pud_pfn(pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
 		for (i = 0; i < npages; ++i, ++pfn) {
 			hmm_vma_walk->pgmap = get_dev_pagemap(pfn,
@@ -813,9 +812,6 @@ static int hmm_vma_walk_pud(pud_t *pudp,
 		}
 		hmm_vma_walk->last = end;
 		return 0;
-#else
-		return -EINVAL;
-#endif
 	}
 
 	split_huge_pud(walk->vma, pudp, addr);
@@ -1024,9 +1020,8 @@ long hmm_range_snapshot(struct hmm_range *range)
 			return -EFAULT;
 
 		if (is_vm_hugetlb_page(vma)) {
-			struct hstate *h = hstate_vma(vma);
-
-			if (huge_page_shift(h) != range->page_shift &&
+			if (huge_page_shift(hstate_vma(vma)) !=
+				    range->page_shift &&
 			    range->page_shift != PAGE_SHIFT)
 				return -EINVAL;
 		} else {
-- 
2.21.0

Re: [PATCH] hmm: Suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set

[Andrew Morton]

On Wed, 22 May 2019 19:51:55 +0000 Jason Gunthorpe <jgg@mellanox.com> wrote:

> gcc reports that several variables are defined but not used.
> 
> For the first hunk CONFIG_HUGETLB_PAGE the entire if block is already
> protected by pud_huge() which is forced to 0. None of the stuff under
> the ifdef causes compilation problems as it is already stubbed out in
> the header files.
> 
> For the second hunk the dummy huge_page_shift macro doesn't touch the
> argument, so just inline the argument.
> 
> ...
>
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -797,7 +797,6 @@ static int hmm_vma_walk_pud(pud_t *pudp,
>  			return hmm_vma_walk_hole_(addr, end, fault,
>  						write_fault, walk);
>  
> -#ifdef CONFIG_HUGETLB_PAGE
>  		pfn = pud_pfn(pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
>  		for (i = 0; i < npages; ++i, ++pfn) {
>  			hmm_vma_walk->pgmap = get_dev_pagemap(pfn,
> @@ -813,9 +812,6 @@ static int hmm_vma_walk_pud(pud_t *pudp,
>  		}
>  		hmm_vma_walk->last = end;
>  		return 0;
> -#else
> -		return -EINVAL;
> -#endif
>  	}

Fair enough.

>  	split_huge_pud(walk->vma, pudp, addr);
> @@ -1024,9 +1020,8 @@ long hmm_range_snapshot(struct hmm_range *range)
>  			return -EFAULT;
>  
>  		if (is_vm_hugetlb_page(vma)) {
> -			struct hstate *h = hstate_vma(vma);
> -
> -			if (huge_page_shift(h) != range->page_shift &&
> +			if (huge_page_shift(hstate_vma(vma)) !=
> +				    range->page_shift &&
>  			    range->page_shift != PAGE_SHIFT)
>  				return -EINVAL;

Also fair enough.  But why the heck is huge_page_shift() a macro?  We
keep doing that and it bites so often :(

Re: [PATCH] hmm: Suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set

[Jason Gunthorpe]

On Wed, May 22, 2019 at 01:23:22PM -0700, Andrew Morton wrote:
> On Wed, 22 May 2019 19:51:55 +0000 Jason Gunthorpe <jgg@mellanox.com> wrote:
> 
> > gcc reports that several variables are defined but not used.
> > 
> > For the first hunk CONFIG_HUGETLB_PAGE the entire if block is already
> > protected by pud_huge() which is forced to 0. None of the stuff under
> > the ifdef causes compilation problems as it is already stubbed out in
> > the header files.
> > 
> > For the second hunk the dummy huge_page_shift macro doesn't touch the
> > argument, so just inline the argument.
> > 
> > ...
> >
> > +++ b/mm/hmm.c
> > @@ -797,7 +797,6 @@ static int hmm_vma_walk_pud(pud_t *pudp,
> >  			return hmm_vma_walk_hole_(addr, end, fault,
> >  						write_fault, walk);
> >  
> > -#ifdef CONFIG_HUGETLB_PAGE
> >  		pfn = pud_pfn(pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
> >  		for (i = 0; i < npages; ++i, ++pfn) {
> >  			hmm_vma_walk->pgmap = get_dev_pagemap(pfn,
> > @@ -813,9 +812,6 @@ static int hmm_vma_walk_pud(pud_t *pudp,
> >  		}
> >  		hmm_vma_walk->last = end;
> >  		return 0;
> > -#else
> > -		return -EINVAL;
> > -#endif
> >  	}
> 
> Fair enough.
> 
> >  	split_huge_pud(walk->vma, pudp, addr);
> > @@ -1024,9 +1020,8 @@ long hmm_range_snapshot(struct hmm_range *range)
> >  			return -EFAULT;
> >  
> >  		if (is_vm_hugetlb_page(vma)) {
> > -			struct hstate *h = hstate_vma(vma);
> > -
> > -			if (huge_page_shift(h) != range->page_shift &&
> > +			if (huge_page_shift(hstate_vma(vma)) !=
> > +				    range->page_shift &&
> >  			    range->page_shift != PAGE_SHIFT)
> >  				return -EINVAL;
> 
> Also fair enough.  But why the heck is huge_page_shift() a macro?  We
> keep doing that and it bites so often :(

Let's fix it, with the below? (compile tested)

Note __alloc_bootmem_huge_page was returning null but the signature
was unsigned int.

From b5e2ff3c88e6962d0e8297c87af855e6fe1a584e Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@mellanox.com>
Date: Wed, 22 May 2019 20:45:59 -0300
Subject: [PATCH] mm: Make !CONFIG_HUGE_PAGE wrappers into static inlines

Instead of using defines, which looses type safety and provokes unused
variable warnings from gcc, put the constants into static inlines.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 include/linux/hugetlb.h | 102 +++++++++++++++++++++++++++++++++-------
 1 file changed, 86 insertions(+), 16 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index edf476c8cfb9c0..f895a79c6f5cb4 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -608,22 +608,92 @@ static inline void huge_ptep_modify_prot_commit(struct vm_area_struct *vma,
 
 #else	/* CONFIG_HUGETLB_PAGE */
 struct hstate {};
-#define alloc_huge_page(v, a, r) NULL
-#define alloc_huge_page_node(h, nid) NULL
-#define alloc_huge_page_nodemask(h, preferred_nid, nmask) NULL
-#define alloc_huge_page_vma(h, vma, address) NULL
-#define alloc_bootmem_huge_page(h) NULL
-#define hstate_file(f) NULL
-#define hstate_sizelog(s) NULL
-#define hstate_vma(v) NULL
-#define hstate_inode(i) NULL
-#define page_hstate(page) NULL
-#define huge_page_size(h) PAGE_SIZE
-#define huge_page_mask(h) PAGE_MASK
-#define vma_kernel_pagesize(v) PAGE_SIZE
-#define vma_mmu_pagesize(v) PAGE_SIZE
-#define huge_page_order(h) 0
-#define huge_page_shift(h) PAGE_SHIFT
+
+static inline struct page *alloc_huge_page(struct vm_area_struct *vma,
+					   unsigned long addr,
+					   int avoid_reserve)
+{
+	return NULL;
+}
+
+static inline struct page *alloc_huge_page_node(struct hstate *h, int nid)
+{
+	return NULL;
+}
+
+static inline struct page *
+alloc_huge_page_nodemask(struct hstate *h, int preferred_nid, nodemask_t *nmask)
+{
+	return NULL;
+}
+
+static inline struct page *alloc_huge_page_vma(struct hstate *h,
+					       struct vm_area_struct *vma,
+					       unsigned long address)
+{
+	return NULL;
+}
+
+static inline int __alloc_bootmem_huge_page(struct hstate *h)
+{
+	return 0;
+}
+
+static inline struct hstate *hstate_file(struct file *f)
+{
+	return NULL;
+}
+
+static inline struct hstate *hstate_sizelog(int page_size_log)
+{
+	return NULL;
+}
+
+static inline struct hstate *hstate_vma(struct vm_area_struct *vma)
+{
+	return NULL;
+}
+
+static inline struct hstate *hstate_inode(struct inode *i)
+{
+	return NULL;
+}
+
+static inline struct hstate *page_hstate(struct page *page)
+{
+	return NULL;
+}
+
+static inline unsigned long huge_page_size(struct hstate *h)
+{
+	return PAGE_SIZE;
+}
+
+static inline unsigned long huge_page_mask(struct hstate *h)
+{
+	return PAGE_MASK;
+}
+
+static inline unsigned long vma_kernel_pagesize(struct vm_area_struct *vma)
+{
+	return PAGE_SIZE;
+}
+
+static inline unsigned long vma_mmu_pagesize(struct vm_area_struct *vma)
+{
+	return PAGE_SIZE;
+}
+
+static inline unsigned int huge_page_order(struct hstate *h)
+{
+	return 0;
+}
+
+static inline unsigned int huge_page_shift(struct hstate *h)
+{
+	return PAGE_SHIFT;
+}
+
 static inline bool hstate_is_gigantic(struct hstate *h)
 {
 	return false;
-- 
2.21.0

Re: [PATCH] hmm: Suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set

[Mike Kravetz]

On 5/22/19 4:51 PM, Jason Gunthorpe wrote:
> On Wed, May 22, 2019 at 01:23:22PM -0700, Andrew Morton wrote:
>>
>> Also fair enough.  But why the heck is huge_page_shift() a macro?  We
>> keep doing that and it bites so often :(
> 
> Let's fix it, with the below? (compile tested)
> 
> Note __alloc_bootmem_huge_page was returning null but the signature
> was unsigned int.
> 
> From b5e2ff3c88e6962d0e8297c87af855e6fe1a584e Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgg@mellanox.com>
> Date: Wed, 22 May 2019 20:45:59 -0300
> Subject: [PATCH] mm: Make !CONFIG_HUGE_PAGE wrappers into static inlines
> 
> Instead of using defines, which looses type safety and provokes unused
> variable warnings from gcc, put the constants into static inlines.
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>

Thanks for doing this Jason.

I do not see any issues unless there is some weird arch specific usage which
would be caught by zero day testing.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

Re: [PATCH] hmm: Suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set

[Ira Weiny]

On Thu, May 23, 2019 at 10:56:09AM -0700, Mike Kravetz wrote:
> On 5/22/19 4:51 PM, Jason Gunthorpe wrote:
> > On Wed, May 22, 2019 at 01:23:22PM -0700, Andrew Morton wrote:
> >>
> >> Also fair enough.  But why the heck is huge_page_shift() a macro?  We
> >> keep doing that and it bites so often :(
> > 
> > Let's fix it, with the below? (compile tested)
> > 
> > Note __alloc_bootmem_huge_page was returning null but the signature
> > was unsigned int.
> > 
> > From b5e2ff3c88e6962d0e8297c87af855e6fe1a584e Mon Sep 17 00:00:00 2001
> > From: Jason Gunthorpe <jgg@mellanox.com>
> > Date: Wed, 22 May 2019 20:45:59 -0300
> > Subject: [PATCH] mm: Make !CONFIG_HUGE_PAGE wrappers into static inlines
> > 
> > Instead of using defines, which looses type safety and provokes unused
> > variable warnings from gcc, put the constants into static inlines.
> > 
> > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> 
> Thanks for doing this Jason.
> 
> I do not see any issues unless there is some weird arch specific usage which
> would be caught by zero day testing.

Agreed.  I did a couple quick searches and I don't see any such issues.  I was
thinking the same thing WRT zero day.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> 
> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> 
> -- 
> Mike Kravetz
>