Re: [PATCH v4 10/15] riscv: pgtable: move pagetable_dtor() to __tlb_remove_table()
From: Qi Zheng <hidden>
Date: 2025-01-03 03:48:42
Also in:
linux-arch, linux-arm-kernel, linux-m68k, linux-mips, linux-mm, linux-riscv, linux-s390, linux-sh, linux-um, lkml, loongarch, sparclinux
Subsystem:
generic include/asm header files, mmu gather and tlb invalidation, superh, the rest, user-mode linux (uml) · Maintainers:
Arnd Bergmann, Will Deacon, "Aneesh Kumar K.V", Andrew Morton, Nick Piggin, Peter Zijlstra, Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz, Linus Torvalds, Richard Weinberger, Anton Ivanov, Johannes Berg
Hi Kevin, On 2025/1/3 00:53, Kevin Brodsky wrote:
On 30/12/2024 10:07, Qi Zheng wrote:quoted
static inline void riscv_tlb_remove_ptdesc(struct mmu_gather *tlb, void *pt) { - if (riscv_use_sbi_for_rfence()) + if (riscv_use_sbi_for_rfence()) { tlb_remove_ptdesc(tlb, pt); - else + } else { + pagetable_dtor(pt); tlb_remove_page_ptdesc(tlb, pt);I find the imbalance pretty confusing: pagetable_dtor() is called explicitly before using tlb_remove_page() but not tlb_remove_ptdesc(). Doesn't that assume that CONFIG_MMU_GATHER_HAVE_TABLE_FREE is selected? Could we not call pagetable_dtor() from __tlb_batch_free_encoded_pages() to ensure that the dtor is always called just before freeing, and remove
In __tlb_batch_free_encoded_pages(), we can indeed detect PageTable() and call pagetable_dtor() to dtor the page table pages. But __tlb_batch_free_encoded_pages() is also used to free normal pages (not page table pages), so I don't want to add overhead there. But now I think maybe we can do this in tlb_remove_page_ptdesc(), like this:
diff --git a/arch/csky/include/asm/pgalloc.h b/arch/csky/include/asm/pgalloc.h index f1ce5b7b28f22..e45c7e91dcbf9 100644
--- a/arch/csky/include/asm/pgalloc.h
+++ b/arch/csky/include/asm/pgalloc.h@@ -63,7 +63,6 @@ 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)); \ } while (0)
diff --git a/arch/hexagon/include/asm/pgalloc.h b/arch/hexagon/include/asm/pgalloc.h index 40e42a0e71673..9903449f45cff 100644
--- a/arch/hexagon/include/asm/pgalloc.h
+++ b/arch/hexagon/include/asm/pgalloc.h@@ -89,7 +89,6 @@ 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))); \
} while (0)
diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h index 7211dff8c969e..de5b3f5c85d1c 100644
--- a/arch/loongarch/include/asm/pgalloc.h
+++ b/arch/loongarch/include/asm/pgalloc.h@@ -57,7 +57,6 @@ 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)); \
} while (0)
diff --git a/arch/m68k/include/asm/sun3_pgalloc.h b/arch/m68k/include/asm/sun3_pgalloc.h index 2b626cb3ad0ae..731cc8f0731d3 100644
--- a/arch/m68k/include/asm/sun3_pgalloc.h
+++ b/arch/m68k/include/asm/sun3_pgalloc.h@@ -19,7 +19,6 @@ 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)); \ } while (0)
diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h index 36d9805033c4b..964ad514be281 100644
--- a/arch/mips/include/asm/pgalloc.h
+++ b/arch/mips/include/asm/pgalloc.h@@ -56,7 +56,6 @@ 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)); \
} while (0)
diff --git a/arch/nios2/include/asm/pgalloc.h b/arch/nios2/include/asm/pgalloc.h index 12a536b7bfbd4..ef6b4b8301ac6 100644
--- a/arch/nios2/include/asm/pgalloc.h
+++ b/arch/nios2/include/asm/pgalloc.h@@ -30,7 +30,6 @@ 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))); \ } while (0)
diff --git a/arch/openrisc/include/asm/pgalloc.h b/arch/openrisc/include/asm/pgalloc.h index 596e2355824e3..9361205610910 100644
--- a/arch/openrisc/include/asm/pgalloc.h
+++ b/arch/openrisc/include/asm/pgalloc.h@@ -68,7 +68,6 @@ 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))); \ } while (0)
diff --git a/arch/riscv/include/asm/pgalloc.h b/arch/riscv/include/asm/pgalloc.h index c8907b8317115..61c26576614da 100644
--- a/arch/riscv/include/asm/pgalloc.h
+++ b/arch/riscv/include/asm/pgalloc.h@@ -25,12 +25,10 @@ */ static inline void riscv_tlb_remove_ptdesc(struct mmu_gather *tlb,
void *pt)
{
- if (riscv_use_sbi_for_rfence()) {
+ if (riscv_use_sbi_for_rfence())
tlb_remove_ptdesc(tlb, pt);
- } else {
- pagetable_dtor(pt);
+ else
tlb_remove_page_ptdesc(tlb, pt);
- }
}
static inline void pmd_populate_kernel(struct mm_struct *mm,diff --git a/arch/sh/include/asm/pgalloc.h b/arch/sh/include/asm/pgalloc.h
index 96d938fdf2244..5b5c73e9fdb4b 100644
--- a/arch/sh/include/asm/pgalloc.h
+++ b/arch/sh/include/asm/pgalloc.h@@ -34,7 +34,6 @@ 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))); \
} while (0)
diff --git a/arch/um/include/asm/pgalloc.h b/arch/um/include/asm/pgalloc.h
index f0af23c3aeb2b..4fd59fb184818 100644
--- a/arch/um/include/asm/pgalloc.h
+++ b/arch/um/include/asm/pgalloc.h@@ -27,7 +27,6 @@ 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))); \ } while (0)
@@ -35,7 +34,6 @@ do { \ #define __pmd_free_tlb(tlb, pmd, address) \ do { \ - pagetable_dtor(virt_to_ptdesc(pmd)); \ tlb_remove_page_ptdesc((tlb), virt_to_ptdesc(pmd)); \ } while (0)
@@ -43,7 +41,6 @@ do { \ #define __pud_free_tlb(tlb, pud, address) \ do { \ - pagetable_dtor(virt_to_ptdesc(pud)); \ tlb_remove_page_ptdesc((tlb), virt_to_ptdesc(pud)); \ } while (0)
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index a96d4b440f3da..a59205863f431 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h@@ -506,6 +506,7 @@ static inline void tlb_remove_ptdesc(struct mmu_gather *tlb, void *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)
{
+ pagetable_dtor(pt);
tlb_remove_page(tlb, ptdesc_page(pt));
}
This avoids explicitly calling pagetable_dtor() in the architecture
code. If that makes sense, I can send a formal separate patch to do
this.
Thanks,
Qi
the extra handling from arch code? I may well be missing something, I'm not super familiar with the tlb handling code > - Kevin