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

[Anshuman Khandual <anshuman.khandual@arm.com>]

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>