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

Commit Message

Anshuman Khandual May 10, 2021, 11:07 a.m. UTC

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 <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Mark Rutland <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Anshuman Khandual <[e-mail]>
---
This applies on v5.13-rc1.

 arch/arm64/include/asm/pgtable.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Mark Rutland May 10, 2021, 2:43 p.m. UTC | #1

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 <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Anshuman Khandual <[e-mail]>

I have no strong feelings either way, so: 

Acked-by: Mark Rutland <[e-mail]>

... 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".

It does seem that would roughly fit p??_leaf() if we had
p??_clear_leaf() and p??_none_or_clear_leaf() helpers.

Thanks,
Mark.

> ---
> This applies on v5.13-rc1.
> 
>  arch/arm64/include/asm/pgtable.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 25f5c04b43ce..69f8183bef29 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -509,13 +509,12 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  
>  #define pmd_none(pmd)		(!pmd_val(pmd))
>  
> -#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
> -
>  #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>  				 PMD_TYPE_TABLE)
>  #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>  				 PMD_TYPE_SECT)
>  #define pmd_leaf(pmd)		pmd_sect(pmd)
> +#define pmd_bad(pmd)		(!pmd_table(pmd))
>  
>  #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
>  #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
> @@ -602,7 +601,7 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>  	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
>  
>  #define pud_none(pud)		(!pud_val(pud))
> -#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
> +#define pud_bad(pud)		(!pud_table(pud))
>  #define pud_present(pud)	pte_present(pud_pte(pud))
>  #define pud_leaf(pud)		pud_sect(pud)
>  #define pud_valid(pud)		pte_valid(pud_pte(pud))
> -- 
> 2.20.1
>

Anshuman Khandual May 11, 2021, 3:51 a.m. UTC | #2

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 <[e-mail]>
>> Cc: Will Deacon <[e-mail]>
>> Cc: Mark Rutland <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Signed-off-by: Anshuman Khandual <[e-mail]>
> 
> I have no strong feelings either way, so: 
> 
> Acked-by: Mark Rutland <[e-mail]>
> 
> ... 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.

> p??_clear_leaf() and p??_none_or_clear_leaf() helpers.

Could you please elaborate how these new helpers might help define
pxx_bad() ?

> 
> Thanks,
> Mark.
> 
>> ---
>> This applies on v5.13-rc1.
>>
>>  arch/arm64/include/asm/pgtable.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 25f5c04b43ce..69f8183bef29 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -509,13 +509,12 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>>  
>>  #define pmd_none(pmd)		(!pmd_val(pmd))
>>  
>> -#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
>> -
>>  #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>>  				 PMD_TYPE_TABLE)
>>  #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>>  				 PMD_TYPE_SECT)
>>  #define pmd_leaf(pmd)		pmd_sect(pmd)
>> +#define pmd_bad(pmd)		(!pmd_table(pmd))
>>  
>>  #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
>>  #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
>> @@ -602,7 +601,7 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>>  	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
>>  
>>  #define pud_none(pud)		(!pud_val(pud))
>> -#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
>> +#define pud_bad(pud)		(!pud_table(pud))
>>  #define pud_present(pud)	pte_present(pud_pte(pud))
>>  #define pud_leaf(pud)		pud_sect(pud)
>>  #define pud_valid(pud)		pte_valid(pud_pte(pud))
>> -- 
>> 2.20.1
>>

Mark Rutland May 11, 2021, 2:07 p.m. UTC | #3

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 <[e-mail]>
> >> Cc: Will Deacon <[e-mail]>
> >> Cc: Mark Rutland <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: [e-mail]
> >> Signed-off-by: Anshuman Khandual <[e-mail]>
> > 
> > I have no strong feelings either way, so: 
> > 
> > Acked-by: Mark Rutland <[e-mail]>
> > 
> > ... 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.

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.

Anshuman Khandual May 13, 2021, 5:14 a.m. UTC | #4

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 <[e-mail]>
>>>> Cc: Will Deacon <[e-mail]>
>>>> Cc: Mark Rutland <[e-mail]>
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Signed-off-by: Anshuman Khandual <[e-mail]>
>>>
>>> I have no strong feelings either way, so: 
>>>
>>> Acked-by: Mark Rutland <[e-mail]>
>>>
>>> ... 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.
>

Catalin Marinas May 14, 2021, 10:59 a.m. UTC | #5

On Thu, May 13, 2021 at 10:44:04AM +0530, Anshuman Khandual wrote:
> 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 <[e-mail]>
> >>>> Cc: Will Deacon <[e-mail]>
> >>>> Cc: Mark Rutland <[e-mail]>
> >>>> Cc: [e-mail]
> >>>> Cc: [e-mail]
> >>>> Signed-off-by: Anshuman Khandual <[e-mail]>
> >>>
> >>> I have no strong feelings either way, so: 
> >>>
> >>> Acked-by: Mark Rutland <[e-mail]>
> >>>
> >>> ... 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 a quick look through some of the uses and it seems the expectation
is that after a !pmd_bad(), the pmd is a table. The checks for
migration, huge page etc. are prior to the pmd_bad() check.

For this patch:

Acked-by: Catalin Marinas <[e-mail]>

Will Deacon May 25, 2021, 6:58 p.m. UTC | #6

On Mon, 10 May 2021 16:37:51 +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.

Applied to arm64 (for-next/mm), thanks!

[1/1] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()
      https://git.kernel.org/arm64/c/e377ab82311a

Cheers,

Patch

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 25f5c04b43ce..69f8183bef29 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -509,13 +509,12 @@  extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
 
 #define pmd_none(pmd)		(!pmd_val(pmd))
 
-#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
-
 #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
 				 PMD_TYPE_TABLE)
 #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
 				 PMD_TYPE_SECT)
 #define pmd_leaf(pmd)		pmd_sect(pmd)
+#define pmd_bad(pmd)		(!pmd_table(pmd))
 
 #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
 #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
@@ -602,7 +601,7 @@  static inline unsigned long pmd_page_vaddr(pmd_t pmd)
 	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
 
 #define pud_none(pud)		(!pud_val(pud))
-#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
+#define pud_bad(pud)		(!pud_table(pud))
 #define pud_present(pud)	pte_present(pud_pte(pud))
 #define pud_leaf(pud)		pud_sect(pud)
 #define pud_valid(pud)		pte_valid(pud_pte(pud))