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

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

[Aneesh Kumar K.V]

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 <aneesh.kumar@linux.ibm.com>
---
 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

[RFC PATCH 2/3] powerpc/mm/hugetlb: Fix kernel crash if we fail to allocate page table caches

[Aneesh Kumar K.V]

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 <aneesh.kumar@linux.ibm.com>
---
 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

[RFC PATCH 3/3] powerpc/mm/hugetlb: Don't enable HugeTLB if we don't have a page table cache

[Aneesh Kumar K.V]

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 <aneesh.kumar@linux.ibm.com>
---
 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

Re: [RFC PATCH 3/3] powerpc/mm/hugetlb: Don't enable HugeTLB if we don't have a page table cache

[Michael Ellerman]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> 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 <aneesh.kumar@linux.ibm.com>
> ---
>  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! :)

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

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

Re: [RFC PATCH 3/3] powerpc/mm/hugetlb: Don't enable HugeTLB if we don't have a page table cache

[Aneesh Kumar K.V]

On 5/16/19 8:17 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> 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 <aneesh.kumar@linux.ibm.com>
>> ---
>>   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 :)

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

Re: [RFC PATCH 3/3] powerpc/mm/hugetlb: Don't enable HugeTLB if we don't have a page table cache

[Aneesh Kumar K.V]

On 5/17/19 9:29 AM, Aneesh Kumar K.V wrote:
> On 5/16/19 8:17 PM, Michael Ellerman wrote:
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>> 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 <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   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 :)
> 
>>
>>>       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.
>>
>>>       }
>>> -    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

Re: [RFC PATCH 3/3] powerpc/mm/hugetlb: Don't enable HugeTLB if we don't have a page table cache

[Michael Ellerman]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> On 5/16/19 8:17 PM, Michael Ellerman wrote:
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>> 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 <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   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.

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

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

[Tyrel Datwyler]

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

> 
> Fixes: e2b3d202d1db ("powerpc: Switch 16GB and 16MB explicit hugepages to a different page table format")
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  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;
>  		}
>