| Message ID | ad21b9392096336cf15aee46f68f9989a9cf877e.1735549103.git.zhengqi.arch@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | move pagetable_*_dtor() to __tlb_remove_table() | expand |
On Mon, Dec 30, 2024 at 05:07:47PM +0800, Qi Zheng wrote: > To unify the PxD and PTE TLB free path, also move the pagetable_dtor() of > PMD|PUD|P4D to __tlb_remove_table(). The above and Subject are still incorrect: pagetable_dtor() is called from pagetable_dtor_free(), not from __tlb_remove_table(). ... > diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c > index 569de24d33761..c73b89811a264 100644 > --- a/arch/s390/mm/pgalloc.c > +++ b/arch/s390/mm/pgalloc.c > @@ -180,7 +180,7 @@ unsigned long *page_table_alloc(struct mm_struct *mm) > return table; > } > > -static void pagetable_pte_dtor_free(struct ptdesc *ptdesc) > +static void pagetable_dtor_free(struct ptdesc *ptdesc) > { > pagetable_dtor(ptdesc); > pagetable_free(ptdesc); > @@ -190,20 +190,14 @@ void page_table_free(struct mm_struct *mm, unsigned long *table) > { > struct ptdesc *ptdesc = virt_to_ptdesc(table); > > - pagetable_pte_dtor_free(ptdesc); > + pagetable_dtor_free(ptdesc); > } > > void __tlb_remove_table(void *table) > { > struct ptdesc *ptdesc = virt_to_ptdesc(table); > - struct page *page = ptdesc_page(ptdesc); > > - if (compound_order(page) == CRST_ALLOC_ORDER) { > - /* pmd, pud, or p4d */ > - pagetable_free(ptdesc); > - return; > - } > - pagetable_pte_dtor_free(ptdesc); > + pagetable_dtor_free(ptdesc); > }
On 2025/1/6 18:36, Alexander Gordeev wrote: > On Mon, Dec 30, 2024 at 05:07:47PM +0800, Qi Zheng wrote: >> To unify the PxD and PTE TLB free path, also move the pagetable_dtor() of >> PMD|PUD|P4D to __tlb_remove_table(). > > The above and Subject are still incorrect: pagetable_dtor() is > called from pagetable_dtor_free(), not from __tlb_remove_table(). Hmm, __tlb_remove_table() calls pagetable_dtor_free(), so moving to pagetable_dtor_free() means moving to __tlb_remove_table(). Right? And the main purpose of this patch is also to move pagetable_dtor() to __tlb_remove_table(). So I think this description makes sense? > > ... >> diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c >> index 569de24d33761..c73b89811a264 100644 >> --- a/arch/s390/mm/pgalloc.c >> +++ b/arch/s390/mm/pgalloc.c >> @@ -180,7 +180,7 @@ unsigned long *page_table_alloc(struct mm_struct *mm) >> return table; >> } >> >> -static void pagetable_pte_dtor_free(struct ptdesc *ptdesc) >> +static void pagetable_dtor_free(struct ptdesc *ptdesc) >> { >> pagetable_dtor(ptdesc); >> pagetable_free(ptdesc); >> @@ -190,20 +190,14 @@ void page_table_free(struct mm_struct *mm, unsigned long *table) >> { >> struct ptdesc *ptdesc = virt_to_ptdesc(table); >> >> - pagetable_pte_dtor_free(ptdesc); >> + pagetable_dtor_free(ptdesc); >> } >> >> void __tlb_remove_table(void *table) >> { >> struct ptdesc *ptdesc = virt_to_ptdesc(table); >> - struct page *page = ptdesc_page(ptdesc); >> >> - if (compound_order(page) == CRST_ALLOC_ORDER) { >> - /* pmd, pud, or p4d */ >> - pagetable_free(ptdesc); >> - return; >> - } >> - pagetable_pte_dtor_free(ptdesc); >> + pagetable_dtor_free(ptdesc); >> }
On Mon, Jan 06, 2025 at 07:02:17PM +0800, Qi Zheng wrote: > > On Mon, Dec 30, 2024 at 05:07:47PM +0800, Qi Zheng wrote: > > > To unify the PxD and PTE TLB free path, also move the pagetable_dtor() of > > > PMD|PUD|P4D to __tlb_remove_table(). > > > > The above and Subject are still incorrect: pagetable_dtor() is > > called from pagetable_dtor_free(), not from __tlb_remove_table(). > > Hmm, __tlb_remove_table() calls pagetable_dtor_free(), so moving to > pagetable_dtor_free() means moving to __tlb_remove_table(). Right? Right. But you Subject and description claim "... also move the pagetable_dtor()" not to pagetable_dtor_free() - which is another function. > And the main purpose of this patch is also to move pagetable_dtor() > to __tlb_remove_table(). So I think this description makes sense? The patch makes sense, but the description it is incorrect ;) Thanks!
On 2025/1/6 20:44, Alexander Gordeev wrote: > On Mon, Jan 06, 2025 at 07:02:17PM +0800, Qi Zheng wrote: >>> On Mon, Dec 30, 2024 at 05:07:47PM +0800, Qi Zheng wrote: >>>> To unify the PxD and PTE TLB free path, also move the pagetable_dtor() of >>>> PMD|PUD|P4D to __tlb_remove_table(). >>> >>> The above and Subject are still incorrect: pagetable_dtor() is >>> called from pagetable_dtor_free(), not from __tlb_remove_table(). >> >> Hmm, __tlb_remove_table() calls pagetable_dtor_free(), so moving to >> pagetable_dtor_free() means moving to __tlb_remove_table(). Right? > > Right. But you Subject and description claim "... also move the > pagetable_dtor()" not to pagetable_dtor_free() - which is another > function. OK, will change the subject and description to: s390: pgtable: also move pagetable_dtor() of PxD to pagetable_dtor_free() To unify the PxD and PTE TLB free path, also move the pagetable_dtor() of PMD|PUD|P4D to pagetable_dtor_free(). But pagetable_dtor_free() is newly introduced in this patch, should it be changed to 'move ... to pagetable_pte_dtor_free()'? But this seems strange. :( > >> And the main purpose of this patch is also to move pagetable_dtor() >> to __tlb_remove_table(). So I think this description makes sense? > > The patch makes sense, but the description it is incorrect ;) > > Thanks! Thanks!
On Mon, Jan 06, 2025 at 09:34:55PM +0800, Qi Zheng wrote: > OK, will change the subject and description to: > > s390: pgtable: also move pagetable_dtor() of PxD to pagetable_dtor_free() > > To unify the PxD and PTE TLB free path, also move the pagetable_dtor() of > PMD|PUD|P4D to pagetable_dtor_free(). > > But pagetable_dtor_free() is newly introduced in this patch, should it > be changed to 'move ... to pagetable_pte_dtor_free()'? But this seems > strange. :( s390: pgtable: consolidate PxD and PTE TLB free paths Call pagetable_dtor() for PMD|PUD|P4D tables just before ptdesc is freed - same as it is done for PTE tables. That allows consolidating TLB free paths for all table types. Makes sense? > Thanks! Thank you!
On 2025/1/6 22:35, Alexander Gordeev wrote: > On Mon, Jan 06, 2025 at 09:34:55PM +0800, Qi Zheng wrote: >> OK, will change the subject and description to: >> >> s390: pgtable: also move pagetable_dtor() of PxD to pagetable_dtor_free() >> >> To unify the PxD and PTE TLB free path, also move the pagetable_dtor() of >> PMD|PUD|P4D to pagetable_dtor_free(). >> >> But pagetable_dtor_free() is newly introduced in this patch, should it >> be changed to 'move ... to pagetable_pte_dtor_free()'? But this seems >> strange. :( > > s390: pgtable: consolidate PxD and PTE TLB free paths > > Call pagetable_dtor() for PMD|PUD|P4D tables just before ptdesc is > freed - same as it is done for PTE tables. That allows consolidating > TLB free paths for all table types. > > Makes sense? Ah, make sense. Many thanks to you! Will do it in v5. > >> Thanks! > > Thank you!
diff --git a/arch/s390/include/asm/tlb.h b/arch/s390/include/asm/tlb.h index 74b6fba4c2ee3..79df7c0932c56 100644 --- a/arch/s390/include/asm/tlb.h +++ b/arch/s390/include/asm/tlb.h @@ -102,7 +102,6 @@ static inline void pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd, { if (mm_pmd_folded(tlb->mm)) return; - pagetable_dtor(virt_to_ptdesc(pmd)); __tlb_adjust_range(tlb, address, PAGE_SIZE); tlb->mm->context.flush_mm = 1; tlb->freed_tables = 1; @@ -122,7 +121,6 @@ static inline void pud_free_tlb(struct mmu_gather *tlb, pud_t *pud, { if (mm_pud_folded(tlb->mm)) return; - pagetable_dtor(virt_to_ptdesc(pud)); tlb->mm->context.flush_mm = 1; tlb->freed_tables = 1; tlb->cleared_p4ds = 1; @@ -141,7 +139,6 @@ static inline void p4d_free_tlb(struct mmu_gather *tlb, p4d_t *p4d, { if (mm_p4d_folded(tlb->mm)) return; - pagetable_dtor(virt_to_ptdesc(p4d)); __tlb_adjust_range(tlb, address, PAGE_SIZE); tlb->mm->context.flush_mm = 1; tlb->freed_tables = 1; diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c index 569de24d33761..c73b89811a264 100644 --- a/arch/s390/mm/pgalloc.c +++ b/arch/s390/mm/pgalloc.c @@ -180,7 +180,7 @@ unsigned long *page_table_alloc(struct mm_struct *mm) return table; } -static void pagetable_pte_dtor_free(struct ptdesc *ptdesc) +static void pagetable_dtor_free(struct ptdesc *ptdesc) { pagetable_dtor(ptdesc); pagetable_free(ptdesc); @@ -190,20 +190,14 @@ void page_table_free(struct mm_struct *mm, unsigned long *table) { struct ptdesc *ptdesc = virt_to_ptdesc(table); - pagetable_pte_dtor_free(ptdesc); + pagetable_dtor_free(ptdesc); } void __tlb_remove_table(void *table) { struct ptdesc *ptdesc = virt_to_ptdesc(table); - struct page *page = ptdesc_page(ptdesc); - if (compound_order(page) == CRST_ALLOC_ORDER) { - /* pmd, pud, or p4d */ - pagetable_free(ptdesc); - return; - } - pagetable_pte_dtor_free(ptdesc); + pagetable_dtor_free(ptdesc); } #ifdef CONFIG_TRANSPARENT_HUGEPAGE @@ -211,7 +205,7 @@ static void pte_free_now(struct rcu_head *head) { struct ptdesc *ptdesc = container_of(head, struct ptdesc, pt_rcu_head); - pagetable_pte_dtor_free(ptdesc); + pagetable_dtor_free(ptdesc); } void pte_free_defer(struct mm_struct *mm, pgtable_t pgtable)
To unify the PxD and PTE TLB free path, also move the pagetable_dtor() of PMD|PUD|P4D to __tlb_remove_table(). Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: linux-s390@vger.kernel.org --- arch/s390/include/asm/tlb.h | 3 --- arch/s390/mm/pgalloc.c | 14 ++++---------- 2 files changed, 4 insertions(+), 13 deletions(-)