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

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

On 2024/12/16 20:05, Peter Zijlstra wrote:
> On Mon, Dec 16, 2024 at 01:03:13PM +0100, Peter Zijlstra wrote:
>> On Mon, Dec 16, 2024 at 01:00:43PM +0100, Peter Zijlstra wrote:
>>> On Sat, Dec 14, 2024 at 05:02:57PM +0800, Qi Zheng wrote:
>>>
>>>> diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
>>>> index c73b89811a264..3e002dea6278f 100644
>>>> --- a/arch/s390/mm/pgalloc.c
>>>> +++ b/arch/s390/mm/pgalloc.c
>>>> @@ -193,13 +193,6 @@ void page_table_free(struct mm_struct *mm, unsigned long *table)
>>>>   	pagetable_dtor_free(ptdesc);
>>>>   }
>>>>   
>>>> -void __tlb_remove_table(void *table)
>>>> -{
>>>> -	struct ptdesc *ptdesc = virt_to_ptdesc(table);
>>>> -
>>>> -	pagetable_dtor_free(ptdesc);
>>>> -}
>>>> -
>>>>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>>   static void pte_free_now(struct rcu_head *head)
>>>>   {
>>>
>>>> diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
>>>> index 709830274b756..939a813023d7e 100644
>>>> --- a/include/asm-generic/tlb.h
>>>> +++ b/include/asm-generic/tlb.h
>>>
>>>>   #define MAX_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)
>>>> +{
>>>> +	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.
>>>
>>> ---
>>
>>> -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);
>>
>> And now observe that __tlb_remove_table() is identical to
>> asm-generic/pgalloc.h pte_free(), pmd_free(), __pud_free() and
>> __p4d_free().
>>
>> And I'm sure we don't need 5 copies of this :-), wdyt?
> 
> Argh, nearly, it has the whole page vs virt nonsense still going on.
> Bah.

Yes, maybe it can be unified into struct page parameter, which seems
to be more convenient for conversion:

static inline void pagtable_dtor_free(struct mm_struct *mm, void *table)
{
	struct ptdesc *ptdesc = page_to_ptdesc(table);

	pagetable_dtor(ptdesc);
	pagetable_free(ptdesc);
}