| Message ID | 20200819130107.478414-8-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/debug_vm_pgtable fixes | expand |
Le 19/08/2020 à 15:01, Aneesh Kumar K.V a écrit : > set_pte_at() should not be used to set a pte entry at locations that > already holds a valid pte entry. Architectures like ppc64 don't do TLB > invalidate in set_pte_at() and hence expect it to be used to set locations > that are not a valid PTE. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > mm/debug_vm_pgtable.c | 35 +++++++++++++++-------------------- > 1 file changed, 15 insertions(+), 20 deletions(-) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 76f4c713e5a3..9c7e2c9cfc76 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -74,15 +74,18 @@ static void __init pte_advanced_tests(struct mm_struct *mm, > { > pte_t pte = pfn_pte(pfn, prot); > > + /* > + * Architectures optimize set_pte_at by avoiding TLB flush. > + * This requires set_pte_at to be not used to update an > + * existing pte entry. Clear pte before we do set_pte_at > + */ > + > pr_debug("Validating PTE advanced\n"); > pte = pfn_pte(pfn, prot); > set_pte_at(mm, vaddr, ptep, pte); > ptep_set_wrprotect(mm, vaddr, ptep); > pte = ptep_get(ptep); > WARN_ON(pte_write(pte)); > - > - pte = pfn_pte(pfn, prot); > - set_pte_at(mm, vaddr, ptep, pte); > ptep_get_and_clear(mm, vaddr, ptep); > pte = ptep_get(ptep); > WARN_ON(!pte_none(pte)); > @@ -96,13 +99,11 @@ static void __init pte_advanced_tests(struct mm_struct *mm, > ptep_set_access_flags(vma, vaddr, ptep, pte, 1); > pte = ptep_get(ptep); > WARN_ON(!(pte_write(pte) && pte_dirty(pte))); > - > - pte = pfn_pte(pfn, prot); > - set_pte_at(mm, vaddr, ptep, pte); > ptep_get_and_clear_full(mm, vaddr, ptep, 1); > pte = ptep_get(ptep); > WARN_ON(!pte_none(pte)); > > + pte = pfn_pte(pfn, prot); > pte = pte_mkyoung(pte); > set_pte_at(mm, vaddr, ptep, pte); > ptep_test_and_clear_young(vma, vaddr, ptep); > @@ -164,9 +165,6 @@ static void __init pmd_advanced_tests(struct mm_struct *mm, > pmdp_set_wrprotect(mm, vaddr, pmdp); > pmd = READ_ONCE(*pmdp); > WARN_ON(pmd_write(pmd)); > - > - pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); > - set_pmd_at(mm, vaddr, pmdp, pmd); > pmdp_huge_get_and_clear(mm, vaddr, pmdp); > pmd = READ_ONCE(*pmdp); > WARN_ON(!pmd_none(pmd)); > @@ -180,13 +178,11 @@ static void __init pmd_advanced_tests(struct mm_struct *mm, > pmdp_set_access_flags(vma, vaddr, pmdp, pmd, 1); > pmd = READ_ONCE(*pmdp); > WARN_ON(!(pmd_write(pmd) && pmd_dirty(pmd))); > - > - pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); > - set_pmd_at(mm, vaddr, pmdp, pmd); > pmdp_huge_get_and_clear_full(vma, vaddr, pmdp, 1); > pmd = READ_ONCE(*pmdp); > WARN_ON(!pmd_none(pmd)); > > + pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); > pmd = pmd_mkyoung(pmd); > set_pmd_at(mm, vaddr, pmdp, pmd); > pmdp_test_and_clear_young(vma, vaddr, pmdp); > @@ -283,18 +279,10 @@ static void __init pud_advanced_tests(struct mm_struct *mm, > WARN_ON(pud_write(pud)); > > #ifndef __PAGETABLE_PMD_FOLDED Same as below, once set_put_at() is gone, I don't think this #ifndef __PAGETABLE_PMD_FOLDED is still need, should be possible to replace by 'if (mm_pmd_folded())' > - > - pud = pud_mkhuge(pfn_pud(pfn, prot)); > - set_pud_at(mm, vaddr, pudp, pud); > pudp_huge_get_and_clear(mm, vaddr, pudp); > pud = READ_ONCE(*pudp); > WARN_ON(!pud_none(pud)); > > - pud = pud_mkhuge(pfn_pud(pfn, prot)); > - set_pud_at(mm, vaddr, pudp, pud); > - pudp_huge_get_and_clear_full(mm, vaddr, pudp, 1); > - pud = READ_ONCE(*pudp); > - WARN_ON(!pud_none(pud)); > #endif /* __PAGETABLE_PMD_FOLDED */ > > pud = pud_mkhuge(pfn_pud(pfn, prot)); > @@ -307,6 +295,13 @@ static void __init pud_advanced_tests(struct mm_struct *mm, > pud = READ_ONCE(*pudp); > WARN_ON(!(pud_write(pud) && pud_dirty(pud))); > > +#ifndef __PAGETABLE_PMD_FOLDED > + pudp_huge_get_and_clear_full(vma, vaddr, pudp, 1); > + pud = READ_ONCE(*pudp); > + WARN_ON(!pud_none(pud)); > +#endif /* __PAGETABLE_PMD_FOLDED */ pudp_huge_get_and_clear_full() and pud_none() are always defined, I think this #ifndef can be replaced by an 'if (mm_pmd_folded())' > + > + pud = pud_mkhuge(pfn_pud(pfn, prot)); > pud = pud_mkyoung(pud); > set_pud_at(mm, vaddr, pudp, pud); > pudp_test_and_clear_young(vma, vaddr, pudp); > Christophe
On 8/20/20 8:02 PM, Christophe Leroy wrote: > > > Le 19/08/2020 à 15:01, Aneesh Kumar K.V a écrit : >> set_pte_at() should not be used to set a pte entry at locations that >> already holds a valid pte entry. Architectures like ppc64 don't do TLB >> invalidate in set_pte_at() and hence expect it to be used to set >> locations >> that are not a valid PTE. >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> mm/debug_vm_pgtable.c | 35 +++++++++++++++-------------------- >> 1 file changed, 15 insertions(+), 20 deletions(-) >> >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index 76f4c713e5a3..9c7e2c9cfc76 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -74,15 +74,18 @@ static void __init pte_advanced_tests(struct >> mm_struct *mm, >> { >> pte_t pte = pfn_pte(pfn, prot); >> + /* >> + * Architectures optimize set_pte_at by avoiding TLB flush. >> + * This requires set_pte_at to be not used to update an >> + * existing pte entry. Clear pte before we do set_pte_at >> + */ >> + >> pr_debug("Validating PTE advanced\n"); >> pte = pfn_pte(pfn, prot); >> set_pte_at(mm, vaddr, ptep, pte); >> ptep_set_wrprotect(mm, vaddr, ptep); >> pte = ptep_get(ptep); >> WARN_ON(pte_write(pte)); >> - >> - pte = pfn_pte(pfn, prot); >> - set_pte_at(mm, vaddr, ptep, pte); >> ptep_get_and_clear(mm, vaddr, ptep); >> pte = ptep_get(ptep); >> WARN_ON(!pte_none(pte)); >> @@ -96,13 +99,11 @@ static void __init pte_advanced_tests(struct >> mm_struct *mm, >> ptep_set_access_flags(vma, vaddr, ptep, pte, 1); >> pte = ptep_get(ptep); >> WARN_ON(!(pte_write(pte) && pte_dirty(pte))); >> - >> - pte = pfn_pte(pfn, prot); >> - set_pte_at(mm, vaddr, ptep, pte); >> ptep_get_and_clear_full(mm, vaddr, ptep, 1); >> pte = ptep_get(ptep); >> WARN_ON(!pte_none(pte)); >> + pte = pfn_pte(pfn, prot); >> pte = pte_mkyoung(pte); >> set_pte_at(mm, vaddr, ptep, pte); >> ptep_test_and_clear_young(vma, vaddr, ptep); >> @@ -164,9 +165,6 @@ static void __init pmd_advanced_tests(struct >> mm_struct *mm, >> pmdp_set_wrprotect(mm, vaddr, pmdp); >> pmd = READ_ONCE(*pmdp); >> WARN_ON(pmd_write(pmd)); >> - >> - pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); >> - set_pmd_at(mm, vaddr, pmdp, pmd); >> pmdp_huge_get_and_clear(mm, vaddr, pmdp); >> pmd = READ_ONCE(*pmdp); >> WARN_ON(!pmd_none(pmd)); >> @@ -180,13 +178,11 @@ static void __init pmd_advanced_tests(struct >> mm_struct *mm, >> pmdp_set_access_flags(vma, vaddr, pmdp, pmd, 1); >> pmd = READ_ONCE(*pmdp); >> WARN_ON(!(pmd_write(pmd) && pmd_dirty(pmd))); >> - >> - pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); >> - set_pmd_at(mm, vaddr, pmdp, pmd); >> pmdp_huge_get_and_clear_full(vma, vaddr, pmdp, 1); >> pmd = READ_ONCE(*pmdp); >> WARN_ON(!pmd_none(pmd)); >> + pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); >> pmd = pmd_mkyoung(pmd); >> set_pmd_at(mm, vaddr, pmdp, pmd); >> pmdp_test_and_clear_young(vma, vaddr, pmdp); >> @@ -283,18 +279,10 @@ static void __init pud_advanced_tests(struct >> mm_struct *mm, >> WARN_ON(pud_write(pud)); >> #ifndef __PAGETABLE_PMD_FOLDED > > Same as below, once set_put_at() is gone, I don't think this #ifndef > __PAGETABLE_PMD_FOLDED is still need, should be possible to replace by > 'if (mm_pmd_folded())' I would skip that change in this series because I still haven't worked out what it means to have FOLDED PMD with CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD. We should probably push that as a cleanup later and somebody who can test that config can do that? Currently i can't boot ppc64 with DBUG_VM_PGTABLE enabled on ppc64 because it is all buggy w.r.t rules. -aneesh
On 08/21/2020 12:44 PM, Aneesh Kumar K.V wrote: > On 8/20/20 8:02 PM, Christophe Leroy wrote: >> >> >> Le 19/08/2020 à 15:01, Aneesh Kumar K.V a écrit : >>> set_pte_at() should not be used to set a pte entry at locations that >>> already holds a valid pte entry. Architectures like ppc64 don't do TLB >>> invalidate in set_pte_at() and hence expect it to be used to set locations >>> that are not a valid PTE. >>> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>> --- >>> mm/debug_vm_pgtable.c | 35 +++++++++++++++-------------------- >>> 1 file changed, 15 insertions(+), 20 deletions(-) >>> >>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>> index 76f4c713e5a3..9c7e2c9cfc76 100644 >>> --- a/mm/debug_vm_pgtable.c >>> +++ b/mm/debug_vm_pgtable.c >>> @@ -74,15 +74,18 @@ static void __init pte_advanced_tests(struct mm_struct *mm, >>> { >>> pte_t pte = pfn_pte(pfn, prot); >>> + /* >>> + * Architectures optimize set_pte_at by avoiding TLB flush. >>> + * This requires set_pte_at to be not used to update an >>> + * existing pte entry. Clear pte before we do set_pte_at >>> + */ >>> + >>> pr_debug("Validating PTE advanced\n"); >>> pte = pfn_pte(pfn, prot); >>> set_pte_at(mm, vaddr, ptep, pte); >>> ptep_set_wrprotect(mm, vaddr, ptep); >>> pte = ptep_get(ptep); >>> WARN_ON(pte_write(pte)); >>> - >>> - pte = pfn_pte(pfn, prot); >>> - set_pte_at(mm, vaddr, ptep, pte); >>> ptep_get_and_clear(mm, vaddr, ptep); >>> pte = ptep_get(ptep); >>> WARN_ON(!pte_none(pte)); >>> @@ -96,13 +99,11 @@ static void __init pte_advanced_tests(struct mm_struct *mm, >>> ptep_set_access_flags(vma, vaddr, ptep, pte, 1); >>> pte = ptep_get(ptep); >>> WARN_ON(!(pte_write(pte) && pte_dirty(pte))); >>> - >>> - pte = pfn_pte(pfn, prot); >>> - set_pte_at(mm, vaddr, ptep, pte); >>> ptep_get_and_clear_full(mm, vaddr, ptep, 1); >>> pte = ptep_get(ptep); >>> WARN_ON(!pte_none(pte)); >>> + pte = pfn_pte(pfn, prot); >>> pte = pte_mkyoung(pte); >>> set_pte_at(mm, vaddr, ptep, pte); >>> ptep_test_and_clear_young(vma, vaddr, ptep); >>> @@ -164,9 +165,6 @@ static void __init pmd_advanced_tests(struct mm_struct *mm, >>> pmdp_set_wrprotect(mm, vaddr, pmdp); >>> pmd = READ_ONCE(*pmdp); >>> WARN_ON(pmd_write(pmd)); >>> - >>> - pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); >>> - set_pmd_at(mm, vaddr, pmdp, pmd); >>> pmdp_huge_get_and_clear(mm, vaddr, pmdp); >>> pmd = READ_ONCE(*pmdp); >>> WARN_ON(!pmd_none(pmd)); >>> @@ -180,13 +178,11 @@ static void __init pmd_advanced_tests(struct mm_struct *mm, >>> pmdp_set_access_flags(vma, vaddr, pmdp, pmd, 1); >>> pmd = READ_ONCE(*pmdp); >>> WARN_ON(!(pmd_write(pmd) && pmd_dirty(pmd))); >>> - >>> - pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); >>> - set_pmd_at(mm, vaddr, pmdp, pmd); >>> pmdp_huge_get_and_clear_full(vma, vaddr, pmdp, 1); >>> pmd = READ_ONCE(*pmdp); >>> WARN_ON(!pmd_none(pmd)); >>> + pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); >>> pmd = pmd_mkyoung(pmd); >>> set_pmd_at(mm, vaddr, pmdp, pmd); >>> pmdp_test_and_clear_young(vma, vaddr, pmdp); >>> @@ -283,18 +279,10 @@ static void __init pud_advanced_tests(struct mm_struct *mm, >>> WARN_ON(pud_write(pud)); >>> #ifndef __PAGETABLE_PMD_FOLDED >> >> Same as below, once set_put_at() is gone, I don't think this #ifndef __PAGETABLE_PMD_FOLDED is still need, should be possible to replace by 'if (mm_pmd_folded())' > > I would skip that change in this series because I still haven't worked out what it means to have FOLDED PMD with CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD. > > > We should probably push that as a cleanup later and somebody who can test that config can do that? Currently i can't boot ppc64 with DBUG_VM_PGTABLE enabled on ppc64 because it is all buggy w.r.t rules. Agreed. I think its OK not address these changes/improvements in this particular series which is trying to modify the test to make it run on ppc64 platform. I will probably look into that later.
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 76f4c713e5a3..9c7e2c9cfc76 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -74,15 +74,18 @@ static void __init pte_advanced_tests(struct mm_struct *mm, { pte_t pte = pfn_pte(pfn, prot); + /* + * Architectures optimize set_pte_at by avoiding TLB flush. + * This requires set_pte_at to be not used to update an + * existing pte entry. Clear pte before we do set_pte_at + */ + pr_debug("Validating PTE advanced\n"); pte = pfn_pte(pfn, prot); set_pte_at(mm, vaddr, ptep, pte); ptep_set_wrprotect(mm, vaddr, ptep); pte = ptep_get(ptep); WARN_ON(pte_write(pte)); - - pte = pfn_pte(pfn, prot); - set_pte_at(mm, vaddr, ptep, pte); ptep_get_and_clear(mm, vaddr, ptep); pte = ptep_get(ptep); WARN_ON(!pte_none(pte)); @@ -96,13 +99,11 @@ static void __init pte_advanced_tests(struct mm_struct *mm, ptep_set_access_flags(vma, vaddr, ptep, pte, 1); pte = ptep_get(ptep); WARN_ON(!(pte_write(pte) && pte_dirty(pte))); - - pte = pfn_pte(pfn, prot); - set_pte_at(mm, vaddr, ptep, pte); ptep_get_and_clear_full(mm, vaddr, ptep, 1); pte = ptep_get(ptep); WARN_ON(!pte_none(pte)); + pte = pfn_pte(pfn, prot); pte = pte_mkyoung(pte); set_pte_at(mm, vaddr, ptep, pte); ptep_test_and_clear_young(vma, vaddr, ptep); @@ -164,9 +165,6 @@ static void __init pmd_advanced_tests(struct mm_struct *mm, pmdp_set_wrprotect(mm, vaddr, pmdp); pmd = READ_ONCE(*pmdp); WARN_ON(pmd_write(pmd)); - - pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); - set_pmd_at(mm, vaddr, pmdp, pmd); pmdp_huge_get_and_clear(mm, vaddr, pmdp); pmd = READ_ONCE(*pmdp); WARN_ON(!pmd_none(pmd)); @@ -180,13 +178,11 @@ static void __init pmd_advanced_tests(struct mm_struct *mm, pmdp_set_access_flags(vma, vaddr, pmdp, pmd, 1); pmd = READ_ONCE(*pmdp); WARN_ON(!(pmd_write(pmd) && pmd_dirty(pmd))); - - pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); - set_pmd_at(mm, vaddr, pmdp, pmd); pmdp_huge_get_and_clear_full(vma, vaddr, pmdp, 1); pmd = READ_ONCE(*pmdp); WARN_ON(!pmd_none(pmd)); + pmd = pmd_mkhuge(pfn_pmd(pfn, prot)); pmd = pmd_mkyoung(pmd); set_pmd_at(mm, vaddr, pmdp, pmd); pmdp_test_and_clear_young(vma, vaddr, pmdp); @@ -283,18 +279,10 @@ static void __init pud_advanced_tests(struct mm_struct *mm, WARN_ON(pud_write(pud)); #ifndef __PAGETABLE_PMD_FOLDED - - pud = pud_mkhuge(pfn_pud(pfn, prot)); - set_pud_at(mm, vaddr, pudp, pud); pudp_huge_get_and_clear(mm, vaddr, pudp); pud = READ_ONCE(*pudp); WARN_ON(!pud_none(pud)); - pud = pud_mkhuge(pfn_pud(pfn, prot)); - set_pud_at(mm, vaddr, pudp, pud); - pudp_huge_get_and_clear_full(mm, vaddr, pudp, 1); - pud = READ_ONCE(*pudp); - WARN_ON(!pud_none(pud)); #endif /* __PAGETABLE_PMD_FOLDED */ pud = pud_mkhuge(pfn_pud(pfn, prot)); @@ -307,6 +295,13 @@ static void __init pud_advanced_tests(struct mm_struct *mm, pud = READ_ONCE(*pudp); WARN_ON(!(pud_write(pud) && pud_dirty(pud))); +#ifndef __PAGETABLE_PMD_FOLDED + pudp_huge_get_and_clear_full(vma, vaddr, pudp, 1); + pud = READ_ONCE(*pudp); + WARN_ON(!pud_none(pud)); +#endif /* __PAGETABLE_PMD_FOLDED */ + + pud = pud_mkhuge(pfn_pud(pfn, prot)); pud = pud_mkyoung(pud); set_pud_at(mm, vaddr, pudp, pud); pudp_test_and_clear_young(vma, vaddr, pudp);
set_pte_at() should not be used to set a pte entry at locations that already holds a valid pte entry. Architectures like ppc64 don't do TLB invalidate in set_pte_at() and hence expect it to be used to set locations that are not a valid PTE. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- mm/debug_vm_pgtable.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-)