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

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

On 2026/1/18 16:39, Lance Yang wrote:
> Hi Hugh,
> 
> Could you check if my understanding is correct?
> 
> On PAE, pmdp_get_lockless() reads pmd_low first, then pmd_high. There's a
> risk of reading mismatched values if another CPU modifies the PMD between
> the two reads.
> 
> Commit 146b42e07494[1] introduced local_irq_save() to protect the
> split-read, blocking TLB flush IPIs during the operation.
> 
> After modifying the PMD, pmdp_get_lockless_sync() sends an IPI to ensure
> all ongoing split-reads complete before proceeding with pte_free_defer().
> 
> As commit 146b42e07494[1] says:
> 
> ```
> Complement this pmdp_get_lockless_start() and pmdp_get_lockless_end(),
> used only locally in __pte_offset_map(), with a pmdp_get_lockless_sync()
> synonym for tlb_remove_table_sync_one(): to send the necessary interrupt
> at the right moment on those configs which do not already send it.
> ```
> 
> And commit 1043173eb5eb[2] says:
> 
> ```
> Follow the pattern in retract_page_tables(); and using pte_free_defer()
> removes most of the need for tlb_remove_table_sync_one() here; but call
> pmdp_get_lockless_sync() to use it in the PAE case.
> ```
> 
> Regarding moving pmdp_get_lockless_sync() out from under PTL: Since
> lockless readers (e.g., GUP-fast, __pte_offset_map()) are protected by
> local_irq_save() rather than PTL, pmdp_get_lockless_sync() can be called
> outside PTL as long as it's before pte_free_defer().


Looking at commit 146b42e07494[1] again, it says pmdp_get_lockless_sync()
should be called "at the right moment". I now realize moving it outside
PTL might not be safe.

If we release PTL before calling pmdp_get_lockless_sync(), another CPU
could set a new PMD while a lockless reader is still in local_irq_save()
reading the old PMD (split-read). I'm not sure if this race is actually
possible, but if it is, it would hit the ABA problem where the reader
gets mismatched pmd_low (old) and pmd_high (new) - the "faint risk"
mentioned in commit 146b42e07494[1].

On Native x86 PAE, pmdp_collapse_flush() sends IPI and waits, preventing
this race. But on PV, the hypercall returns immediately, so we need
pmdp_get_lockless_sync() to ensure all IRQ-off readers complete before
releasing PTL.

I should keep it under PTL to be safe.

Sorry for the churn :(

[1] 
https://github.com/torvalds/linux/commit/146b42e07494e45f7c7bcf2cbf7afd1424afd78e


Thanks,
Lance

> 
> In contrast, for non-PAE, PMD reads are atomic, so pmdp_get_lockless_sync()
> is a no-op.
> 
> [1] https://github.com/torvalds/linux/commit/146b42e07494e45f7c7bcf2cbf7afd1424afd78e
> [2] https://github.com/torvalds/linux/commit/1043173eb5eb351a1dba11cca12705075fe74a9e
> 
> 
> Thanks,
> Lance
> 
> On Fri, 16 Jan 2026 09:25:54 +0800, Lance Yang wrote:
>>
>>
>> On 2026/1/16 09:03, Baolin Wang wrote:
>>>
>>>
>>> On 1/15/26 8:28 PM, Lance Yang wrote:
>>>>
>>>>
>>>> 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.
>>>
>>> Cc Hugh who introduced the pmdp_get_lockless_sync(), to double check.
>>>
>>> Sounds reasonable to me, please add these comments into the commit
>>> message. Thanks.
>>
>> Yes, will do. Thanks!
>>
>>>
>>>>> 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.
>>>
>>> OK.
>>
>> Cheers,
>> Lance
>>