[V2,5/6] arm64/mm: Enable page table page accounting for user space
diff mbox series

Message ID 1551071039-20192-6-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
Page table pages created for user space processes must be accounted against
their memory control groups if initialized. This just introduces a helper
function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
table page allocations but adds __GFP_ACCOUNT for user page table pages.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/pgalloc.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Mark Rutland Feb. 25, 2019, 11:11 a.m. UTC | #1
On Mon, Feb 25, 2019 at 10:33:58AM +0530, Anshuman Khandual wrote:
> Page table pages created for user space processes must be accounted against
> their memory control groups if initialized. This just introduces a helper
> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
> table page allocations but adds __GFP_ACCOUNT for user page table pages.

Can't we have pte_alloc_one_kernel() and pte_alloc_one() explicitly pass
the GFP flags down to a __pte_alloc_one() helper, and consistently use
pte_alloc_one_kernel() for kernel mappings?

That would seem less surprising than hiding that detail in another
function.

Thanks,
Mark.

> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/include/asm/pgalloc.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
> index 076764f66fb0..a02a4d1d967d 100644
> --- a/arch/arm64/include/asm/pgalloc.h
> +++ b/arch/arm64/include/asm/pgalloc.h
> @@ -100,12 +100,24 @@ pte_alloc_one_kernel(struct mm_struct *mm)
>  	return (pte_t *)pte_alloc_one_virt(mm);
>  }
>  
> +extern struct mm_struct efi_mm;
> +
> +static inline gfp_t pgtable_gfp_flags(struct mm_struct *mm)
> +{
> +	if (unlikely((mm == &init_mm) || (mm == &efi_mm)))
> +		return PGALLOC_GFP;
> +	else
> +		return PGALLOC_GFP | __GFP_ACCOUNT;
> +}
> +
>  static inline pgtable_t
>  pte_alloc_one(struct mm_struct *mm)
>  {
>  	struct page *pte;
> +	gfp_t gfp;
>  
> -	pte = alloc_pages(PGALLOC_GFP, 0);
> +	gfp = pgtable_gfp_flags(mm);
> +	pte = alloc_pages(gfp, 0);
>  	if (!pte)
>  		return NULL;
>  	if (!pgtable_page_ctor(pte)) {
> -- 
> 2.20.1
>
Anshuman Khandual Feb. 25, 2019, 2:49 p.m. UTC | #2
On 02/25/2019 04:41 PM, Mark Rutland wrote:
> On Mon, Feb 25, 2019 at 10:33:58AM +0530, Anshuman Khandual wrote:
>> Page table pages created for user space processes must be accounted against
>> their memory control groups if initialized. This just introduces a helper
>> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
>> table page allocations but adds __GFP_ACCOUNT for user page table pages.
> 
> Can't we have pte_alloc_one_kernel() and pte_alloc_one() explicitly pass
> the GFP flags down to a __pte_alloc_one() helper, and consistently use
> pte_alloc_one_kernel() for kernel mappings?
> 
> That would seem less surprising than hiding that detail in another
> function.

Yes that will work as well. But its better to have this mm_struct based GFP switch
which is used in other archs as well to create generic MM helper functions going
forward.
Mark Rutland Feb. 25, 2019, 3:35 p.m. UTC | #3
On Mon, Feb 25, 2019 at 08:19:53PM +0530, Anshuman Khandual wrote:
> 
> 
> On 02/25/2019 04:41 PM, Mark Rutland wrote:
> > On Mon, Feb 25, 2019 at 10:33:58AM +0530, Anshuman Khandual wrote:
> >> Page table pages created for user space processes must be accounted against
> >> their memory control groups if initialized. This just introduces a helper
> >> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
> >> table page allocations but adds __GFP_ACCOUNT for user page table pages.
> > 
> > Can't we have pte_alloc_one_kernel() and pte_alloc_one() explicitly pass
> > the GFP flags down to a __pte_alloc_one() helper, and consistently use
> > pte_alloc_one_kernel() for kernel mappings?
> > 
> > That would seem less surprising than hiding that detail in another
> > function.
> 
> Yes that will work as well. But its better to have this mm_struct based GFP switch
> which is used in other archs as well to create generic MM helper functions going
> forward.

I don't follow -- if we can make pte_alloc_one() generic, then we can
*also* make pte_alloc_one_kernel() generic.

Looking at x86, I see:

pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
{
        return (pte_t *)__get_free_page(PGALLOC_GFP & ~__GFP_ACCOUNT);
}

... which arhitectures do you see which look at the mm struct to decide
the flags?

Thanks,
Mark.
Anshuman Khandual Feb. 26, 2019, 5:06 a.m. UTC | #4
On 02/25/2019 09:05 PM, Mark Rutland wrote:
> On Mon, Feb 25, 2019 at 08:19:53PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 02/25/2019 04:41 PM, Mark Rutland wrote:
>>> On Mon, Feb 25, 2019 at 10:33:58AM +0530, Anshuman Khandual wrote:
>>>> Page table pages created for user space processes must be accounted against
>>>> their memory control groups if initialized. This just introduces a helper
>>>> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
>>>> table page allocations but adds __GFP_ACCOUNT for user page table pages.
>>>
>>> Can't we have pte_alloc_one_kernel() and pte_alloc_one() explicitly pass
>>> the GFP flags down to a __pte_alloc_one() helper, and consistently use
>>> pte_alloc_one_kernel() for kernel mappings?
>>>
>>> That would seem less surprising than hiding that detail in another
>>> function.
>>
>> Yes that will work as well. But its better to have this mm_struct based GFP switch
>> which is used in other archs as well to create generic MM helper functions going
>> forward.
> 
> I don't follow -- if we can make pte_alloc_one() generic, then we can
> *also* make pte_alloc_one_kernel() generic.

Right but when we do mm_struct can be used to decide applicable GFP for
the allocation. The way your proposal defers is that it wants to have
the GFP flag unique for both pte_alloc_one and pte_alloc_one_kernel
without looking into mm_struct which will work as well.

> 
> Looking at x86, I see:
> 
> pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
> {
>         return (pte_t *)__get_free_page(PGALLOC_GFP & ~__GFP_ACCOUNT);
> }
> 
> ... which arhitectures do you see which look at the mm struct to decide
> the flags?
> 

On powerpc its an explicit helper function

arch/powerpc/include/asm/pgalloc.h (pgtable_gfp_flags)

On X86 its not an explicit helper function but we can see

arch/x86/include/asm/pgalloc.h

static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long addr)
{
        struct page *page;
        gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO;

        if (mm == &init_mm)
                gfp &= ~__GFP_ACCOUNT;
        page = alloc_pages(gfp, 0);
        if (!page)
                return NULL;
        if (!pgtable_pmd_page_ctor(page)) {
                __free_pages(page, 0);
                return NULL;
        }
        return (pmd_t *)page_address(page);
}

static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
{
        gfp_t gfp = GFP_KERNEL_ACCOUNT;

        if (mm == &init_mm)
                gfp &= ~__GFP_ACCOUNT;
        return (pud_t *)get_zeroed_page(gfp);
}

static inline p4d_t *p4d_alloc_one(struct mm_struct *mm, unsigned long addr)
{
        gfp_t gfp = GFP_KERNEL_ACCOUNT;

        if (mm == &init_mm)
                gfp &= ~__GFP_ACCOUNT;
        return (p4d_t *)get_zeroed_page(gfp);
}

There are some more places on powerpc and x86 where it does the switch. But you
are right, pte_alloc_one_kernel() does not go through ctor functions on other
archs. But it should and this series changes it. Do you see any concerns in
doing so ?
Anshuman Khandual Feb. 26, 2019, 6:37 a.m. UTC | #5
On 02/25/2019 10:33 AM, Anshuman Khandual wrote:
> Page table pages created for user space processes must be accounted against
> their memory control groups if initialized. This just introduces a helper
> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
> table page allocations but adds __GFP_ACCOUNT for user page table pages.
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/include/asm/pgalloc.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
> index 076764f66fb0..a02a4d1d967d 100644
> --- a/arch/arm64/include/asm/pgalloc.h
> +++ b/arch/arm64/include/asm/pgalloc.h
> @@ -100,12 +100,24 @@ pte_alloc_one_kernel(struct mm_struct *mm)
>  	return (pte_t *)pte_alloc_one_virt(mm);
>  }
>  
> +extern struct mm_struct efi_mm;
> +
> +static inline gfp_t pgtable_gfp_flags(struct mm_struct *mm)
> +{
> +	if (unlikely((mm == &init_mm) || (mm == &efi_mm)))
> +		return PGALLOC_GFP;

Hello Will,

Checking for efi_mm is not necessary here as we just pass init_mm into
pte_alloc_one() from pgd_pgtable_alloc() which is used for all runtime
allocations for kernel mappings including init_mm and efi_mm. If we
really want to differentiate them, the helper pgtable_alloc() has to be
changed to include mm_struct. That might be some amount of code churn.
Wondering what will be preferred here.

- Anshuman

Patch
diff mbox series

diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
index 076764f66fb0..a02a4d1d967d 100644
--- a/arch/arm64/include/asm/pgalloc.h
+++ b/arch/arm64/include/asm/pgalloc.h
@@ -100,12 +100,24 @@  pte_alloc_one_kernel(struct mm_struct *mm)
 	return (pte_t *)pte_alloc_one_virt(mm);
 }
 
+extern struct mm_struct efi_mm;
+
+static inline gfp_t pgtable_gfp_flags(struct mm_struct *mm)
+{
+	if (unlikely((mm == &init_mm) || (mm == &efi_mm)))
+		return PGALLOC_GFP;
+	else
+		return PGALLOC_GFP | __GFP_ACCOUNT;
+}
+
 static inline pgtable_t
 pte_alloc_one(struct mm_struct *mm)
 {
 	struct page *pte;
+	gfp_t gfp;
 
-	pte = alloc_pages(PGALLOC_GFP, 0);
+	gfp = pgtable_gfp_flags(mm);
+	pte = alloc_pages(gfp, 0);
 	if (!pte)
 		return NULL;
 	if (!pgtable_page_ctor(pte)) {