Re: [PATCH 02/12] mm: pgtable: introduce generic p4d_alloc_one() and p4d_free()

[Qi Zheng <zhengqi.arch@bytedance.com>]

On 2024/12/18 22:26, Klara Modin wrote:
> Hi,
> 
> On 2024-12-14 10:02, Qi Zheng wrote:
>> Several architectures (arm64, riscv, x86) define p4d_alloc_one() as a
>> wrapper for get_zeroed_page() and p4d_free() as a wrapper for 
>> free_page().
>>
>> For these architectures, provide a generic implementation in
>> asm-generic/pgalloc.h and convert them to use it. And like other levels
>> of page tables, add statistics for P4D level page table.
>>
>> For s390, it also defines p4d_alloc_one() and p4d_free(), but it uses its
>> own logic, so skip it.
>>
>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>> ---
>>   arch/arm64/include/asm/pgalloc.h | 15 ++++-----
>>   arch/riscv/include/asm/pgalloc.h | 25 ++++++---------
>>   arch/x86/include/asm/pgalloc.h   | 16 ++++------
>>   arch/x86/mm/pgtable.c            |  3 ++
>>   include/asm-generic/pgalloc.h    | 55 ++++++++++++++++++++++++++++++++
>>   include/linux/mm.h               | 16 ++++++++++
>>   6 files changed, 98 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgalloc.h 
>> b/arch/arm64/include/asm/pgalloc.h
>> index e75422864d1bd..679c530549327 100644
>> --- a/arch/arm64/include/asm/pgalloc.h
>> +++ b/arch/arm64/include/asm/pgalloc.h
>> @@ -15,6 +15,8 @@
>>   #define __HAVE_ARCH_PGD_FREE
>>   #define __HAVE_ARCH_PUD_FREE
>> +#define __HAVE_ARCH_P4D_ALLOC_ONE
>> +#define __HAVE_ARCH_P4D_FREE
>>   #include <asm-generic/pgalloc.h>
>>   #define PGD_SIZE    (PTRS_PER_PGD * sizeof(pgd_t))
>> @@ -87,19 +89,16 @@ static inline void pgd_populate(struct mm_struct 
>> *mm, pgd_t *pgdp, p4d_t *p4dp)
>>   static inline p4d_t *p4d_alloc_one(struct mm_struct *mm, unsigned 
>> long addr)
>>   {
>> -    gfp_t gfp = GFP_PGTABLE_USER;
>> +    if (!pgtable_l5_enabled())
>> +        return NULL;
>> -    if (mm == &init_mm)
>> -        gfp = GFP_PGTABLE_KERNEL;
>> -    return (p4d_t *)get_zeroed_page(gfp);
>> +    return __p4d_alloc_one(mm, addr);
>>   }
>>   static inline void p4d_free(struct mm_struct *mm, p4d_t *p4d)
>>   {
>> -    if (!pgtable_l5_enabled())
>> -        return;
>> -    BUG_ON((unsigned long)p4d & (PAGE_SIZE-1));
>> -    free_page((unsigned long)p4d);
>> +    if (pgtable_l5_enabled())
>> +        __p4d_free(mm, p4d);
>>   }
>>   #define __p4d_free_tlb(tlb, p4d, addr)  p4d_free((tlb)->mm, p4d)
>> diff --git a/arch/riscv/include/asm/pgalloc.h 
>> b/arch/riscv/include/asm/pgalloc.h
>> index f52264304f772..bb6e1c5f1fb19 100644
>> --- a/arch/riscv/include/asm/pgalloc.h
>> +++ b/arch/riscv/include/asm/pgalloc.h
>> @@ -14,6 +14,8 @@
>>   #ifdef CONFIG_MMU
>>   #define __HAVE_ARCH_PUD_ALLOC_ONE
>>   #define __HAVE_ARCH_PUD_FREE
>> +#define __HAVE_ARCH_P4D_ALLOC_ONE
>> +#define __HAVE_ARCH_P4D_FREE
>>   #include <asm-generic/pgalloc.h>
>>   static inline void riscv_tlb_remove_ptdesc(struct mmu_gather *tlb, 
>> void *pt)
>> @@ -118,21 +120,10 @@ static inline void __pud_free_tlb(struct 
>> mmu_gather *tlb, pud_t *pud,
>>   #define p4d_alloc_one p4d_alloc_one
>>   static inline p4d_t *p4d_alloc_one(struct mm_struct *mm, unsigned 
>> long addr)
>>   {
>> -    if (pgtable_l5_enabled) {
>> -        gfp_t gfp = GFP_PGTABLE_USER;
>> -
>> -        if (mm == &init_mm)
>> -            gfp = GFP_PGTABLE_KERNEL;
>> -        return (p4d_t *)get_zeroed_page(gfp);
>> -    }
>> +    if (!pgtable_l5_enabled)
>> +        return NULL;
>> -    return NULL;
>> -}
>> -
>> -static inline void __p4d_free(struct mm_struct *mm, p4d_t *p4d)
>> -{
>> -    BUG_ON((unsigned long)p4d & (PAGE_SIZE-1));
>> -    free_page((unsigned long)p4d);
>> +    return __p4d_alloc_one(mm, addr);
>>   }
>>   #define p4d_free p4d_free
>> @@ -145,8 +136,12 @@ static inline void p4d_free(struct mm_struct *mm, 
>> p4d_t *p4d)
>>   static inline void __p4d_free_tlb(struct mmu_gather *tlb, p4d_t *p4d,
>>                     unsigned long addr)
>>   {
>> -    if (pgtable_l5_enabled)
>> +    if (pgtable_l5_enabled) {
>> +        struct ptdesc *ptdesc = virt_to_ptdesc(p4d);
>> +
>> +        pagetable_p4d_dtor(ptdesc);
>>           riscv_tlb_remove_ptdesc(tlb, virt_to_ptdesc(p4d));
>> +    }
>>   }
>>   #endif /* __PAGETABLE_PMD_FOLDED */
>> diff --git a/arch/x86/include/asm/pgalloc.h 
>> b/arch/x86/include/asm/pgalloc.h
>> index dcd836b59bebd..d9bc6cae77c9e 100644
>> --- a/arch/x86/include/asm/pgalloc.h
>> +++ b/arch/x86/include/asm/pgalloc.h
>> @@ -8,6 +8,8 @@
>>   #define __HAVE_ARCH_PTE_ALLOC_ONE
>>   #define __HAVE_ARCH_PGD_FREE
>> +#define __HAVE_ARCH_P4D_ALLOC_ONE
>> +#define __HAVE_ARCH_P4D_FREE
>>   #include <asm-generic/pgalloc.h>
>>   static inline int  __paravirt_pgd_alloc(struct mm_struct *mm) { 
>> return 0; }
>> @@ -149,20 +151,16 @@ static inline void pgd_populate_safe(struct 
>> mm_struct *mm, pgd_t *pgd, p4d_t *p4
>>   static inline p4d_t *p4d_alloc_one(struct mm_struct *mm, unsigned 
>> long addr)
>>   {
>> -    gfp_t gfp = GFP_KERNEL_ACCOUNT;
>> +    if (!pgtable_l5_enabled())
>> +        return NULL;
>> -    if (mm == &init_mm)
>> -        gfp &= ~__GFP_ACCOUNT;
>> -    return (p4d_t *)get_zeroed_page(gfp);
>> +    return __p4d_alloc_one(mm, addr);
>>   }
>>   static inline void p4d_free(struct mm_struct *mm, p4d_t *p4d)
>>   {
>> -    if (!pgtable_l5_enabled())
>> -        return;
>> -
>> -    BUG_ON((unsigned long)p4d & (PAGE_SIZE-1));
>> -    free_page((unsigned long)p4d);
>> +    if (pgtable_l5_enabled())
>> +        return __p4d_free(mm, p4d);
>>   }
>>   extern void ___p4d_free_tlb(struct mmu_gather *tlb, p4d_t *p4d);
>> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
>> index 69a357b15974a..3d6e84da45b24 100644
>> --- a/arch/x86/mm/pgtable.c
>> +++ b/arch/x86/mm/pgtable.c
>> @@ -94,6 +94,9 @@ void ___pud_free_tlb(struct mmu_gather *tlb, pud_t 
>> *pud)
>>   #if CONFIG_PGTABLE_LEVELS > 4
>>   void ___p4d_free_tlb(struct mmu_gather *tlb, p4d_t *p4d)
>>   {
>> +    struct ptdesc *ptdesc = virt_to_ptdesc(p4d);
>> +
>> +    pagetable_p4d_dtor(ptdesc);
>>       paravirt_release_p4d(__pa(p4d) >> PAGE_SHIFT);
>>       paravirt_tlb_remove_table(tlb, virt_to_page(p4d));
>>   }
>> diff --git a/include/asm-generic/pgalloc.h 
>> b/include/asm-generic/pgalloc.h
>> index 7c48f5fbf8aa7..dbf61819b3581 100644
>> --- a/include/asm-generic/pgalloc.h
>> +++ b/include/asm-generic/pgalloc.h
>> @@ -215,6 +215,61 @@ static inline void pud_free(struct mm_struct *mm, 
>> pud_t *pud)
>>   #endif /* CONFIG_PGTABLE_LEVELS > 3 */
>> +#if CONFIG_PGTABLE_LEVELS > 4
>> +
>> +static inline p4d_t *__p4d_alloc_one_noprof(struct mm_struct *mm, 
>> unsigned long addr)
>> +{
>> +    gfp_t gfp = GFP_PGTABLE_USER;
>> +    struct ptdesc *ptdesc;
>> +
>> +    if (mm == &init_mm)
>> +        gfp = GFP_PGTABLE_KERNEL;
>> +    gfp &= ~__GFP_HIGHMEM;
>> +
>> +    ptdesc = pagetable_alloc_noprof(gfp, 0);
>> +    if (!ptdesc)
>> +        return NULL;
>> +
>> +    pagetable_p4d_ctor(ptdesc);
>> +    return ptdesc_address(ptdesc);
>> +}
>> +#define __p4d_alloc_one(...)    
>> alloc_hooks(__p4d_alloc_one_noprof(__VA_ARGS__))
>> +
>> +#ifndef __HAVE_ARCH_P4D_ALLOC_ONE
>> +/**
>> + * p4d_alloc_one - allocate memory for a P4D-level page table
>> + * @mm: the mm_struct of the current context
>> + *
>> + * Allocate memory for a page table using %GFP_PGTABLE_USER for user 
>> context
>> + * and %GFP_PGTABLE_KERNEL for kernel context.
>> + *
>> + * Return: pointer to the allocated memory or %NULL on error
>> + */
>> +static inline p4d_t *p4d_alloc_one_noprof(struct mm_struct *mm, 
>> unsigned long addr)
>> +{
>> +    return __p4d_alloc_one_noprof(mm, addr);
>> +}
>> +#define p4d_alloc_one(...)    
>> alloc_hooks(p4d_alloc_one_noprof(__VA_ARGS__))
>> +#endif
>> +
>> +static inline void __p4d_free(struct mm_struct *mm, p4d_t *p4d)
>> +{
>> +    struct ptdesc *ptdesc = virt_to_ptdesc(p4d);
>> +
>> +    BUG_ON((unsigned long)p4d & (PAGE_SIZE-1));
>> +    pagetable_p4d_dtor(ptdesc);
>> +    pagetable_free(ptdesc);
>> +}
>> +
> 
> 
>> +#ifndef __HAVE_ARCH_P4D_FREE
>> +static inline void p4d_free(struct mm_struct *mm, pud_t *p4d)
> 
> Should this perhaps be p4d_t *p4d rather than pud_t *p4d?

Yes, my bad. Will fix it.

Thanks!

> 
> Otherwise I get this build error:
> 
> In file included from 
> /home/klara/git/linux/arch/riscv/include/asm/kfence.h:8,
>                   from /home/klara/git/linux/mm/kfence/core.c:34:
> /home/klara/git/linux/include/asm-generic/pgalloc.h: In function 
> ‘p4d_free’:
> /home/klara/git/linux/include/asm-generic/pgalloc.h:252:24: error: 
> passing argument 2 of ‘__p4d_free’ from incompatible pointer type 
> [-Wincompatible-pointer-types]
>    252 |         __p4d_free(mm, p4d);
>        |                        ^~~
>        |                        |
>        |                        pud_t *
> /home/klara/git/linux/include/asm-generic/pgalloc.h:244:60: note: 
> expected ‘p4d_t *’ but argument is of type ‘pud_t *’
>    244 | static inline void __p4d_free(struct mm_struct *mm, p4d_t *p4d)
>        |                                                     ~~~~~~~^~~
> In file included from 
> /home/klara/git/linux/arch/riscv/include/asm/kfence.h:8,
>                   from /home/klara/git/linux/mm/kfence/report.c:22:
> /home/klara/git/linux/include/asm-generic/pgalloc.h: In function 
> ‘p4d_free’:
> /home/klara/git/linux/include/asm-generic/pgalloc.h:252:24: error: 
> passing argument 2 of ‘__p4d_free’ from incompatible pointer type 
> [-Wincompatible-pointer-types]
>    252 |         __p4d_free(mm, p4d);
>        |                        ^~~
>        |                        |
>        |                        pud_t *
> /home/klara/git/linux/include/asm-generic/pgalloc.h:244:60: note: 
> expected ‘p4d_t *’ but argument is of type ‘pud_t *’
>    244 | static inline void __p4d_free(struct mm_struct *mm, p4d_t *p4d)
>        |                                                     ~~~~~~~^~~
> 
> Regards,
> Klara Modin
> 
>> +{
>> +    __p4d_free(mm, p4d);
>> +}
>> +#endif
>> +
>> +#endif
>> +
>>   #ifndef __HAVE_ARCH_PGD_FREE
>>   static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
>>   {
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 5e73e53c34e9e..807a12ed8ec96 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -3237,6 +3237,22 @@ static inline void pagetable_pud_dtor(struct 
>> ptdesc *ptdesc)
>>       lruvec_stat_sub_folio(folio, NR_PAGETABLE);
>>   }
>> +static inline void pagetable_p4d_ctor(struct ptdesc *ptdesc)
>> +{
>> +    struct folio *folio = ptdesc_folio(ptdesc);
>> +
>> +    __folio_set_pgtable(folio);
>> +    lruvec_stat_add_folio(folio, NR_PAGETABLE);
>> +}
>> +
>> +static inline void pagetable_p4d_dtor(struct ptdesc *ptdesc)
>> +{
>> +    struct folio *folio = ptdesc_folio(ptdesc);
>> +
>> +    __folio_clear_pgtable(folio);
>> +    lruvec_stat_sub_folio(folio, NR_PAGETABLE);
>> +}
>> +
>>   extern void __init pagecache_init(void);
>>   extern void free_initmem(void);
>