| Message ID | 20200827080438.315345-10-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/debug_vm_pgtable fixes | expand |
On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: > This will help in adding proper locks in a later patch It really makes sense to classify these tests here as static and dynamic. Static are the ones that test via page table entry values modification without changing anything on the actual page table itself. Where as the dynamic tests do change the page table. Static tests would probably never require any lock synchronization but the dynamic ones will do. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++------------------- > 1 file changed, 29 insertions(+), 23 deletions(-) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 0ce5c6a24c5b..78c8af3445ac 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void) > p4dp = p4d_alloc(mm, pgdp, vaddr); > pudp = pud_alloc(mm, p4dp, vaddr); > pmdp = pmd_alloc(mm, pudp, vaddr); > - ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); > + ptep = pte_alloc_map(mm, pmdp, vaddr); > > /* > * Save all the page table page addresses as the page table > @@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void) > p4d_basic_tests(p4d_aligned, prot); > pgd_basic_tests(pgd_aligned, prot); > > - pte_clear_tests(mm, ptep, vaddr); > - pmd_clear_tests(mm, pmdp); > - pud_clear_tests(mm, pudp); > - p4d_clear_tests(mm, p4dp); > - pgd_clear_tests(mm, pgdp); > - > - pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); > - pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep); > - pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot); > - hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); > - > pmd_leaf_tests(pmd_aligned, prot); > pud_leaf_tests(pud_aligned, prot); > > - pmd_huge_tests(pmdp, pmd_aligned, prot); > - pud_huge_tests(pudp, pud_aligned, prot); > - > pte_savedwrite_tests(pte_aligned, protnone); > pmd_savedwrite_tests(pmd_aligned, protnone); > > - pte_unmap_unlock(ptep, ptl); > - > - pmd_populate_tests(mm, pmdp, saved_ptep); > - pud_populate_tests(mm, pudp, saved_pmdp); > - p4d_populate_tests(mm, p4dp, saved_pudp); > - pgd_populate_tests(mm, pgdp, saved_p4dp); > - > pte_special_tests(pte_aligned, prot); > pte_protnone_tests(pte_aligned, protnone); > pmd_protnone_tests(pmd_aligned, protnone); > @@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void) > pmd_swap_tests(pmd_aligned, prot); > > swap_migration_tests(); > - hugetlb_basic_tests(pte_aligned, prot); > > pmd_thp_tests(pmd_aligned, prot); > pud_thp_tests(pud_aligned, prot); > > + /* > + * Page table modifying tests > + */ In this comment, please do add some more details about how these tests need proper locking mechanism unlike the previous static ones. Also add a similar comment section for the static tests that dont really change page table and need not require corresponding locking mechanism. Both comment sections will make this classification clear. > + pte_clear_tests(mm, ptep, vaddr); > + pmd_clear_tests(mm, pmdp); > + pud_clear_tests(mm, pudp); > + p4d_clear_tests(mm, p4dp); > + pgd_clear_tests(mm, pgdp); > + > + ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); > + pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); > + pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep); > + pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot); > + hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); > + > + > + pmd_huge_tests(pmdp, pmd_aligned, prot); > + pud_huge_tests(pudp, pud_aligned, prot); > + > + pte_unmap_unlock(ptep, ptl); > + > + pmd_populate_tests(mm, pmdp, saved_ptep); > + pud_populate_tests(mm, pudp, saved_pmdp); > + p4d_populate_tests(mm, p4dp, saved_pudp); > + pgd_populate_tests(mm, pgdp, saved_p4dp); > + > + hugetlb_basic_tests(pte_aligned, prot); hugetlb_basic_tests() is a non page table modifying static test and should be classified accordingly. > + > p4d_free(mm, saved_p4dp); > pud_free(mm, saved_pudp); > pmd_free(mm, saved_pmdp); > Changes till this patch fails to boot on arm64 platform. This should be folded with the next patch. [ 17.080644] ------------[ cut here ]------------ [ 17.081342] kernel BUG at mm/pgtable-generic.c:164! [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP [ 17.082977] Modules linked in: [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14 [ 17.084998] Hardware name: linux,dummy-virt (DT) [ 17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--) [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60 [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0 [ 17.088168] sp : ffff80001219bcf0 [ 17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000 [ 17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3 [ 17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0 [ 17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000 [ 17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8 [ 17.093003] x19: ffff0005df04cb80 x18: 0000000000000014 [ 17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611 [ 17.094716] x15: 0000000000000001 x14: 0000000000000002 [ 17.095572] x13: 000000000055d966 x12: fffffe001755e400 [ 17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210 [ 17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3 [ 17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8 [ 17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3 [ 17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0 [ 17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000 [ 17.101583] Call trace: [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60 [ 17.102754] do_one_initcall+0x74/0x1cc [ 17.103381] kernel_init_freeable+0x1d0/0x238 [ 17.104089] kernel_init+0x14/0x118 [ 17.104658] ret_from_fork+0x10/0x34 [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000) [ 17.106303] ---[ end trace e63471e00f8c147f ]---
On 9/1/20 9:11 AM, Anshuman Khandual wrote: > > > On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >> This will help in adding proper locks in a later patch > > It really makes sense to classify these tests here as static and dynamic. > Static are the ones that test via page table entry values modification > without changing anything on the actual page table itself. Where as the > dynamic tests do change the page table. Static tests would probably never > require any lock synchronization but the dynamic ones will do. > >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++------------------- >> 1 file changed, 29 insertions(+), 23 deletions(-) >> >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index 0ce5c6a24c5b..78c8af3445ac 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void) >> p4dp = p4d_alloc(mm, pgdp, vaddr); >> pudp = pud_alloc(mm, p4dp, vaddr); >> pmdp = pmd_alloc(mm, pudp, vaddr); >> - ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >> + ptep = pte_alloc_map(mm, pmdp, vaddr); >> >> /* >> * Save all the page table page addresses as the page table >> @@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void) >> p4d_basic_tests(p4d_aligned, prot); >> pgd_basic_tests(pgd_aligned, prot); >> >> - pte_clear_tests(mm, ptep, vaddr); >> - pmd_clear_tests(mm, pmdp); >> - pud_clear_tests(mm, pudp); >> - p4d_clear_tests(mm, p4dp); >> - pgd_clear_tests(mm, pgdp); >> - >> - pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >> - pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep); >> - pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot); >> - hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >> - >> pmd_leaf_tests(pmd_aligned, prot); >> pud_leaf_tests(pud_aligned, prot); >> >> - pmd_huge_tests(pmdp, pmd_aligned, prot); >> - pud_huge_tests(pudp, pud_aligned, prot); >> - >> pte_savedwrite_tests(pte_aligned, protnone); >> pmd_savedwrite_tests(pmd_aligned, protnone); >> >> - pte_unmap_unlock(ptep, ptl); >> - >> - pmd_populate_tests(mm, pmdp, saved_ptep); >> - pud_populate_tests(mm, pudp, saved_pmdp); >> - p4d_populate_tests(mm, p4dp, saved_pudp); >> - pgd_populate_tests(mm, pgdp, saved_p4dp); >> - >> pte_special_tests(pte_aligned, prot); >> pte_protnone_tests(pte_aligned, protnone); >> pmd_protnone_tests(pmd_aligned, protnone); >> @@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void) >> pmd_swap_tests(pmd_aligned, prot); >> >> swap_migration_tests(); >> - hugetlb_basic_tests(pte_aligned, prot); >> >> pmd_thp_tests(pmd_aligned, prot); >> pud_thp_tests(pud_aligned, prot); >> >> + /* >> + * Page table modifying tests >> + */ > > In this comment, please do add some more details about how these tests > need proper locking mechanism unlike the previous static ones. Also add > a similar comment section for the static tests that dont really change > page table and need not require corresponding locking mechanism. Both > comment sections will make this classification clear. > >> + pte_clear_tests(mm, ptep, vaddr); >> + pmd_clear_tests(mm, pmdp); >> + pud_clear_tests(mm, pudp); >> + p4d_clear_tests(mm, p4dp); >> + pgd_clear_tests(mm, pgdp); >> + >> + ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >> + pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >> + pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep); >> + pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot); >> + hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >> + >> + >> + pmd_huge_tests(pmdp, pmd_aligned, prot); >> + pud_huge_tests(pudp, pud_aligned, prot); >> + >> + pte_unmap_unlock(ptep, ptl); >> + >> + pmd_populate_tests(mm, pmdp, saved_ptep); >> + pud_populate_tests(mm, pudp, saved_pmdp); >> + p4d_populate_tests(mm, p4dp, saved_pudp); >> + pgd_populate_tests(mm, pgdp, saved_p4dp); >> + >> + hugetlb_basic_tests(pte_aligned, prot); > > hugetlb_basic_tests() is a non page table modifying static test and > should be classified accordingly. > >> + >> p4d_free(mm, saved_p4dp); >> pud_free(mm, saved_pudp); >> pmd_free(mm, saved_pmdp); >> > > Changes till this patch fails to boot on arm64 platform. This should be > folded with the next patch. > > [ 17.080644] ------------[ cut here ]------------ > [ 17.081342] kernel BUG at mm/pgtable-generic.c:164! > [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP > [ 17.082977] Modules linked in: > [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14 > [ 17.084998] Hardware name: linux,dummy-virt (DT) > [ 17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--) > [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60 > [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0 > [ 17.088168] sp : ffff80001219bcf0 > [ 17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000 > [ 17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3 > [ 17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0 > [ 17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000 > [ 17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8 > [ 17.093003] x19: ffff0005df04cb80 x18: 0000000000000014 > [ 17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611 > [ 17.094716] x15: 0000000000000001 x14: 0000000000000002 > [ 17.095572] x13: 000000000055d966 x12: fffffe001755e400 > [ 17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210 > [ 17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3 > [ 17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8 > [ 17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3 > [ 17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0 > [ 17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000 > [ 17.101583] Call trace: > [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60 > [ 17.102754] do_one_initcall+0x74/0x1cc > [ 17.103381] kernel_init_freeable+0x1d0/0x238 > [ 17.104089] kernel_init+0x14/0x118 > [ 17.104658] ret_from_fork+0x10/0x34 > [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000) > [ 17.106303] ---[ end trace e63471e00f8c147f ]--- > IIUC, this should happen even without the series right? This is assert_spin_locked(pmd_lockptr(mm, pmdp)); -aneesh
On 09/01/2020 12:08 PM, Aneesh Kumar K.V wrote: > On 9/1/20 9:11 AM, Anshuman Khandual wrote: >> >> >> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>> This will help in adding proper locks in a later patch >> >> It really makes sense to classify these tests here as static and dynamic. >> Static are the ones that test via page table entry values modification >> without changing anything on the actual page table itself. Where as the >> dynamic tests do change the page table. Static tests would probably never >> require any lock synchronization but the dynamic ones will do. >> >>> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>> --- >>> mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++------------------- >>> 1 file changed, 29 insertions(+), 23 deletions(-) >>> >>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>> index 0ce5c6a24c5b..78c8af3445ac 100644 >>> --- a/mm/debug_vm_pgtable.c >>> +++ b/mm/debug_vm_pgtable.c >>> @@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void) >>> p4dp = p4d_alloc(mm, pgdp, vaddr); >>> pudp = pud_alloc(mm, p4dp, vaddr); >>> pmdp = pmd_alloc(mm, pudp, vaddr); >>> - ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >>> + ptep = pte_alloc_map(mm, pmdp, vaddr); >>> /* >>> * Save all the page table page addresses as the page table >>> @@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void) >>> p4d_basic_tests(p4d_aligned, prot); >>> pgd_basic_tests(pgd_aligned, prot); >>> - pte_clear_tests(mm, ptep, vaddr); >>> - pmd_clear_tests(mm, pmdp); >>> - pud_clear_tests(mm, pudp); >>> - p4d_clear_tests(mm, p4dp); >>> - pgd_clear_tests(mm, pgdp); >>> - >>> - pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >>> - pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep); >>> - pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot); >>> - hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >>> - >>> pmd_leaf_tests(pmd_aligned, prot); >>> pud_leaf_tests(pud_aligned, prot); >>> - pmd_huge_tests(pmdp, pmd_aligned, prot); >>> - pud_huge_tests(pudp, pud_aligned, prot); >>> - >>> pte_savedwrite_tests(pte_aligned, protnone); >>> pmd_savedwrite_tests(pmd_aligned, protnone); >>> - pte_unmap_unlock(ptep, ptl); >>> - >>> - pmd_populate_tests(mm, pmdp, saved_ptep); >>> - pud_populate_tests(mm, pudp, saved_pmdp); >>> - p4d_populate_tests(mm, p4dp, saved_pudp); >>> - pgd_populate_tests(mm, pgdp, saved_p4dp); >>> - >>> pte_special_tests(pte_aligned, prot); >>> pte_protnone_tests(pte_aligned, protnone); >>> pmd_protnone_tests(pmd_aligned, protnone); >>> @@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void) >>> pmd_swap_tests(pmd_aligned, prot); >>> swap_migration_tests(); >>> - hugetlb_basic_tests(pte_aligned, prot); >>> pmd_thp_tests(pmd_aligned, prot); >>> pud_thp_tests(pud_aligned, prot); >>> + /* >>> + * Page table modifying tests >>> + */ >> >> In this comment, please do add some more details about how these tests >> need proper locking mechanism unlike the previous static ones. Also add >> a similar comment section for the static tests that dont really change >> page table and need not require corresponding locking mechanism. Both >> comment sections will make this classification clear. >> >>> + pte_clear_tests(mm, ptep, vaddr); >>> + pmd_clear_tests(mm, pmdp); >>> + pud_clear_tests(mm, pudp); >>> + p4d_clear_tests(mm, p4dp); >>> + pgd_clear_tests(mm, pgdp); >>> + >>> + ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); >>> + pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >>> + pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep); >>> + pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot); >>> + hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); >>> + >>> + >>> + pmd_huge_tests(pmdp, pmd_aligned, prot); >>> + pud_huge_tests(pudp, pud_aligned, prot); >>> + >>> + pte_unmap_unlock(ptep, ptl); >>> + >>> + pmd_populate_tests(mm, pmdp, saved_ptep); >>> + pud_populate_tests(mm, pudp, saved_pmdp); >>> + p4d_populate_tests(mm, p4dp, saved_pudp); >>> + pgd_populate_tests(mm, pgdp, saved_p4dp); >>> + >>> + hugetlb_basic_tests(pte_aligned, prot); >> >> hugetlb_basic_tests() is a non page table modifying static test and >> should be classified accordingly. >> >>> + >>> p4d_free(mm, saved_p4dp); >>> pud_free(mm, saved_pudp); >>> pmd_free(mm, saved_pmdp); >>> >> >> Changes till this patch fails to boot on arm64 platform. This should be >> folded with the next patch. >> >> [ 17.080644] ------------[ cut here ]------------ >> [ 17.081342] kernel BUG at mm/pgtable-generic.c:164! >> [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP >> [ 17.082977] Modules linked in: >> [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14 >> [ 17.084998] Hardware name: linux,dummy-virt (DT) >> [ 17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--) >> [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60 >> [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0 >> [ 17.088168] sp : ffff80001219bcf0 >> [ 17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000 >> [ 17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3 >> [ 17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0 >> [ 17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000 >> [ 17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8 >> [ 17.093003] x19: ffff0005df04cb80 x18: 0000000000000014 >> [ 17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611 >> [ 17.094716] x15: 0000000000000001 x14: 0000000000000002 >> [ 17.095572] x13: 000000000055d966 x12: fffffe001755e400 >> [ 17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210 >> [ 17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3 >> [ 17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8 >> [ 17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3 >> [ 17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0 >> [ 17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000 >> [ 17.101583] Call trace: >> [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60 >> [ 17.102754] do_one_initcall+0x74/0x1cc >> [ 17.103381] kernel_init_freeable+0x1d0/0x238 >> [ 17.104089] kernel_init+0x14/0x118 >> [ 17.104658] ret_from_fork+0x10/0x34 >> [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000) >> [ 17.106303] ---[ end trace e63471e00f8c147f ]--- >> > > IIUC, this should happen even without the series right? This is > > assert_spin_locked(pmd_lockptr(mm, pmdp)); Crash does not happen without this series. A previous patch [PATCH v3 08/13] added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64 does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic pgtable_trans_huge_deposit() which the asserts the lock.
>>> >>> [ 17.080644] ------------[ cut here ]------------ >>> [ 17.081342] kernel BUG at mm/pgtable-generic.c:164! >>> [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP >>> [ 17.082977] Modules linked in: >>> [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14 >>> [ 17.084998] Hardware name: linux,dummy-virt (DT) >>> [ 17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--) >>> [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60 >>> [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0 >>> [ 17.088168] sp : ffff80001219bcf0 >>> [ 17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000 >>> [ 17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3 >>> [ 17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0 >>> [ 17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000 >>> [ 17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8 >>> [ 17.093003] x19: ffff0005df04cb80 x18: 0000000000000014 >>> [ 17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611 >>> [ 17.094716] x15: 0000000000000001 x14: 0000000000000002 >>> [ 17.095572] x13: 000000000055d966 x12: fffffe001755e400 >>> [ 17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210 >>> [ 17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3 >>> [ 17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8 >>> [ 17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3 >>> [ 17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0 >>> [ 17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000 >>> [ 17.101583] Call trace: >>> [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60 >>> [ 17.102754] do_one_initcall+0x74/0x1cc >>> [ 17.103381] kernel_init_freeable+0x1d0/0x238 >>> [ 17.104089] kernel_init+0x14/0x118 >>> [ 17.104658] ret_from_fork+0x10/0x34 >>> [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000) >>> [ 17.106303] ---[ end trace e63471e00f8c147f ]--- >>> >> >> IIUC, this should happen even without the series right? This is >> >> assert_spin_locked(pmd_lockptr(mm, pmdp)); > > Crash does not happen without this series. A previous patch [PATCH v3 08/13] > added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64 > does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic > pgtable_trans_huge_deposit() which the asserts the lock. > I fixed that by moving the pgtable deposit after adding the pmd locks correctly. mm/debug_vm_pgtable/locks: Move non page table modifying test together mm/debug_vm_pgtable/locks: Take correct page table lock mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP -aneesh
On 09/01/2020 03:06 PM, Aneesh Kumar K.V wrote: > >>>> >>>> [ 17.080644] ------------[ cut here ]------------ >>>> [ 17.081342] kernel BUG at mm/pgtable-generic.c:164! >>>> [ 17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP >>>> [ 17.082977] Modules linked in: >>>> [ 17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G W 5.9.0-rc2-00105-g740e72cd6717 #14 >>>> [ 17.084998] Hardware name: linux,dummy-virt (DT) >>>> [ 17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--) >>>> [ 17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60 >>>> [ 17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0 >>>> [ 17.088168] sp : ffff80001219bcf0 >>>> [ 17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000 >>>> [ 17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3 >>>> [ 17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0 >>>> [ 17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000 >>>> [ 17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8 >>>> [ 17.093003] x19: ffff0005df04cb80 x18: 0000000000000014 >>>> [ 17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611 >>>> [ 17.094716] x15: 0000000000000001 x14: 0000000000000002 >>>> [ 17.095572] x13: 000000000055d966 x12: fffffe001755e400 >>>> [ 17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210 >>>> [ 17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3 >>>> [ 17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8 >>>> [ 17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3 >>>> [ 17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0 >>>> [ 17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000 >>>> [ 17.101583] Call trace: >>>> [ 17.101993] pgtable_trans_huge_deposit+0x58/0x60 >>>> [ 17.102754] do_one_initcall+0x74/0x1cc >>>> [ 17.103381] kernel_init_freeable+0x1d0/0x238 >>>> [ 17.104089] kernel_init+0x14/0x118 >>>> [ 17.104658] ret_from_fork+0x10/0x34 >>>> [ 17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000) >>>> [ 17.106303] ---[ end trace e63471e00f8c147f ]--- >>>> >>> >>> IIUC, this should happen even without the series right? This is >>> >>> assert_spin_locked(pmd_lockptr(mm, pmdp)); >> >> Crash does not happen without this series. A previous patch [PATCH v3 08/13] >> added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64 >> does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic >> pgtable_trans_huge_deposit() which the asserts the lock. >> > > > I fixed that by moving the pgtable deposit after adding the pmd locks correctly. > > mm/debug_vm_pgtable/locks: Move non page table modifying test together > mm/debug_vm_pgtable/locks: Take correct page table lock > mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP Right, it does fix. But then both those patches should be folded/merged in order to preserve git bisect ability, besides test classification reasons as mentioned in a previous response and a possible redundant movement of hugetlb_basic_tests().
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 0ce5c6a24c5b..78c8af3445ac 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void) p4dp = p4d_alloc(mm, pgdp, vaddr); pudp = pud_alloc(mm, p4dp, vaddr); pmdp = pmd_alloc(mm, pudp, vaddr); - ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); + ptep = pte_alloc_map(mm, pmdp, vaddr); /* * Save all the page table page addresses as the page table @@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void) p4d_basic_tests(p4d_aligned, prot); pgd_basic_tests(pgd_aligned, prot); - pte_clear_tests(mm, ptep, vaddr); - pmd_clear_tests(mm, pmdp); - pud_clear_tests(mm, pudp); - p4d_clear_tests(mm, p4dp); - pgd_clear_tests(mm, pgdp); - - pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); - pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep); - pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot); - hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); - pmd_leaf_tests(pmd_aligned, prot); pud_leaf_tests(pud_aligned, prot); - pmd_huge_tests(pmdp, pmd_aligned, prot); - pud_huge_tests(pudp, pud_aligned, prot); - pte_savedwrite_tests(pte_aligned, protnone); pmd_savedwrite_tests(pmd_aligned, protnone); - pte_unmap_unlock(ptep, ptl); - - pmd_populate_tests(mm, pmdp, saved_ptep); - pud_populate_tests(mm, pudp, saved_pmdp); - p4d_populate_tests(mm, p4dp, saved_pudp); - pgd_populate_tests(mm, pgdp, saved_p4dp); - pte_special_tests(pte_aligned, prot); pte_protnone_tests(pte_aligned, protnone); pmd_protnone_tests(pmd_aligned, protnone); @@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void) pmd_swap_tests(pmd_aligned, prot); swap_migration_tests(); - hugetlb_basic_tests(pte_aligned, prot); pmd_thp_tests(pmd_aligned, prot); pud_thp_tests(pud_aligned, prot); + /* + * Page table modifying tests + */ + pte_clear_tests(mm, ptep, vaddr); + pmd_clear_tests(mm, pmdp); + pud_clear_tests(mm, pudp); + p4d_clear_tests(mm, p4dp); + pgd_clear_tests(mm, pgdp); + + ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl); + pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); + pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep); + pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot); + hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot); + + + pmd_huge_tests(pmdp, pmd_aligned, prot); + pud_huge_tests(pudp, pud_aligned, prot); + + pte_unmap_unlock(ptep, ptl); + + pmd_populate_tests(mm, pmdp, saved_ptep); + pud_populate_tests(mm, pudp, saved_pmdp); + p4d_populate_tests(mm, p4dp, saved_pudp); + pgd_populate_tests(mm, pgdp, saved_p4dp); + + hugetlb_basic_tests(pte_aligned, prot); + p4d_free(mm, saved_p4dp); pud_free(mm, saved_pudp); pmd_free(mm, saved_pmdp);
This will help in adding proper locks in a later patch Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 23 deletions(-)