Re: [PATCH 11/12] mm: pgtable: introduce generic __tlb_remove_table()

[Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>]

On 2024/12/16 20:00, Peter Zijlstra wrote:
> On Sat, Dec 14, 2024 at 05:02:57PM +0800, Qi Zheng wrote:

[...]

> >   
> > +#ifndef __HAVE_ARCH_TLB_REMOVE_TABLE
> > +static inline void __tlb_remove_table(void *_table)
> > +{
> > +	struct ptdesc *ptdesc = (struct ptdesc *)_table;
> > +
> > +	pagetable_dtor(ptdesc);
> > +	pagetable_free(ptdesc);
> > +}
> > +#endif
>
>
> Spot the fail...
>
> That said, all this ptdesc stuff is another giant trainwreck. Let me
> clean that up for you.

It looks like you want to revert what was done in this patch series:

https://lore.kernel.org/all/20230807230513.102486-1-vishal.moola@xxxxxxxxx/

But why? It seems that splitting ptdesc from struct page is a good
thing?

> ---
> diff --git a/arch/arm/include/asm/tlb.h b/arch/arm/include/asm/tlb.h
> index ea4fbe7b17f6..ac3881ec342f 100644
> --- a/arch/arm/include/asm/tlb.h
> +++ b/arch/arm/include/asm/tlb.h
> @@ -32,8 +32,6 @@
>   static inline void
>   __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte, unsigned long addr)
>   {
> -	struct ptdesc *ptdesc = page_ptdesc(pte);
> -
>   #ifndef CONFIG_ARM_LPAE
>   	/*
>   	 * With the classic ARM MMU, a pte page has two corresponding pmd
> @@ -43,16 +41,14 @@ __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte, unsigned long addr)
>   	__tlb_adjust_range(tlb, addr - PAGE_SIZE, 2 * PAGE_SIZE);
>   #endif
>   
> -	tlb_remove_ptdesc(tlb, ptdesc);
> +	tlb_remove_table(tlb, pte);
>   }
>   
>   static inline void
>   __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmdp, unsigned long addr)
>   {
>   #ifdef CONFIG_ARM_LPAE
> -	struct ptdesc *ptdesc = virt_to_ptdesc(pmdp);
> -
> -	tlb_remove_ptdesc(tlb, ptdesc);
> +	tlb_remove_table(tlb, virt_to_page(pmdp));
>   #endif
>   }
>   
> diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
> index 8d762607285c..4a60569fed69 100644
> --- a/arch/arm64/include/asm/tlb.h
> +++ b/arch/arm64/include/asm/tlb.h
> @@ -75,18 +75,14 @@ static inline void tlb_flush(struct mmu_gather *tlb)
>   static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte,
>   				  unsigned long addr)
>   {
> -	struct ptdesc *ptdesc = page_ptdesc(pte);
> -
> -	tlb_remove_ptdesc(tlb, ptdesc);
> +	tlb_remove_table(tlb, pte);
>   }
>   
>   #if CONFIG_PGTABLE_LEVELS > 2
>   static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmdp,
>   				  unsigned long addr)
>   {
> -	struct ptdesc *ptdesc = virt_to_ptdesc(pmdp);
> -
> -	tlb_remove_ptdesc(tlb, ptdesc);
> +	tlb_remove_table(tlb, virt_to_page(pmdp));
>   }
>   #endif
>   
> @@ -94,12 +90,10 @@ static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmdp,
>   static inline void __pud_free_tlb(struct mmu_gather *tlb, pud_t *pudp,
>   				  unsigned long addr)
>   {
> -	struct ptdesc *ptdesc = virt_to_ptdesc(pudp);
> -
>   	if (!pgtable_l4_enabled())
>   		return;
>   
> -	tlb_remove_ptdesc(tlb, ptdesc);
> +	tlb_remove_table(tlb, virt_to_page(pudp));
>   }
>   #endif
>   
> @@ -107,12 +101,10 @@ static inline void __pud_free_tlb(struct mmu_gather *tlb, pud_t *pudp,
>   static inline void __p4d_free_tlb(struct mmu_gather *tlb, p4d_t *p4dp,
>   				  unsigned long addr)
>   {
> -	struct ptdesc *ptdesc = virt_to_ptdesc(p4dp);
> -
>   	if (!pgtable_l5_enabled())
>   		return;
>   
> -	tlb_remove_ptdesc(tlb, ptdesc);
> +	tlb_remove_table(tlb, virt_to_page(p4dp));
>   }
>   #endif
>   
> diff --git a/arch/csky/include/asm/pgalloc.h b/arch/csky/include/asm/pgalloc.h
> index f1ce5b7b28f2..2c0897624699 100644
> --- a/arch/csky/include/asm/pgalloc.h
> +++ b/arch/csky/include/asm/pgalloc.h
> @@ -64,7 +64,7 @@ static inline pgd_t *pgd_alloc(struct mm_struct *mm)
>   #define __pte_free_tlb(tlb, pte, address)		\
>   do {							\
>   	pagetable_dtor(page_ptdesc(pte));		\
> -	tlb_remove_page_ptdesc(tlb, page_ptdesc(pte));	\
> +	tlb_remove_page(tlb, pte);			\
>   } while (0)
>   
>   extern void pagetable_init(void);
> diff --git a/arch/hexagon/include/asm/pgalloc.h b/arch/hexagon/include/asm/pgalloc.h
> index 40e42a0e7167..8b1550498f1b 100644
> --- a/arch/hexagon/include/asm/pgalloc.h
> +++ b/arch/hexagon/include/asm/pgalloc.h
> @@ -90,7 +90,7 @@ static inline void pmd_populate_kernel(struct mm_struct *mm, pmd_t *pmd,
>   #define __pte_free_tlb(tlb, pte, addr)				\
>   do {								\
>   	pagetable_dtor((page_ptdesc(pte)));			\
> -	tlb_remove_page_ptdesc((tlb), (page_ptdesc(pte)));	\
> +	tlb_remove_page((tlb), (pte));				\
>   } while (0)
>   
>   #endif
> diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h
> index 7211dff8c969..5a4f22aeb618 100644
> --- a/arch/loongarch/include/asm/pgalloc.h
> +++ b/arch/loongarch/include/asm/pgalloc.h
> @@ -58,7 +58,7 @@ static inline pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
>   #define __pte_free_tlb(tlb, pte, address)			\
>   do {								\
>   	pagetable_dtor(page_ptdesc(pte));			\
> -	tlb_remove_page_ptdesc((tlb), page_ptdesc(pte));	\
> +	tlb_remove_page((tlb), (pte));				\
>   } while (0)
>   
>   #ifndef __PAGETABLE_PMD_FOLDED
> diff --git a/arch/m68k/include/asm/sun3_pgalloc.h b/arch/m68k/include/asm/sun3_pgalloc.h
> index 2b626cb3ad0a..63d9f95f5e3d 100644
> --- a/arch/m68k/include/asm/sun3_pgalloc.h
> +++ b/arch/m68k/include/asm/sun3_pgalloc.h
> @@ -20,7 +20,7 @@ extern const char bad_pmd_string[];
>   #define __pte_free_tlb(tlb, pte, addr)				\
>   do {								\
>   	pagetable_dtor(page_ptdesc(pte));			\
> -	tlb_remove_page_ptdesc((tlb), page_ptdesc(pte));	\
> +	tlb_remove_page((tlb), (pte));				\
>   } while (0)
>   
>   static inline void pmd_populate_kernel(struct mm_struct *mm, pmd_t *pmd, pte_t *pte)
> diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h
> index 36d9805033c4..bbee21345154 100644
> --- a/arch/mips/include/asm/pgalloc.h
> +++ b/arch/mips/include/asm/pgalloc.h
> @@ -57,7 +57,7 @@ static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
>   #define __pte_free_tlb(tlb, pte, address)			\
>   do {								\
>   	pagetable_dtor(page_ptdesc(pte));			\
> -	tlb_remove_page_ptdesc((tlb), page_ptdesc(pte));	\
> +	tlb_remove_page((tlb), (pte));				\
>   } while (0)
>   
>   #ifndef __PAGETABLE_PMD_FOLDED
> diff --git a/arch/nios2/include/asm/pgalloc.h b/arch/nios2/include/asm/pgalloc.h
> index 12a536b7bfbd..641cec8fb2a2 100644
> --- a/arch/nios2/include/asm/pgalloc.h
> +++ b/arch/nios2/include/asm/pgalloc.h
> @@ -31,7 +31,7 @@ extern pgd_t *pgd_alloc(struct mm_struct *mm);
>   #define __pte_free_tlb(tlb, pte, addr)					\
>   	do {								\
>   		pagetable_dtor(page_ptdesc(pte));			\
> -		tlb_remove_page_ptdesc((tlb), (page_ptdesc(pte)));	\
> +		tlb_remove_page((tlb), (pte));				\
>   	} while (0)
>   
>   #endif /* _ASM_NIOS2_PGALLOC_H */
> diff --git a/arch/openrisc/include/asm/pgalloc.h b/arch/openrisc/include/asm/pgalloc.h
> index 596e2355824e..e9b9bc53ece0 100644
> --- a/arch/openrisc/include/asm/pgalloc.h
> +++ b/arch/openrisc/include/asm/pgalloc.h
> @@ -69,7 +69,7 @@ extern pte_t *pte_alloc_one_kernel(struct mm_struct *mm);
>   #define __pte_free_tlb(tlb, pte, addr)				\
>   do {								\
>   	pagetable_dtor(page_ptdesc(pte));			\
> -	tlb_remove_page_ptdesc((tlb), (page_ptdesc(pte)));	\
> +	tlb_remove_page((tlb), (pte));				\
>   } while (0)
>   
>   #endif
> diff --git a/arch/riscv/include/asm/pgalloc.h b/arch/riscv/include/asm/pgalloc.h
> index fc50d1401024..baedbd2546b9 100644
> --- a/arch/riscv/include/asm/pgalloc.h
> +++ b/arch/riscv/include/asm/pgalloc.h
> @@ -26,13 +26,13 @@
>    * comment below 'ifdef CONFIG_MMU_GATHER_RCU_TABLE_FREE' in include/asm-generic/tlb.h
>    * for more details.
>    */
> -static inline void riscv_tlb_remove_ptdesc(struct mmu_gather *tlb, void *pt)
> +static inline void riscv_tlb_remove_table(struct mmu_gather *tlb, void *pt)
>   {
>   	if (riscv_use_sbi_for_rfence()) {
> -		tlb_remove_ptdesc(tlb, pt);
> +		tlb_remove_table(tlb, pt);
>   	} else {
>   		pagetable_dtor(pt);
> -		tlb_remove_page_ptdesc(tlb, pt);
> +		tlb_remove_page(tlb, pt);
>   	}
>   }
>   
> @@ -120,7 +120,7 @@ static inline void __pud_free_tlb(struct mmu_gather *tlb, pud_t *pud,
>   				  unsigned long addr)
>   {
>   	if (pgtable_l4_enabled)
> -		riscv_tlb_remove_ptdesc(tlb, virt_to_ptdesc(pud));
> +		riscv_tlb_remove_table(tlb, virt_to_page(pud));
>   }
>   
>   #define p4d_alloc_one p4d_alloc_one
> @@ -143,7 +143,7 @@ static inline void __p4d_free_tlb(struct mmu_gather *tlb, p4d_t *p4d,
>   				  unsigned long addr)
>   {
>   	if (pgtable_l5_enabled)
> -		riscv_tlb_remove_ptdesc(tlb, virt_to_ptdesc(p4d));
> +		riscv_tlb_remove_table(tlb, virt_to_page(p4d));
>   }
>   #endif /* __PAGETABLE_PMD_FOLDED */
>   
> @@ -172,7 +172,7 @@ static inline pgd_t *pgd_alloc(struct mm_struct *mm)
>   static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd,
>   				  unsigned long addr)
>   {
> -	riscv_tlb_remove_ptdesc(tlb, virt_to_ptdesc(pmd));
> +	riscv_tlb_remove_table(tlb, virt_to_page(pmd));
>   }
>   
>   #endif /* __PAGETABLE_PMD_FOLDED */
> @@ -180,7 +180,7 @@ static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd,
>   static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte,
>   				  unsigned long addr)
>   {
> -	riscv_tlb_remove_ptdesc(tlb, page_ptdesc(pte));
> +	riscv_tlb_remove_table(tlb, pte);
>   }
>   #endif /* CONFIG_MMU */
>   
> diff --git a/arch/s390/include/asm/tlb.h b/arch/s390/include/asm/tlb.h
> index 705278074034..fba11949dd2e 100644
> --- a/arch/s390/include/asm/tlb.h
> +++ b/arch/s390/include/asm/tlb.h
> @@ -86,7 +86,7 @@ static inline void pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte,
>   	tlb->cleared_pmds = 1;
>   	if (mm_alloc_pgste(tlb->mm))
>   		gmap_unlink(tlb->mm, (unsigned long *)pte, address);
> -	tlb_remove_ptdesc(tlb, pte);
> +	tlb_remove_table(tlb, pte);
>   }
>   
>   /*
> @@ -105,7 +105,7 @@ static inline void pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd,
>   	tlb->mm->context.flush_mm = 1;
>   	tlb->freed_tables = 1;
>   	tlb->cleared_puds = 1;
> -	tlb_remove_ptdesc(tlb, pmd);
> +	tlb_remove_table(tlb, pmd);
>   }
>   
>   /*
> @@ -123,7 +123,7 @@ static inline void pud_free_tlb(struct mmu_gather *tlb, pud_t *pud,
>   	tlb->mm->context.flush_mm = 1;
>   	tlb->freed_tables = 1;
>   	tlb->cleared_p4ds = 1;
> -	tlb_remove_ptdesc(tlb, pud);
> +	tlb_remove_table(tlb, pud);
>   }
>   
>   /*
> @@ -141,7 +141,7 @@ static inline void p4d_free_tlb(struct mmu_gather *tlb, p4d_t *p4d,
>   	__tlb_adjust_range(tlb, address, PAGE_SIZE);
>   	tlb->mm->context.flush_mm = 1;
>   	tlb->freed_tables = 1;
> -	tlb_remove_ptdesc(tlb, p4d);
> +	tlb_remove_table(tlb, p4d);
>   }
>   
>   #endif /* _S390_TLB_H */
> diff --git a/arch/sh/include/asm/pgalloc.h b/arch/sh/include/asm/pgalloc.h
> index 96d938fdf224..43812b2363ef 100644
> --- a/arch/sh/include/asm/pgalloc.h
> +++ b/arch/sh/include/asm/pgalloc.h
> @@ -35,7 +35,7 @@ static inline void pmd_populate(struct mm_struct *mm, pmd_t *pmd,
>   #define __pte_free_tlb(tlb, pte, addr)				\
>   do {								\
>   	pagetable_dtor(page_ptdesc(pte));			\
> -	tlb_remove_page_ptdesc((tlb), (page_ptdesc(pte)));	\
> +	tlb_remove_page((tlb), (pte));				\
>   } while (0)
>   
>   #endif /* __ASM_SH_PGALLOC_H */
> diff --git a/arch/um/include/asm/pgalloc.h b/arch/um/include/asm/pgalloc.h
> index f0af23c3aeb2..aa6063dc5b1e 100644
> --- a/arch/um/include/asm/pgalloc.h
> +++ b/arch/um/include/asm/pgalloc.h
> @@ -28,7 +28,7 @@ extern pgd_t *pgd_alloc(struct mm_struct *);
>   #define __pte_free_tlb(tlb, pte, address)			\
>   do {								\
>   	pagetable_dtor(page_ptdesc(pte));			\
> -	tlb_remove_page_ptdesc((tlb), (page_ptdesc(pte)));	\
> +	tlb_remove_page((tlb), (pte));				\
>   } while (0)
>   
>   #if CONFIG_PGTABLE_LEVELS > 2
> @@ -36,15 +36,15 @@ do {								\
>   #define __pmd_free_tlb(tlb, pmd, address)			\
>   do {								\
>   	pagetable_dtor(virt_to_ptdesc(pmd));			\
> -	tlb_remove_page_ptdesc((tlb), virt_to_ptdesc(pmd));	\
> +	tlb_remove_page((tlb), virt_page(pmd));			\
>   } while (0)
>   
>   #if CONFIG_PGTABLE_LEVELS > 3
>   
>   #define __pud_free_tlb(tlb, pud, address)			\
>   do {								\
> -	pagetable_dtor(virt_to_ptdesc(pud));		\
> -	tlb_remove_page_ptdesc((tlb), virt_to_ptdesc(pud));	\
> +	pagetable_dtor(virt_to_ptdesc(pud));			\
> +	tlb_remove_page((tlb), virt_to_page(pud));		\
>   } while (0)
>   
>   #endif
> diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
> index 939a813023d7..7991950e98f6 100644
> --- a/include/asm-generic/tlb.h
> +++ b/include/asm-generic/tlb.h
> @@ -209,9 +209,9 @@ struct mmu_table_batch {
>   	((PAGE_SIZE - sizeof(struct mmu_table_batch)) / sizeof(void *))
>   
>   #ifndef __HAVE_ARCH_TLB_REMOVE_TABLE
> -static inline void __tlb_remove_table(void *_table)
> +static inline void __tlb_remove_table(void *table)
>   {
> -	struct ptdesc *ptdesc = (struct ptdesc *)_table;
> +	struct ptdesc *ptdesc = page_to_ptdesc(table);
>   
>   	pagetable_dtor(ptdesc);
>   	pagetable_free(ptdesc);
> @@ -499,17 +499,6 @@ static inline void tlb_remove_page(struct mmu_gather *tlb, struct page *page)
>   	return tlb_remove_page_size(tlb, page, PAGE_SIZE);
>   }
>   
> -static inline void tlb_remove_ptdesc(struct mmu_gather *tlb, void *pt)
> -{
> -	tlb_remove_table(tlb, pt);
> -}
> -
> -/* Like tlb_remove_ptdesc, but for page-like page directories. */
> -static inline void tlb_remove_page_ptdesc(struct mmu_gather *tlb, struct ptdesc *pt)
> -{
> -	tlb_remove_page(tlb, ptdesc_page(pt));
> -}
> -
>   static inline void tlb_change_page_size(struct mmu_gather *tlb,
>   						     unsigned int page_size)
>   {