diff mbox series

[v3,09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together

Message ID 20200827080438.315345-10-aneesh.kumar@linux.ibm.com
State New
Headers show
Series mm/debug_vm_pgtable fixes | expand

Commit Message

Aneesh Kumar K.V Aug. 27, 2020, 8:04 a.m. UTC
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(-)

Comments

Anshuman Khandual Sept. 1, 2020, 3:41 a.m. UTC | #1
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 ]---
Aneesh Kumar K.V Sept. 1, 2020, 6:38 a.m. UTC | #2
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
Anshuman Khandual Sept. 1, 2020, 9:33 a.m. UTC | #3
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.
Aneesh Kumar K.V Sept. 1, 2020, 9:36 a.m. UTC | #4
>>>
>>> [   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
Anshuman Khandual Sept. 1, 2020, 9:58 a.m. UTC | #5
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 mbox series

Patch

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);