| Message ID | 20200812063358.369514-3-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] powerpc/mm: Add DEBUG_VM WARN for pmd_clear | expand |
On 08/12/2020 12:03 PM, Aneesh Kumar K.V wrote: > 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. Even though set_pte_at() is not really a arch page table helper and very much arch specific, I just wonder why this deviation on ppc64 as compared to other platforms. Detecting such semantics variation across platforms is an objective of this test. As small nit. Please follow the existing subject format for all patches in here. It will improve readability and also help understand these changes better, later on. mm/debug_vm_pgtable: <Specify changes to an individual test> > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > mm/debug_vm_pgtable.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 4c32063a8acf..02a7c20aa4a2 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -81,8 +81,6 @@ static void __init pte_advanced_tests(struct mm_struct *mm, > 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)); This makes sense. But could you please fold this code stanza with the previous one in order to imply that 'ptep' did have some valid entry before being cleared and checked against pte_none(). > @@ -97,12 +95,14 @@ static void __init pte_advanced_tests(struct mm_struct *mm, > 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)); Same, please fold back. > > + /* > + * We should clear pte before we do set_pte_at > + */ > + pte = ptep_get_and_clear(mm, vaddr, ptep); > pte = pte_mkyoung(pte); > set_pte_at(mm, vaddr, ptep, pte); > ptep_test_and_clear_young(vma, vaddr, ptep); > The comment above should also explain details that are mentioned in the commit message i.e how platforms such as ppc64 expects a clear pte entry for set_pte_at() to work.
On 8/12/20 2:42 PM, Anshuman Khandual wrote: > > > On 08/12/2020 12:03 PM, Aneesh Kumar K.V wrote: >> 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. > > Even though set_pte_at() is not really a arch page table helper and > very much arch specific, I just wonder why this deviation on ppc64 > as compared to other platforms. Detecting such semantics variation > across platforms is an objective of this test. Not sure what you mean by set_pte_at is not a page table helper. Generic kernel use that helper to set a pte entry. Now w.r.t ppc64 behavior this was discussed multiple times. I guess Linux kernel always used set_pte_at on a none pte entry. We had some exceptions in the recent past. But all fixed when noticed. 383321ab8578dfe3bbcc0bc5604c0f8ae08a5c98 mm/hugetlb/migration: use set_huge_pte_at instead of set_pte_at cee216a696b2004017a5ecb583366093d90b1568 mm/autonuma: don't use set_pte_at when updating protnone ptes 56eecdb912b536a4fa97fb5bfe5a940a54d79be6 mm: Use ptep/pmdp_set_numa() for updating _PAGE_NUMA bit Yes. Having a testcase like this help > > As small nit. > > Please follow the existing subject format for all patches in here. > It will improve readability and also help understand these changes > better, later on. > > mm/debug_vm_pgtable: <Specify changes to an individual test> > >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> mm/debug_vm_pgtable.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index 4c32063a8acf..02a7c20aa4a2 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -81,8 +81,6 @@ static void __init pte_advanced_tests(struct mm_struct *mm, >> 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)); > > This makes sense. But could you please fold this code stanza with > the previous one in order to imply that 'ptep' did have some valid > entry before being cleared and checked against pte_none(). > will do that >> @@ -97,12 +95,14 @@ static void __init pte_advanced_tests(struct mm_struct *mm, >> 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)); > > Same, please fold back. > ok >> + /* >> + * We should clear pte before we do set_pte_at >> + */ >> + pte = ptep_get_and_clear(mm, vaddr, ptep); >> pte = pte_mkyoung(pte); >> set_pte_at(mm, vaddr, ptep, pte); >> ptep_test_and_clear_young(vma, vaddr, ptep); >> > > The comment above should also explain details that are mentioned > in the commit message i.e how platforms such as ppc64 expects a > clear pte entry for set_pte_at() to work. > I don't think it is specific to ppc64. There is nothing specific to ppc64 architecture in there. It is an optimization used in kernel to help architecture avoid TLB flush. I will update the comment as below /* We should clear pte before we do set_pte_at so that set_pte_at don't find a valid pte at ptep *? is that good? -aneesh
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 4c32063a8acf..02a7c20aa4a2 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -81,8 +81,6 @@ static void __init pte_advanced_tests(struct mm_struct *mm, 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)); @@ -97,12 +95,14 @@ static void __init pte_advanced_tests(struct mm_struct *mm, 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)); + /* + * We should clear pte before we do set_pte_at + */ + pte = ptep_get_and_clear(mm, vaddr, ptep); pte = pte_mkyoung(pte); set_pte_at(mm, vaddr, ptep, pte); ptep_test_and_clear_young(vma, vaddr, ptep);
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 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)