[PATCH v1] mm/debug_vm_pgtable: Test pmd_leaf() behavior with pmd_mkinvalid()

[PATCH v1] mm/debug_vm_pgtable: Test pmd_leaf() behavior with pmd_mkinvalid()

[Ryan Roberts]

An invalidated pmd should still cause pmd_leaf() to return true. Let's
test for that to ensure all arches remain consistent.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---

Hi Andrew,

This applies on top of v6.9-rc5. It came out of a discussion with Catalin around
the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test
on both arm64 and x86_64.

Thanks,
Ryan

 mm/debug_vm_pgtable.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 65c19025da3d..57e9cb0820ab 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args)
 #ifndef __HAVE_ARCH_PMDP_INVALIDATE
 	WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd))));
 	WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd))));
+	WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd))));
 #endif /* __HAVE_ARCH_PMDP_INVALIDATE */
 }

--
2.25.1

Re: [PATCH v1] mm/debug_vm_pgtable: Test pmd_leaf() behavior with pmd_mkinvalid()

[Anshuman Khandual]

Hello Ryan,

On 5/1/24 20:14, Ryan Roberts wrote:
> An invalidated pmd should still cause pmd_leaf() to return true. Let's
> test for that to ensure all arches remain consistent.

This test definitely makes sense.

> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
> 
> Hi Andrew,
> 
> This applies on top of v6.9-rc5. It came out of a discussion with Catalin around
> the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test
> on both arm64 and x86_64.

Right, works on arm64.

> 
> Thanks,
> Ryan
> 
>  mm/debug_vm_pgtable.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 65c19025da3d..57e9cb0820ab 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args)
>  #ifndef __HAVE_ARCH_PMDP_INVALIDATE
>  	WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd))));
>  	WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd))));
> +	WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd))));
>  #endif /* __HAVE_ARCH_PMDP_INVALIDATE */
>  }

Should not we update descriptions in Documentation/mm/arch_pgtable_helpers.rst
asserting that pmd_mkinvalid() also preserves pmd_leaf() ?

Re: [PATCH v1] mm/debug_vm_pgtable: Test pmd_leaf() behavior with pmd_mkinvalid()

[Ryan Roberts]

On 02/05/2024 03:43, Anshuman Khandual wrote:
> Hello Ryan,
> 
> On 5/1/24 20:14, Ryan Roberts wrote:
>> An invalidated pmd should still cause pmd_leaf() to return true. Let's
>> test for that to ensure all arches remain consistent.
> 
> This test definitely makes sense.
> 
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
>>
>> Hi Andrew,
>>
>> This applies on top of v6.9-rc5. It came out of a discussion with Catalin around
>> the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test
>> on both arm64 and x86_64.
> 
> Right, works on arm64.
> 
>>
>> Thanks,
>> Ryan
>>
>>  mm/debug_vm_pgtable.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index 65c19025da3d..57e9cb0820ab 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args)
>>  #ifndef __HAVE_ARCH_PMDP_INVALIDATE
>>  	WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd))));
>>  	WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd))));
>> +	WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd))));
>>  #endif /* __HAVE_ARCH_PMDP_INVALIDATE */
>>  }
> 
> Should not we update descriptions in Documentation/mm/arch_pgtable_helpers.rst
> asserting that pmd_mkinvalid() also preserves pmd_leaf() ?

Thanks for the review!

We don't document that pmd_mkinvalid() preserves pmd_present() and
pmd_trans_huge() so I wasn't sure how much detail was appropriate in that
document - its pretty light at the moment.

If you think this is valuable (and isn't clear enough from the test) then I can
add something. But as you say in the other patch, it would then start
conflicting with that. I'd prefer to just put this in as-is to avoid the mess.

Re: [PATCH v1] mm/debug_vm_pgtable: Test pmd_leaf() behavior with pmd_mkinvalid()

[Anshuman Khandual]

On 5/2/24 13:00, Ryan Roberts wrote:
> On 02/05/2024 03:43, Anshuman Khandual wrote:
>> Hello Ryan,
>>
>> On 5/1/24 20:14, Ryan Roberts wrote:
>>> An invalidated pmd should still cause pmd_leaf() to return true. Let's
>>> test for that to ensure all arches remain consistent.
>>
>> This test definitely makes sense.
>>
>>>
>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>>
>>> Hi Andrew,
>>>
>>> This applies on top of v6.9-rc5. It came out of a discussion with Catalin around
>>> the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test
>>> on both arm64 and x86_64.
>>
>> Right, works on arm64.
>>
>>>
>>> Thanks,
>>> Ryan
>>>
>>>  mm/debug_vm_pgtable.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index 65c19025da3d..57e9cb0820ab 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args)
>>>  #ifndef __HAVE_ARCH_PMDP_INVALIDATE
>>>  	WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd))));
>>>  	WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd))));
>>> +	WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd))));
>>>  #endif /* __HAVE_ARCH_PMDP_INVALIDATE */
>>>  }
>>
>> Should not we update descriptions in Documentation/mm/arch_pgtable_helpers.rst
>> asserting that pmd_mkinvalid() also preserves pmd_leaf() ?
> 
> Thanks for the review!
> 
> We don't document that pmd_mkinvalid() preserves pmd_present() and
> pmd_trans_huge() so I wasn't sure how much detail was appropriate in that
> document - its pretty light at the moment.

For all other helpers documentation has been light but pxd_mkinvalid() is turning
out to be a special case though.

> 
> If you think this is valuable (and isn't clear enough from the test) then I can
> add something. But as you say in the other patch, it would then start
> conflicting with that. I'd prefer to just put this in as-is to avoid the mess.

Sure, fair enough. I will try and update how pmd_mkinvalid() preserves pmd_leaf(),
pmd_present(), and pmd_trans_huge() at a later point. Otherwise this patch itself
LGTM and runs fine on arm64.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH v1] mm/debug_vm_pgtable: Test pmd_leaf() behavior with pmd_mkinvalid()

[Ryan Roberts]

On 02/05/2024 09:03, Anshuman Khandual wrote:
> 
> 
> On 5/2/24 13:00, Ryan Roberts wrote:
>> On 02/05/2024 03:43, Anshuman Khandual wrote:
>>> Hello Ryan,
>>>
>>> On 5/1/24 20:14, Ryan Roberts wrote:
>>>> An invalidated pmd should still cause pmd_leaf() to return true. Let's
>>>> test for that to ensure all arches remain consistent.
>>>
>>> This test definitely makes sense.
>>>
>>>>
>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>> ---
>>>>
>>>> Hi Andrew,
>>>>
>>>> This applies on top of v6.9-rc5. It came out of a discussion with Catalin around
>>>> the pmd_mkinvalid() bug (the fix for which I just posted). I've run the new test
>>>> on both arm64 and x86_64.
>>>
>>> Right, works on arm64.
>>>
>>>>
>>>> Thanks,
>>>> Ryan
>>>>
>>>>  mm/debug_vm_pgtable.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index 65c19025da3d..57e9cb0820ab 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -981,6 +981,7 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args)
>>>>  #ifndef __HAVE_ARCH_PMDP_INVALIDATE
>>>>  	WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd))));
>>>>  	WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd))));
>>>> +	WARN_ON(!pmd_leaf(pmd_mkinvalid(pmd_mkhuge(pmd))));
>>>>  #endif /* __HAVE_ARCH_PMDP_INVALIDATE */
>>>>  }
>>>
>>> Should not we update descriptions in Documentation/mm/arch_pgtable_helpers.rst
>>> asserting that pmd_mkinvalid() also preserves pmd_leaf() ?
>>
>> Thanks for the review!
>>
>> We don't document that pmd_mkinvalid() preserves pmd_present() and
>> pmd_trans_huge() so I wasn't sure how much detail was appropriate in that
>> document - its pretty light at the moment.
> 
> For all other helpers documentation has been light but pxd_mkinvalid() is turning
> out to be a special case though.
> 
>>
>> If you think this is valuable (and isn't clear enough from the test) then I can
>> add something. But as you say in the other patch, it would then start
>> conflicting with that. I'd prefer to just put this in as-is to avoid the mess.
> 
> Sure, fair enough. I will try and update how pmd_mkinvalid() preserves pmd_leaf(),
> pmd_present(), and pmd_trans_huge() at a later point. Otherwise this patch itself
> LGTM and runs fine on arm64.
> 
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Thanks!

Re: [PATCH v1] mm/debug_vm_pgtable: Test pmd_leaf() behavior with pmd_mkinvalid()

[David Hildenbrand]

On 01.05.24 16:44, Ryan Roberts wrote:
> An invalidated pmd should still cause pmd_leaf() to return true. Let's
> test for that to ensure all arches remain consistent.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb