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

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

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().

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
>