[RFC PATCH 1/3] powerpc/mm: Handle page table allocation failures

This fixes the below crash that arises due to not handling page table allocation
failures while allocating hugetlb page table.

Fixes: e2b3d202d1db ("powerpc: Switch 16GB and 16MB explicit hugepages to a different page table format")
Signed-off-by: Aneesh Kumar K.V <redacted>
---
 arch/powerpc/mm/hugetlbpage.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index c5c9ff2d7afc..ae9d71da5219 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c

@@ -130,6 +130,8 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr, unsigned long sz
 	} else {
 		pdshift = PUD_SHIFT;
 		pu = pud_alloc(mm, pg, addr);
+		if (!pu)
+			return NULL;
 		if (pshift == PUD_SHIFT)
 			return (pte_t *)pu;
 		else if (pshift > PMD_SHIFT) {

@@ -138,6 +140,8 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr, unsigned long sz
 		} else {
 			pdshift = PMD_SHIFT;
 			pm = pmd_alloc(mm, pu, addr);
+			if (!pm)
+				return NULL;
 			if (pshift == PMD_SHIFT)
 				/* 16MB hugepage */
 				return (pte_t *)pm;

@@ -154,12 +158,16 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr, unsigned long sz
 	} else {
 		pdshift = PUD_SHIFT;
 		pu = pud_alloc(mm, pg, addr);
+		if (!pu)
+			return NULL;
 		if (pshift >= PUD_SHIFT) {
 			ptl = pud_lockptr(mm, pu);
 			hpdp = (hugepd_t *)pu;
 		} else {
 			pdshift = PMD_SHIFT;
 			pm = pmd_alloc(mm, pu, addr);
+			if (!pm)
+				return NULL;
 			ptl = pmd_lockptr(mm, pm);
 			hpdp = (hugepd_t *)pm;
 		}

-- 
2.21.0

We only check for hugetlb allocations, because with hugetlb we do conditional
registration. For PGD/PUD/PMD levels we register them always in
pgtable_cache_init.

Signed-off-by: Aneesh Kumar K.V <redacted>
---
 arch/powerpc/mm/hugetlbpage.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index ae9d71da5219..ee16a3fb788a 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c

@@ -61,12 +61,17 @@ static int __hugepte_alloc(struct mm_struct *mm, hugepd_t *hpdp,
 		num_hugepd = 1;
 	}
 
+	if (!cachep) {
+		WARN_ONCE(1, "No page table cache created for hugetlb tables");
+		return -ENOMEM;
+	}
+
 	new = kmem_cache_alloc(cachep, pgtable_gfp_flags(mm, GFP_KERNEL));
 
 	BUG_ON(pshift > HUGEPD_SHIFT_MASK);
 	BUG_ON((unsigned long)new & HUGEPD_SHIFT_MASK);
 
-	if (! new)
+	if (!new)
 		return -ENOMEM;
 
 	/*

-- 
2.21.0

This makes sure we don't enable HugeTLB if the cache is not configured.
I am still not sure about this. IMHO hugetlb support should be a hardware
support derivative and any cache allocation failure should be handled as I did
in the earlier patch. But then if we were not able to create hugetlb page table
cache, we can as well declare hugetlb support disabled thereby avoiding calling
into allocation routines.

Signed-off-by: Aneesh Kumar K.V <redacted>
---
 arch/powerpc/mm/hugetlbpage.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index ee16a3fb788a..4bf8bc659cc7 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c

@@ -602,6 +602,7 @@ __setup("hugepagesz=", hugepage_setup_sz);
 static int __init hugetlbpage_init(void)
 {
 	int psize;
+	bool configured = false;
 
 	if (hugetlb_disabled) {
 		pr_info("HugeTLB support is disabled!\n");

@@ -651,10 +652,16 @@ static int __init hugetlbpage_init(void)
 			pgtable_cache_add(pdshift - shift);
 		else if (IS_ENABLED(CONFIG_PPC_FSL_BOOK3E) || IS_ENABLED(CONFIG_PPC_8xx))
 			pgtable_cache_add(PTE_T_ORDER);
+
+		if (!configured)
+			configured = true;
 	}
 
-	if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
-		hugetlbpage_init_default();
+	if (configured) {
+		if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
+			hugetlbpage_init_default();
+	} else
+		pr_info("Disabling HugeTLB");
 
 	return 0;
 }

-- 
2.21.0

On 05/14/2019 07:50 AM, Aneesh Kumar K.V wrote:

This fixes the below crash that arises due to not handling page table allocation
failures while allocating hugetlb page table.

Was there supposed to be a oops stack trace attached here in the commit log?

-Tyrel

"Aneesh Kumar K.V" [off-list ref] writes:

Where's my reverse Christmas tree! :)

I'd just not worry about the if.

 	}
 
-	if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
-		hugetlbpage_init_default();
+	if (configured) {
+		if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
+			hugetlbpage_init_default();
+	} else
+		pr_info("Disabling HugeTLB");

We're not actually doing anything to disable it in the
CONFIG_HUGETLB_PAGE_SIZE_VARIABLE=n case, but I guess the print is still
correct because we didn't enable a size in the for loop above?

Can we make it a bit more explicit? Maybe like:

  "Disabling HugeTLB, no usable page sizes found."

??

cheers

On 5/16/19 8:17 PM, Michael Ellerman wrote:

"Aneesh Kumar K.V" [off-list ref] writes:

quoted

This makes sure we don't enable HugeTLB if the cache is not configured.
I am still not sure about this. IMHO hugetlb support should be a hardware
support derivative and any cache allocation failure should be handled as I did
in the earlier patch. But then if we were not able to create hugetlb page table
cache, we can as well declare hugetlb support disabled thereby avoiding calling
into allocation routines.

Signed-off-by: Aneesh Kumar K.V <redacted>
---
  arch/powerpc/mm/hugetlbpage.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index ee16a3fb788a..4bf8bc659cc7 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c

@@ -602,6 +602,7 @@ __setup("hugepagesz=", hugepage_setup_sz);
  static int __init hugetlbpage_init(void)
  {
  	int psize;
+	bool configured = false;

Where's my reverse Christmas tree! :)

Will fix that :)

quoted

  	if (hugetlb_disabled) {
  		pr_info("HugeTLB support is disabled!\n");

@@ -651,10 +652,16 @@ static int __init hugetlbpage_init(void)
  			pgtable_cache_add(pdshift - shift);
  		else if (IS_ENABLED(CONFIG_PPC_FSL_BOOK3E) || IS_ENABLED(CONFIG_PPC_8xx))
  			pgtable_cache_add(PTE_T_ORDER);
+
+		if (!configured)
+			configured = true;

I'd just not worry about the if.

quoted

  	}
  
-	if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
-		hugetlbpage_init_default();
+	if (configured) {
+		if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
+			hugetlbpage_init_default();
+	} else
+		pr_info("Disabling HugeTLB");

We're not actually doing anything to disable it in the
CONFIG_HUGETLB_PAGE_SIZE_VARIABLE=n case, but I guess the print is still
correct because we didn't enable a size in the for loop above?

Can we make it a bit more explicit? Maybe like:

   "Disabling HugeTLB, no usable page sizes found."

That would confuse when they find in the dmesg

[    0.000000] hash-mmu: Page sizes from device-tree: 

[    0.000000] hash-mmu: base_shift=12: shift=12, sllp=0x0000, 
avpnm=0x00000000, tlbiel=1, penc=0 

[    0.000000] hash-mmu: base_shift=12: shift=16, sllp=0x0000, 
avpnm=0x00000000, tlbiel=1, penc=7 

[    0.000000] hash-mmu: base_shift=12: shift=24, sllp=0x0000, 
avpnm=0x00000000, tlbiel=1, penc=56 

[    0.000000] hash-mmu: base_shift=16: shift=16, sllp=0x0110, 
avpnm=0x00000000, tlbiel=1, penc=1 

[    0.000000] hash-mmu: base_shift=16: shift=24, sllp=0x0110, 
avpnm=0x00000000, tlbiel=1, penc=8 

[    0.000000] hash-mmu: base_shift=24: shift=24, sllp=0x0100, 
avpnm=0x00000001, tlbiel=0, penc=0 

[    0.000000] hash-mmu: base_shift=34: shift=34, sllp=0x0120, 
avpnm=0x000007ff, tlbiel=0, penc=3

-aneesh

On 5/17/19 9:29 AM, Aneesh Kumar K.V wrote:

On 5/16/19 8:17 PM, Michael Ellerman wrote:

quoted

"Aneesh Kumar K.V" [off-list ref] writes:

quoted

This makes sure we don't enable HugeTLB if the cache is not configured.
I am still not sure about this. IMHO hugetlb support should be a 
hardware
support derivative and any cache allocation failure should be handled 
as I did
in the earlier patch. But then if we were not able to create hugetlb 
page table
cache, we can as well declare hugetlb support disabled thereby 
avoiding calling
into allocation routines.

Signed-off-by: Aneesh Kumar K.V <redacted>
---
  arch/powerpc/mm/hugetlbpage.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/hugetlbpage.c 

b/arch/powerpc/mm/hugetlbpage.c
index ee16a3fb788a..4bf8bc659cc7 100644

--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c

@@ -602,6 +602,7 @@ __setup("hugepagesz=", hugepage_setup_sz);

  static int __init hugetlbpage_init(void)
  {
      int psize;
+    bool configured = false;

Where's my reverse Christmas tree! :)

Will fix that :)

quoted

quoted

      if (hugetlb_disabled) {
          pr_info("HugeTLB support is disabled!\n");

@@ -651,10 +652,16 @@ static int __init hugetlbpage_init(void)

              pgtable_cache_add(pdshift - shift);
          else if (IS_ENABLED(CONFIG_PPC_FSL_BOOK3E) || 
IS_ENABLED(CONFIG_PPC_8xx))
              pgtable_cache_add(PTE_T_ORDER);
+
+        if (!configured)
+            configured = true;

I'd just not worry about the if.

quoted

      }
-    if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
-        hugetlbpage_init_default();
+    if (configured) {
+        if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
+            hugetlbpage_init_default();
+    } else
+        pr_info("Disabling HugeTLB");

We're not actually doing anything to disable it in the
CONFIG_HUGETLB_PAGE_SIZE_VARIABLE=n case, but I guess the print is still
correct because we didn't enable a size in the for loop above?

Can we make it a bit more explicit? Maybe like:

   "Disabling HugeTLB, no usable page sizes found."

That would confuse when they find in the dmesg

[    0.000000] hash-mmu: Page sizes from device-tree:
[    0.000000] hash-mmu: base_shift=12: shift=12, sllp=0x0000, 
avpnm=0x00000000, tlbiel=1, penc=0
[    0.000000] hash-mmu: base_shift=12: shift=16, sllp=0x0000, 
avpnm=0x00000000, tlbiel=1, penc=7
[    0.000000] hash-mmu: base_shift=12: shift=24, sllp=0x0000, 
avpnm=0x00000000, tlbiel=1, penc=56
[    0.000000] hash-mmu: base_shift=16: shift=16, sllp=0x0110, 
avpnm=0x00000000, tlbiel=1, penc=1
[    0.000000] hash-mmu: base_shift=16: shift=24, sllp=0x0110, 
avpnm=0x00000000, tlbiel=1, penc=8
[    0.000000] hash-mmu: base_shift=24: shift=24, sllp=0x0100, 
avpnm=0x00000001, tlbiel=0, penc=0
[    0.000000] hash-mmu: base_shift=34: shift=34, sllp=0x0120, 
avpnm=0x000007ff, tlbiel=0, penc=3

There is another failure condition which i am not sure how to handle 
with the pagetable cache creation failures. With above, if we had kernel 
command line hugepagesz=x hugepages=y, and if that x is a gigantic 
hugepage, we will allocate those pages early even if we don't support 
hugetlb because we failed to create page table cache.

I am not sure whether we should handle that error gracefully?

-aneesh

"Aneesh Kumar K.V" [off-list ref] writes:

On 5/16/19 8:17 PM, Michael Ellerman wrote:

quoted

"Aneesh Kumar K.V" [off-list ref] writes:

quoted

This makes sure we don't enable HugeTLB if the cache is not configured.
I am still not sure about this. IMHO hugetlb support should be a hardware
support derivative and any cache allocation failure should be handled as I did
in the earlier patch. But then if we were not able to create hugetlb page table
cache, we can as well declare hugetlb support disabled thereby avoiding calling
into allocation routines.

Signed-off-by: Aneesh Kumar K.V <redacted>
---
  arch/powerpc/mm/hugetlbpage.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index ee16a3fb788a..4bf8bc659cc7 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c

@@ -602,6 +602,7 @@ __setup("hugepagesz=", hugepage_setup_sz);
  static int __init hugetlbpage_init(void)
  {
  	int psize;
+	bool configured = false;

Where's my reverse Christmas tree! :)

Will fix that :)

Thanks.

quoted

quoted

@@ -651,10 +652,16 @@ static int __init hugetlbpage_init(void)
  
-	if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
-		hugetlbpage_init_default();
+	if (configured) {
+		if (IS_ENABLED(CONFIG_HUGETLB_PAGE_SIZE_VARIABLE))
+			hugetlbpage_init_default();
+	} else
+		pr_info("Disabling HugeTLB");

We're not actually doing anything to disable it in the
CONFIG_HUGETLB_PAGE_SIZE_VARIABLE=n case, but I guess the print is still
correct because we didn't enable a size in the for loop above?

Can we make it a bit more explicit? Maybe like:

   "Disabling HugeTLB, no usable page sizes found."

That would confuse when they find in the dmesg

[    0.000000] hash-mmu: Page sizes from device-tree: 
[    0.000000] hash-mmu: base_shift=12: shift=12, sllp=0x0000, avpnm=0x00000000, tlbiel=1, penc=0 
[    0.000000] hash-mmu: base_shift=12: shift=16, sllp=0x0000, avpnm=0x00000000, tlbiel=1, penc=7 
[    0.000000] hash-mmu: base_shift=12: shift=24, sllp=0x0000, avpnm=0x00000000, tlbiel=1, penc=56 
[    0.000000] hash-mmu: base_shift=16: shift=16, sllp=0x0110, avpnm=0x00000000, tlbiel=1, penc=1 
[    0.000000] hash-mmu: base_shift=16: shift=24, sllp=0x0110, avpnm=0x00000000, tlbiel=1, penc=8 
[    0.000000] hash-mmu: base_shift=24: shift=24, sllp=0x0100, avpnm=0x00000001, tlbiel=0, penc=0 
[    0.000000] hash-mmu: base_shift=34: shift=34, sllp=0x0120, avpnm=0x000007ff, tlbiel=0, penc=3

But aren't they going to be even more confused when all we print is
"Disabling HugeTLB" with no explanation?

cheers