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

[Ryan Roberts <ryan.roberts@arm.com>]

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!