hmm: Suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set

PATCH In review Jason Gunthorpe 4 participants 5 messages May 22, 2019 – May 23, 2019

Subsystems (3)

Memory management HugeTLB HMM - Heterogeneous Memory Management

lore

Share & tools

lore

Jason Gunthorpe

author

2019-05-22 19:52 UTC #0 lore raw

PATCH 1/1 hmm: Suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set

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 <[email protected]>

---
 mm/hmm.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

mm/hmm.c +2 -7

index 0db8491090b888..816c2356f2449f 100644
@@ -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 {

Signature · 1 line

2.21.0

Andrew Morton

maintainer

2019-05-22 20:23 UTC #1 lore raw

Quoted 23 lines · On Wed, 22 May 2019 19:51:55 +0000 Jason Gunthorpe <[email protected]> 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.
...
mm/hmm.c +0 -1
@@ -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,

mm/hmm.c static int hmm_vma_walk_pud(pud_t *pudp, · line 815

Quoted 7 lines

mm/hmm.c +0 -3

@@ -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.

mm/hmm.c long hmm_range_snapshot(struct hmm_range *range) · line 1026

Quoted 12 lines

mm/hmm.c +2 -3

 	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 :(

Jason Gunthorpe

author

2019-05-22 23:51 UTC #2 lore raw

Quoted 38 lines · Andrew Morton

Quoted 22 lines · On Wed, 22 May 2019 19:51:55 +0000 Jason Gunthorpe <[email protected]> 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
(quoted diff) +0 -1
@@ -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,
Review static int hmm_vma_walk_pud(pud_t *pudp, · line 815
Quoted 7 lines
(quoted diff) +0 -3
@@ -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.
Quoted 4 lines
(quoted diff) +0 -0
 	split_huge_pud(walk->vma, pudp, addr);
@@ -1024,9 +1020,8 @@ long hmm_range_snapshot(struct hmm_range *range)
 			return -EFAULT;
 

Quoted 11 lines

Review long hmm_range_snapshot(struct hmm_range *range) · line 1026
Quoted 8 lines
(quoted diff) +2 -3
 		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 <[email protected]>
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.

Signed-off-by Jason Gunthorpe <[email protected]>

---
 include/linux/hugetlb.h | 102 +++++++++++++++++++++++++++++++++-------
 1 file changed, 86 insertions(+), 16 deletions(-)

include/linux/hugetlb.h +86 ~~-16~~

index edf476c8cfb9c0..f895a79c6f5cb4 100644
@@ -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;

Signature · 1 line

2.21.0

Mike Kravetz

Reviewed-by

2019-05-23 17:56 UTC #3 lore raw

Quoted 15 lines · Jason Gunthorpe

Quoted 3 lines · Andrew Morton
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 <[email protected]>
Date: Wed, 22 May 2019 20:45:59 -0300
Subject: [PATCH] mm: Make !CONFIG_HUGE_PAGE wrappers into static inlines

Quoted 5 lines

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 <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>

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 <[email protected]>

Signature · 1 line

Mike Kravetz

Ira Weiny

Reviewed-by

2019-05-23 23:48 UTC #4 lore raw

Quoted 19 lines · Mike Kravetz

Quoted 18 lines · Jason Gunthorpe
Quoted 3 lines · Andrew Morton
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 <[email protected]>
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.

Quoted 7 lines

Quoted 2 lines
Suggested-by Andrew Morton <[email protected]>
Signed-off-by Jason Gunthorpe <[email protected]>
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 <[email protected]>

Quoted 6 lines

Reviewed-by Mike Kravetz <[email protected]>
Signature · 1 line
Mike Kravetz