Re: [PATCH V2 5/6] arm64/mm: Enable page table page accounting for user space

From: Anshuman Khandual <anshuman.khandual@arm.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: yuzhao@google.com, Steve.Capper@arm.com, marc.zyngier@arm.com,
	Catalin.Marinas@arm.com, suzuki.poulose@arm.com,
	will.deacon@arm.com, james.morse@arm.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH V2 5/6] arm64/mm: Enable page table page accounting for user space
Date: Tue, 26 Feb 2019 10:36:02 +0530	[thread overview]
Message-ID: <27de9775-1ff9-01a1-6bf4-b6a912fc2440@arm.com> (raw)
In-Reply-To: <20190225153553.GI26236@lakrids.cambridge.arm.com>

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 ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel