| Message ID | 20190514145041.7836-3-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,1/3] powerpc/mm: Handle page table allocation failures | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (8150a153c013aa2dd1ffae43370b89ac1347a7fb) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 2 checks, 25 lines checked |
"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
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
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
"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
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; }
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(-)