Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Anshuman Khandual]

On 5/11/21 7:37 PM, Mark Rutland wrote:
> On Tue, May 11, 2021 at 09:21:46AM +0530, Anshuman Khandual wrote:
>>
>> On 5/10/21 8:13 PM, Mark Rutland wrote:
>>> On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
>>>> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
>>>> entry does not have a pointer to the next level page table. This had been
>>>> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
>>>> section maps"). Hence explicitly check for a table entry rather than just
>>>> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
>>>> [pud|pmd]_table() making the semantics clear.
>>>>
>>>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>>>> Cc: Will Deacon <will@xxxxxxxxxx>
>>>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>>
>>> I have no strong feelings either way, so: 
>>>
>>> Acked-by: Mark Rutland <mark.rutland@xxxxxxx>
>>>
>>> ... that said, I think that the "bad" naming is unclear and misleading,
>>> and it'd be really nice if we could clean that up treewide with
>>> something clearer than "bad".
>>
>> Agreed, the name is misleading.
>>
>>> It does seem that would roughly fit p??_leaf() if we had
>>
>> But what if the platform does not support huge page aka leaf mapping
>> at the given level ? Also a non table i.e bad entry might not always
>> mean a leaf/section/huge page mapping, it could simply imply that the
>> entry is not just pointing to next level and might be just in an bad
>> intermediate or invalid state.
> 
> Ah, so that's also covering swap entries, too? It's not entirely clear
> to me what "bad intermediate or invalid state" means, because I assume
> it's not arbitrary junk or this wouldn't be sound genrally.

Intermediate states like swap, migration or probably even splitting THP.
Though I am not really sure whether pxx_bad() only gets used for valid
table or leaf entries i.e things which are mapped. Hence checking just
for non table entry is better and even safer, than looking out for what
other states the entry could be in.

> 
> I had assumed it was only covering *valid* non-table entries.
> 
>>> p??_clear_leaf() and p??_none_or_clear_leaf() helpers.
>>
>> Could you please elaborate how these new helpers might help define
>> pxx_bad() ?
> 
> This was based on my (evidently wrong) prior understanding above.
> 
> Thanks,
> Mark.
>