[V2,2/6] arm64/mm: Make pgd_pgtable_alloc() call pte_alloc_one() always
diff mbox series

Message ID 1551071039-20192-3-git-send-email-anshuman.khandual@arm.com
State New
Headers show
Series
  • arm64/mm: Enable accounting for page table pages
Related show

Commit Message

Anshuman Khandual Feb. 25, 2019, 5:03 a.m. UTC
pgd_pgtable_alloc() provides page allocation function while creating all
levels of page table (PGD, PUD, CONT PMD etc) for various kernel mappings.
It calls __get_free_page() and initializes page with pagetable_page_ctor().
pte_alloc_one() already provides a standard interface for allocating a page
table page and initializes it with pagetable_page_ctor(). This removes the
redundancy and instead make it call pte_alloc_one() directly.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/mm/mmu.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Mark Rutland Feb. 25, 2019, 11:08 a.m. UTC | #1
On Mon, Feb 25, 2019 at 10:33:55AM +0530, Anshuman Khandual wrote:
> pgd_pgtable_alloc() provides page allocation function while creating all
> levels of page table (PGD, PUD, CONT PMD etc) for various kernel mappings.
> It calls __get_free_page() and initializes page with pagetable_page_ctor().
> pte_alloc_one() already provides a standard interface for allocating a page
> table page and initializes it with pagetable_page_ctor(). This removes the
> redundancy and instead make it call pte_alloc_one() directly.

I see we also have pte_alloc_one_kernel(), which does not call
pgtable_page_ctor().

Was it deliberate that we don't call the ctor on kernel mappings?

> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/mm/mmu.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index b6f5aa52ac67..2dbd72319152 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -372,13 +372,22 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>  
>  static phys_addr_t pgd_pgtable_alloc(void)
>  {
> -	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
> -	if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
> -		BUG();
> +
> +	pgtable_t ptr;
> +
> +	/*
> +	 * pgd_pgtable_alloc() is called while creating kernel mappings
> +	 * through __create_pgd_mapping() might not install it through
> +	 * swapper_pg_dir (&init_mm). Even then init_mm is passed here
> +	 * just to indicate that the allocation is kernel specific not
> +	 * for the user space page tables.
> +	 */

I'm not sure what this is trying to convey. I guess we also use this for
creating the EFI mappings?

No other architecture seems to use pte_alloc_one() for kernel tables,
and it's not clear to me how the mm argument is intended to be used by
the implementation.

Thanks,
Mark.

> +	ptr = pte_alloc_one(&init_mm);
> +	BUG_ON(!ptr);
>  
>  	/* Ensure the zeroed page is visible to the page table walker */
>  	dsb(ishst);
> -	return __pa(ptr);
> +	return page_to_phys(ptr);
>  }
>  
>  /*
> -- 
> 2.20.1
>
Anshuman Khandual Feb. 25, 2019, 2:41 p.m. UTC | #2
pgtable_page_ctor/pgtable

On 02/25/2019 04:38 PM, Mark Rutland wrote:
> On Mon, Feb 25, 2019 at 10:33:55AM +0530, Anshuman Khandual wrote:
>> pgd_pgtable_alloc() provides page allocation function while creating all
>> levels of page table (PGD, PUD, CONT PMD etc) for various kernel mappings.
>> It calls __get_free_page() and initializes page with pagetable_page_ctor().
>> pte_alloc_one() already provides a standard interface for allocating a page
>> table page and initializes it with pagetable_page_ctor(). This removes the
>> redundancy and instead make it call pte_alloc_one() directly.
> 
> I see we also have pte_alloc_one_kernel(), which does not call
> pgtable_page_ctor().

We are changing it.

> 
> Was it deliberate that we don't call the ctor on kernel mappings?

Right now parts of the kernel mapping call pgtable_page_ctor() like everything
with __create_pgd_mapping(pgd_pgtabble_alloc,....) but pgtable_page_ctor()
never gets called with pte_alloc_one_kernel() based allocations. This series
is trying to move all possible kernel mappings to use pgtable_page_ctor/pgtable
_page_dtor() constructs.

It makes sense to call pgtable_page_ctor/pgtable_page_dtor for kernel mappings
as it helps put the page in right state PageTable and also account towards zone
statistics (NR_PAGETABLE).

>
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/mm/mmu.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index b6f5aa52ac67..2dbd72319152 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -372,13 +372,22 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>>  
>>  static phys_addr_t pgd_pgtable_alloc(void)
>>  {
>> -	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
>> -	if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
>> -		BUG();
>> +
>> +	pgtable_t ptr;
>> +
>> +	/*
>> +	 * pgd_pgtable_alloc() is called while creating kernel mappings
>> +	 * through __create_pgd_mapping() might not install it through
>> +	 * swapper_pg_dir (&init_mm). Even then init_mm is passed here
>> +	 * just to indicate that the allocation is kernel specific not
>> +	 * for the user space page tables.
>> +	 */
> 
> I'm not sure what this is trying to convey. I guess we also use this for
> creating the EFI mappings?

The argument here will be used to select right GFP flags. With this init_mm
would pick the right one whether its used for init_mm or efi_mm mappings.
Though we still check for both the options while picking up the GFP flags
in one of the later patches.

> 
> No other architecture seems to use pte_alloc_one() for kernel tables,
> and it's not clear to me how the mm argument is intended to be used by
> the implementation.

It selects appropriate GFP flags (with or without __GFP_ACCOUNT). To make
things better will probably pass mm_struct in pgtable_alloc() which is used
with __create_pgd_mappings. Parts of arm64 kernel mappings already use it.
This is just making it uniform across the kernel.

Patch
diff mbox series

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index b6f5aa52ac67..2dbd72319152 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -372,13 +372,22 @@  static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
 
 static phys_addr_t pgd_pgtable_alloc(void)
 {
-	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
-	if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
-		BUG();
+
+	pgtable_t ptr;
+
+	/*
+	 * pgd_pgtable_alloc() is called while creating kernel mappings
+	 * through __create_pgd_mapping() might not install it through
+	 * swapper_pg_dir (&init_mm). Even then init_mm is passed here
+	 * just to indicate that the allocation is kernel specific not
+	 * for the user space page tables.
+	 */
+	ptr = pte_alloc_one(&init_mm);
+	BUG_ON(!ptr);
 
 	/* Ensure the zeroed page is visible to the page table walker */
 	dsb(ishst);
-	return __pa(ptr);
+	return page_to_phys(ptr);
 }
 
 /*