| Message ID | 20240125092624.537564-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/mm: Code cleanup for __hash_page_thp | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Kunwu Chan <chentao@kylinos.cn> writes: > This part was commented from commit 6d492ecc6489 > ("powerpc/THP: Add code to handle HPTE faults for hugepages") > in about 11 years before. > > If there are no plans to enable this part code in the future, > we can remove this dead code. I agree the code can go. But I'd like it to be replaced with a comment explaining what the dead code was trying to say. cheers > diff --git a/arch/powerpc/mm/book3s64/hash_hugepage.c b/arch/powerpc/mm/book3s64/hash_hugepage.c > index c0fabe6c5a12..127a3a2c174b 100644 > --- a/arch/powerpc/mm/book3s64/hash_hugepage.c > +++ b/arch/powerpc/mm/book3s64/hash_hugepage.c > @@ -59,16 +59,6 @@ int __hash_page_thp(unsigned long ea, unsigned long access, unsigned long vsid, > > rflags = htab_convert_pte_flags(new_pmd, flags); > > -#if 0 > - if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE)) { > - > - /* > - * No CPU has hugepages but lacks no execute, so we > - * don't need to worry about that case > - */ > - rflags = hash_page_do_lazy_icache(rflags, __pte(old_pte), trap); > - } > -#endif > /* > * Find the slot index details for this ea, using base page size. > */ > -- > 2.39.2
Thanks for the reply. On 2024/2/26 18:49, Michael Ellerman wrote: > Kunwu Chan <chentao@kylinos.cn> writes: >> This part was commented from commit 6d492ecc6489 >> ("powerpc/THP: Add code to handle HPTE faults for hugepages") >> in about 11 years before. >> >> If there are no plans to enable this part code in the future, >> we can remove this dead code. > > I agree the code can go. But I'd like it to be replaced with a comment > explaining what the dead code was trying to say. Thanks, i'll update a new patch with the following comment: /* * No CPU has hugepages but lacks no execute, so we * don't need to worry about cpu no CPU_FTR_COHERENT_ICACHE feature case */ > > cheers > >> diff --git a/arch/powerpc/mm/book3s64/hash_hugepage.c b/arch/powerpc/mm/book3s64/hash_hugepage.c >> index c0fabe6c5a12..127a3a2c174b 100644 >> --- a/arch/powerpc/mm/book3s64/hash_hugepage.c >> +++ b/arch/powerpc/mm/book3s64/hash_hugepage.c >> @@ -59,16 +59,6 @@ int __hash_page_thp(unsigned long ea, unsigned long access, unsigned long vsid, >> >> rflags = htab_convert_pte_flags(new_pmd, flags); >> >> -#if 0 >> - if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE)) { >> - >> - /* >> - * No CPU has hugepages but lacks no execute, so we >> - * don't need to worry about that case >> - */ >> - rflags = hash_page_do_lazy_icache(rflags, __pte(old_pte), trap); >> - } >> -#endif >> /* >> * Find the slot index details for this ea, using base page size. >> */ >> -- >> 2.39.2
Kunwu Chan <chentao@kylinos.cn> writes: > Thanks for the reply. > > On 2024/2/26 18:49, Michael Ellerman wrote: >> Kunwu Chan <chentao@kylinos.cn> writes: >>> This part was commented from commit 6d492ecc6489 >>> ("powerpc/THP: Add code to handle HPTE faults for hugepages") >>> in about 11 years before. >>> >>> If there are no plans to enable this part code in the future, >>> we can remove this dead code. >> >> I agree the code can go. But I'd like it to be replaced with a comment >> explaining what the dead code was trying to say. > Thanks, i'll update a new patch with the following comment: > /* > * No CPU has hugepages but lacks no execute, so we > * don't need to worry about cpu no CPU_FTR_COHERENT_ICACHE feature case > */ Maybe wait until we can get some input from Aneesh. I'm not sure the code/comment are really up to date. cheers
Thanks for the reply. On 2024/2/27 14:07, Michael Ellerman wrote: > Kunwu Chan <chentao@kylinos.cn> writes: >> Thanks for the reply. >> >> On 2024/2/26 18:49, Michael Ellerman wrote: >>> Kunwu Chan <chentao@kylinos.cn> writes: >>>> This part was commented from commit 6d492ecc6489 >>>> ("powerpc/THP: Add code to handle HPTE faults for hugepages") >>>> in about 11 years before. >>>> >>>> If there are no plans to enable this part code in the future, >>>> we can remove this dead code. >>> >>> I agree the code can go. But I'd like it to be replaced with a comment >>> explaining what the dead code was trying to say. > >> Thanks, i'll update a new patch with the following comment: >> /* >> * No CPU has hugepages but lacks no execute, so we >> * don't need to worry about cpu no CPU_FTR_COHERENT_ICACHE feature case >> */ > > Maybe wait until we can get some input from Aneesh. I'm not sure the > code/comment are really up to date. I won't do anything until I get a reply. I'll wait for the latest msg. > > cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > Kunwu Chan <chentao@kylinos.cn> writes: >> Thanks for the reply. >> >> On 2024/2/26 18:49, Michael Ellerman wrote: >>> Kunwu Chan <chentao@kylinos.cn> writes: >>>> This part was commented from commit 6d492ecc6489 >>>> ("powerpc/THP: Add code to handle HPTE faults for hugepages") >>>> in about 11 years before. >>>> >>>> If there are no plans to enable this part code in the future, >>>> we can remove this dead code. >>> >>> I agree the code can go. But I'd like it to be replaced with a comment >>> explaining what the dead code was trying to say. > >> Thanks, i'll update a new patch with the following comment: >> /* >> * No CPU has hugepages but lacks no execute, so we >> * don't need to worry about cpu no CPU_FTR_COHERENT_ICACHE feature case >> */ > > Maybe wait until we can get some input from Aneesh. I'm not sure the > code/comment are really up to date. > > cheers How about? modified arch/powerpc/mm/book3s64/hash_hugepage.c @@ -58,17 +58,13 @@ int __hash_page_thp(unsigned long ea, unsigned long access, unsigned long vsid, return 0; rflags = htab_convert_pte_flags(new_pmd, flags); + /* + * THPs are only supported on platforms that can do mixed page size + * segments (MPSS) and all such platforms have coherent icache. Hence we + * don't need to do lazy icache flush (hash_page_do_lazy_icache()) on + * noexecute fault. + */ -#if 0 - if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE)) { - - /* - * No CPU has hugepages but lacks no execute, so we - * don't need to worry about that case - */ - rflags = hash_page_do_lazy_icache(rflags, __pte(old_pte), trap); - } -#endif /* * Find the slot index details for this ea, using base page size. */
Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes: > Michael Ellerman <mpe@ellerman.id.au> writes: >> Kunwu Chan <chentao@kylinos.cn> writes: >>> On 2024/2/26 18:49, Michael Ellerman wrote: >>>> Kunwu Chan <chentao@kylinos.cn> writes: >>>>> This part was commented from commit 6d492ecc6489 >>>>> ("powerpc/THP: Add code to handle HPTE faults for hugepages") >>>>> in about 11 years before. >>>>> >>>>> If there are no plans to enable this part code in the future, >>>>> we can remove this dead code. >>>> >>>> I agree the code can go. But I'd like it to be replaced with a comment >>>> explaining what the dead code was trying to say. >> >>> Thanks, i'll update a new patch with the following comment: >>> /* >>> * No CPU has hugepages but lacks no execute, so we >>> * don't need to worry about cpu no CPU_FTR_COHERENT_ICACHE feature case >>> */ >> >> Maybe wait until we can get some input from Aneesh. I'm not sure the >> code/comment are really up to date. > > How about? > > modified arch/powerpc/mm/book3s64/hash_hugepage.c > @@ -58,17 +58,13 @@ int __hash_page_thp(unsigned long ea, unsigned long access, unsigned long vsid, > return 0; > > rflags = htab_convert_pte_flags(new_pmd, flags); > + /* > + * THPs are only supported on platforms that can do mixed page size > + * segments (MPSS) and all such platforms have coherent icache. Hence we > + * don't need to do lazy icache flush (hash_page_do_lazy_icache()) on > + * noexecute fault. > + */ Yeah thanks that looks good. It could say "see eg. __hash_page_4K()", but that's probably unnecessary as it mentions hash_page_do_lazy_icache(), and anyone interested is just going to grep for that anyway. cheers
Thanks all for the reply. On 2024/2/29 14:18, Michael Ellerman wrote: > Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes: >> Michael Ellerman <mpe@ellerman.id.au> writes: >>> Kunwu Chan <chentao@kylinos.cn> writes: >>>> On 2024/2/26 18:49, Michael Ellerman wrote: >>>>> Kunwu Chan <chentao@kylinos.cn> writes: >>>>>> This part was commented from commit 6d492ecc6489 >>>>>> ("powerpc/THP: Add code to handle HPTE faults for hugepages") >>>>>> in about 11 years before. >>>>>> >>>>>> If there are no plans to enable this part code in the future, >>>>>> we can remove this dead code. >>>>> >>>>> I agree the code can go. But I'd like it to be replaced with a comment >>>>> explaining what the dead code was trying to say. >>> >>>> Thanks, i'll update a new patch with the following comment: >>>> /* >>>> * No CPU has hugepages but lacks no execute, so we >>>> * don't need to worry about cpu no CPU_FTR_COHERENT_ICACHE feature case >>>> */ >>> >>> Maybe wait until we can get some input from Aneesh. I'm not sure the >>> code/comment are really up to date. >> >> How about? >> >> modified arch/powerpc/mm/book3s64/hash_hugepage.c >> @@ -58,17 +58,13 @@ int __hash_page_thp(unsigned long ea, unsigned long access, unsigned long vsid, >> return 0; >> >> rflags = htab_convert_pte_flags(new_pmd, flags); >> + /* >> + * THPs are only supported on platforms that can do mixed page size >> + * segments (MPSS) and all such platforms have coherent icache. Hence we >> + * don't need to do lazy icache flush (hash_page_do_lazy_icache()) on >> + * noexecute fault. >> + */ > I'll use this comment in v2 patch. And add two Suggested-by: label for you. > Yeah thanks that looks good. > > It could say "see eg. __hash_page_4K()", but that's probably unnecessary > as it mentions hash_page_do_lazy_icache(), and anyone interested is just > going to grep for that anyway. > > cheers
diff --git a/arch/powerpc/mm/book3s64/hash_hugepage.c b/arch/powerpc/mm/book3s64/hash_hugepage.c index c0fabe6c5a12..127a3a2c174b 100644 --- a/arch/powerpc/mm/book3s64/hash_hugepage.c +++ b/arch/powerpc/mm/book3s64/hash_hugepage.c @@ -59,16 +59,6 @@ int __hash_page_thp(unsigned long ea, unsigned long access, unsigned long vsid, rflags = htab_convert_pte_flags(new_pmd, flags); -#if 0 - if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE)) { - - /* - * No CPU has hugepages but lacks no execute, so we - * don't need to worry about that case - */ - rflags = hash_page_do_lazy_icache(rflags, __pte(old_pte), trap); - } -#endif /* * Find the slot index details for this ea, using base page size. */
This part was commented from commit 6d492ecc6489 ("powerpc/THP: Add code to handle HPTE faults for hugepages") in about 11 years before. If there are no plans to enable this part code in the future, we can remove this dead code. Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- arch/powerpc/mm/book3s64/hash_hugepage.c | 10 ---------- 1 file changed, 10 deletions(-)