Re: [PATCH v1 1/1] mm/khugepaged: move tlb_remove_table_sync_one out from under PTL

[Lance Yang <lance.yang@linux.dev>]

On 2026/1/15 18:00, Baolin Wang wrote:
> Hi Lance,
> 
> On 1/15/26 3:16 PM, Lance Yang wrote:
>> From: Lance Yang <lance.yang@linux.dev>
>>
>> tlb_remove_table_sync_one() sends IPIs to all CPUs and waits for them,
>> which we really don't want to do while holding PTL.
> 
> Could you add more comments to explain why this is safe for the PAE case?

Yep, IIUC, it is safe because we've already done pmdp_collapse_flush()
which ensures the PMD change is visible.

pmdp_get_lockless_sync() (which calls tlb_remove_table_sync_one() on PAE)
is just to ensure any ongoing lockless pmd readers (e.g., GUP-fast) complete
before we proceed. It sends IPIs to all CPUs and waits for responses - a CPU
can only respond when it's not between local_irq_save() and 
local_irq_restore().

Moving it out from under PTL doesn't change the synchronization semantics,
since lockless readers don't depend on PTL anyway.

> 
> For the non-PAE case, you added a new tlb_remove_table_sync_one(), why 
> we need this (to solve what problem)? Please also add more comments to 
> explain.

Oops, you're right, the original macro was a no-op for non-PAE.

I should just move the macro call out from under PTL, rather than
replacing it with direct tlb_remove_table_sync_one() calls.


Thanks,
Lance

> 
>> Just move the call to after we release PTL, and drop the macro wrapper
>> while we're at it.
>>
>> Signed-off-by: Lance Yang <lance.yang@linux.dev>
>> ---
>>   include/linux/pgtable.h | 4 ----
>>   mm/khugepaged.c         | 5 +++--
>>   2 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
>> index eb8aacba3698..fb04ed22052c 100644
>> --- a/include/linux/pgtable.h
>> +++ b/include/linux/pgtable.h
>> @@ -755,7 +755,6 @@ static inline pmd_t pmdp_get_lockless(pmd_t *pmdp)
>>       return pmd;
>>   }
>>   #define pmdp_get_lockless pmdp_get_lockless
>> -#define pmdp_get_lockless_sync() tlb_remove_table_sync_one()
>>   #endif /* CONFIG_PGTABLE_LEVELS > 2 */
>>   #endif /* CONFIG_GUP_GET_PXX_LOW_HIGH */
>> @@ -774,9 +773,6 @@ static inline pmd_t pmdp_get_lockless(pmd_t *pmdp)
>>   {
>>       return pmdp_get(pmdp);
>>   }
>> -static inline void pmdp_get_lockless_sync(void)
>> -{
>> -}
>>   #endif
>>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 9f790ec34400..0a6cebf880e0 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -1664,10 +1664,10 @@ static enum scan_result 
>> try_collapse_pte_mapped_thp(struct mm_struct *mm, unsign
>>           }
>>       }
>>       pgt_pmd = pmdp_collapse_flush(vma, haddr, pmd);
>> -    pmdp_get_lockless_sync();
>>       pte_unmap_unlock(start_pte, ptl);
>>       if (ptl != pml)
>>           spin_unlock(pml);
>> +    tlb_remove_table_sync_one();
>>       mmu_notifier_invalidate_range_end(&range);
>> @@ -1818,7 +1818,6 @@ static void retract_page_tables(struct 
>> address_space *mapping, pgoff_t pgoff)
>>            */
>>           if (likely(file_backed_vma_is_retractable(vma))) {
>>               pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
>> -            pmdp_get_lockless_sync();
>>               success = true;
>>           }
>> @@ -1826,6 +1825,8 @@ static void retract_page_tables(struct 
>> address_space *mapping, pgoff_t pgoff)
>>               spin_unlock(ptl);
>>   drop_pml:
>>           spin_unlock(pml);
>> +        if (success)
>> +            tlb_remove_table_sync_one();
>>           mmu_notifier_invalidate_range_end(&range);
>