| Message ID | 20260922-rcu-pagetable-freeing-v4-1-fe1ad1f1e303@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | mm: make userland page table freeing RCU-safe | expand |
On Tue, Sep 22, 2026 at 04:35:32PM +0100, Lorenzo Stoakes (ARM) wrote: >collapse_huge_page() deposits a PTE page table on PMD collapse in order >that it can be utilised for subsequent split operations, meaning that those >operations do not need to perform an allocation (as they are in a context >where it might be unwise). > >However the PTE page table which is deposited is the one which is currently >mapped by the PMD entry that is in the process of being collapsed. > >Once deposited, the PTE page table may be used in a split of any other >unrelated PMD entry. > >This is currently not an issue as this operation is performed with VMA/mmap >write lock + anon rmap locks held, so ordinary page table walkers will >never accidentally end up walking the wrong thing, and GUP-fast is >protected by an IPI via tlb_remove_table_sync_one(). > >However, the series to which this commit belongs implements RCU-safe page >table traversal, at which point this becomes problematic. > >This can be resolved by using pte_offset_map_lock() which gates on a PTE >PTL and a pmd_same() check, but lockless walks are unsafe as things stand. > >Resolve this by simply allocating a new, zeroed, PTE page table to deposit >at the point of collapse. > >This path is already costly and an allocation has already been performed >for the huge folio, so this allocation is statistical noise in terms of >performance and memory usage at this point. > >With this PTE page table deposited, RCU-free the existing PTE page table >so it is safe for page table walkers to traverse within a grace period. > >This also brings this deposit case in line with all other page table >deposit logic which deposit a fresh page table. > >Additionally, this was the only place in the kernel that displaced a page >table like this, so eliminating it also helps consistency. > >An edge case for deposit exists for powerpc and its hash-based MMU - it >stores hash slot data in deposited page tables and zeroes them on withdraw, >so a zeroed deposited page table works correctly for it. > >Since khugepaged runs as a kernel thread, do a little dance in >alloc_deposit_pte() to correctly charge the allocation. > >This is already done for the folio allocation via alloc_charge_folio() but >no such wrapper exists for a page table allocation. > >Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org> >--- > mm/khugepaged.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > >diff --git a/mm/khugepaged.c b/mm/khugepaged.c >index f49a6710933b..dab421f8233e 100644 >--- a/mm/khugepaged.c >+++ b/mm/khugepaged.c >@@ -1278,6 +1278,23 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru > return SCAN_SUCCEED; > } > >+static pgtable_t alloc_deposit_pte(struct mm_struct *mm) >+{ >+ /* >+ * khugepaged is run from a kernel thread, so need to manually set the >+ * correct memcg so the allocation gets charged correctly. >+ */ >+ struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm); >+ struct mem_cgroup *old_memcg = set_active_memcg(memcg); >+ pgtable_t pgtable; >+ >+ pgtable = pte_alloc_one(mm); >+ >+ set_active_memcg(old_memcg); >+ mem_cgroup_put(memcg); >+ return pgtable; >+} >+ > /* > * collapse_huge_page() expects the mmap_lock to be unlocked before entering and > * will always return with the lock unlocked, to avoid holding the mmap_lock >@@ -1293,7 +1310,7 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > LIST_HEAD(compound_pagelist); > pmd_t *pmd, _pmd; > pte_t *pte = NULL; >- pgtable_t pgtable; >+ pgtable_t pgtable = NULL; > struct folio *folio; > spinlock_t *pmd_ptl, *pte_ptl; > enum scan_result result = SCAN_FAIL; >@@ -1310,6 +1327,12 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > goto out_nolock; > } > >+ if (is_pmd_order(order)) { >+ pgtable = alloc_deposit_pte(mm); >+ if (!pgtable) >+ goto out_nolock; One small thing: result is still SCAN_SUCCEED after alloc_charge_folio(), result = alloc_charge_folio(&folio, mm, cc, order); if (result != SCAN_SUCCEED) goto out_nolock; if (folio_memcg_alloc_deferred(folio)) { result = SCAN_ALLOC_HUGE_PAGE_FAIL; goto out_nolock; } if (is_pmd_order(order)) { pgtable = alloc_deposit_pte(mm); if (!pgtable) goto out_nolock; } so if alloc_deposit_pte() fails, collapse_huge_page() returns success without installing a PMD. I see sashiko pointed that out too :) Cheers, Lance
On Wed, Sep 23, 2026 at 10:24:36AM +0800, Lance Yang wrote: > > mm/khugepaged.c | 32 ++++++++++++++++++++++++++++++-- > > 1 file changed, 30 insertions(+), 2 deletions(-) > > > >diff --git a/mm/khugepaged.c b/mm/khugepaged.c > >index f49a6710933b..dab421f8233e 100644 > >--- a/mm/khugepaged.c > >+++ b/mm/khugepaged.c > >@@ -1278,6 +1278,23 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru > > return SCAN_SUCCEED; > > } > > > >+static pgtable_t alloc_deposit_pte(struct mm_struct *mm) > >+{ > >+ /* > >+ * khugepaged is run from a kernel thread, so need to manually set the > >+ * correct memcg so the allocation gets charged correctly. > >+ */ > >+ struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm); > >+ struct mem_cgroup *old_memcg = set_active_memcg(memcg); > >+ pgtable_t pgtable; > >+ > >+ pgtable = pte_alloc_one(mm); > >+ > >+ set_active_memcg(old_memcg); > >+ mem_cgroup_put(memcg); > >+ return pgtable; > >+} > >+ > > /* > > * collapse_huge_page() expects the mmap_lock to be unlocked before entering and > > * will always return with the lock unlocked, to avoid holding the mmap_lock > >@@ -1293,7 +1310,7 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > > LIST_HEAD(compound_pagelist); > > pmd_t *pmd, _pmd; > > pte_t *pte = NULL; > >- pgtable_t pgtable; > >+ pgtable_t pgtable = NULL; > > struct folio *folio; > > spinlock_t *pmd_ptl, *pte_ptl; > > enum scan_result result = SCAN_FAIL; > >@@ -1310,6 +1327,12 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > > goto out_nolock; > > } > > > >+ if (is_pmd_order(order)) { > >+ pgtable = alloc_deposit_pte(mm); > >+ if (!pgtable) > >+ goto out_nolock; > > One small thing: result is still SCAN_SUCCEED after alloc_charge_folio(), > > result = alloc_charge_folio(&folio, mm, cc, order); > if (result != SCAN_SUCCEED) > goto out_nolock; > > if (folio_memcg_alloc_deferred(folio)) { > result = SCAN_ALLOC_HUGE_PAGE_FAIL; > goto out_nolock; > } > > if (is_pmd_order(order)) { > pgtable = alloc_deposit_pte(mm); > if (!pgtable) > goto out_nolock; > } > > so if alloc_deposit_pte() fails, collapse_huge_page() returns success > without installing a PMD. > > I see sashiko pointed that out too :) > > Cheers, Lance Sigh ack yeah oops :) I think SCAN_ALLOC_HUGE_PAGE_FAIL is the closest though it's not allocating a huge page so much as the page table, but gets the correct MADV_COLLAPSE -ENOMEM back. (Another case of local AI review not catchign something btw! Sashiko does lean towards over-reporting/hallucinating but catches more) Will ask Andrew to update in reply to this patch as a quick thing rather than respin so quick, and fix up on any respin. -- Cheers, Lorenzo
Andrew - would it be possible to make one quick fix up below? On Tue, Sep 22, 2026 at 04:35:32PM +0100, Lorenzo Stoakes (ARM) wrote: > collapse_huge_page() deposits a PTE page table on PMD collapse in order > that it can be utilised for subsequent split operations, meaning that those > operations do not need to perform an allocation (as they are in a context > where it might be unwise). > > However the PTE page table which is deposited is the one which is currently > mapped by the PMD entry that is in the process of being collapsed. > > Once deposited, the PTE page table may be used in a split of any other > unrelated PMD entry. > > This is currently not an issue as this operation is performed with VMA/mmap > write lock + anon rmap locks held, so ordinary page table walkers will > never accidentally end up walking the wrong thing, and GUP-fast is > protected by an IPI via tlb_remove_table_sync_one(). > > However, the series to which this commit belongs implements RCU-safe page > table traversal, at which point this becomes problematic. > > This can be resolved by using pte_offset_map_lock() which gates on a PTE > PTL and a pmd_same() check, but lockless walks are unsafe as things stand. > > Resolve this by simply allocating a new, zeroed, PTE page table to deposit > at the point of collapse. > > This path is already costly and an allocation has already been performed > for the huge folio, so this allocation is statistical noise in terms of > performance and memory usage at this point. > > With this PTE page table deposited, RCU-free the existing PTE page table > so it is safe for page table walkers to traverse within a grace period. > > This also brings this deposit case in line with all other page table > deposit logic which deposit a fresh page table. > > Additionally, this was the only place in the kernel that displaced a page > table like this, so eliminating it also helps consistency. > > An edge case for deposit exists for powerpc and its hash-based MMU - it > stores hash slot data in deposited page tables and zeroes them on withdraw, > so a zeroed deposited page table works correctly for it. > > Since khugepaged runs as a kernel thread, do a little dance in > alloc_deposit_pte() to correctly charge the allocation. > > This is already done for the folio allocation via alloc_charge_folio() but > no such wrapper exists for a page table allocation. > > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org> > --- > mm/khugepaged.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index f49a6710933b..dab421f8233e 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -1278,6 +1278,23 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru > return SCAN_SUCCEED; > } > > +static pgtable_t alloc_deposit_pte(struct mm_struct *mm) > +{ > + /* > + * khugepaged is run from a kernel thread, so need to manually set the > + * correct memcg so the allocation gets charged correctly. > + */ > + struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm); > + struct mem_cgroup *old_memcg = set_active_memcg(memcg); > + pgtable_t pgtable; > + > + pgtable = pte_alloc_one(mm); > + > + set_active_memcg(old_memcg); > + mem_cgroup_put(memcg); > + return pgtable; > +} > + > /* > * collapse_huge_page() expects the mmap_lock to be unlocked before entering and > * will always return with the lock unlocked, to avoid holding the mmap_lock > @@ -1293,7 +1310,7 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > LIST_HEAD(compound_pagelist); > pmd_t *pmd, _pmd; > pte_t *pte = NULL; > - pgtable_t pgtable; > + pgtable_t pgtable = NULL; > struct folio *folio; > spinlock_t *pmd_ptl, *pte_ptl; > enum scan_result result = SCAN_FAIL; > @@ -1310,6 +1327,12 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > goto out_nolock; > } > > + if (is_pmd_order(order)) { > + pgtable = alloc_deposit_pte(mm); > + if (!pgtable) > + goto out_nolock; Fixup is here: - if (!pgtable) - goto out_nolock; + if (!pgtable) { + result = SCAN_ALLOC_HUGE_PAGE_FAIL; + goto out_nolock; + } Thanks! > + } > + > mmap_read_lock(mm); > result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true, > &vma, cc, order); > @@ -1433,8 +1456,8 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > spin_lock(pmd_ptl); > VM_WARN_ON_ONCE(!pmd_none(*pmd)); > if (is_pmd_order(order)) { > - pgtable = pmd_pgtable(_pmd); > pgtable_trans_huge_deposit(mm, pmd, pgtable); > + pgtable = NULL; > map_anon_folio_pmd_nopf(folio, pmd, vma, pmd_addr); > } else { > /* > @@ -1453,6 +1476,9 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > } > spin_unlock(pmd_ptl); > > + if (is_pmd_order(order)) > + pte_free_defer(mm, pmd_pgtable(_pmd)); > + > folio = NULL; > > result = SCAN_SUCCEED; > @@ -1463,6 +1489,8 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s > anon_vma_unlock_write(vma->anon_vma); > mmap_write_unlock(mm); > out_nolock: > + if (pgtable) > + pte_free(mm, pgtable); > if (folio) > folio_put(folio); > trace_mm_collapse_huge_page(mm, result == SCAN_SUCCEED, result, order); > > -- > 2.55.0 > -- Cheers, Lorenzo
On Wed, Sep 23, 2026 at 08:14:01AM +0100, Lorenzo Stoakes (ARM) wrote: >Andrew - would it be possible to make one quick fix up below? > >On Tue, Sep 22, 2026 at 04:35:32PM +0100, Lorenzo Stoakes (ARM) wrote: >> collapse_huge_page() deposits a PTE page table on PMD collapse in order >> that it can be utilised for subsequent split operations, meaning that those >> operations do not need to perform an allocation (as they are in a context >> where it might be unwise). >> >> However the PTE page table which is deposited is the one which is currently >> mapped by the PMD entry that is in the process of being collapsed. >> >> Once deposited, the PTE page table may be used in a split of any other >> unrelated PMD entry. >> >> This is currently not an issue as this operation is performed with VMA/mmap >> write lock + anon rmap locks held, so ordinary page table walkers will >> never accidentally end up walking the wrong thing, and GUP-fast is >> protected by an IPI via tlb_remove_table_sync_one(). >> >> However, the series to which this commit belongs implements RCU-safe page >> table traversal, at which point this becomes problematic. >> >> This can be resolved by using pte_offset_map_lock() which gates on a PTE >> PTL and a pmd_same() check, but lockless walks are unsafe as things stand. >> >> Resolve this by simply allocating a new, zeroed, PTE page table to deposit >> at the point of collapse. >> >> This path is already costly and an allocation has already been performed >> for the huge folio, so this allocation is statistical noise in terms of >> performance and memory usage at this point. >> >> With this PTE page table deposited, RCU-free the existing PTE page table >> so it is safe for page table walkers to traverse within a grace period. >> >> This also brings this deposit case in line with all other page table >> deposit logic which deposit a fresh page table. >> >> Additionally, this was the only place in the kernel that displaced a page >> table like this, so eliminating it also helps consistency. >> >> An edge case for deposit exists for powerpc and its hash-based MMU - it >> stores hash slot data in deposited page tables and zeroes them on withdraw, >> so a zeroed deposited page table works correctly for it. >> >> Since khugepaged runs as a kernel thread, do a little dance in >> alloc_deposit_pte() to correctly charge the allocation. >> >> This is already done for the folio allocation via alloc_charge_folio() but >> no such wrapper exists for a page table allocation. >> >> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org> >> --- >> mm/khugepaged.c | 32 ++++++++++++++++++++++++++++++-- >> 1 file changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c >> index f49a6710933b..dab421f8233e 100644 >> --- a/mm/khugepaged.c >> +++ b/mm/khugepaged.c >> @@ -1278,6 +1278,23 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru >> return SCAN_SUCCEED; >> } >> >> +static pgtable_t alloc_deposit_pte(struct mm_struct *mm) >> +{ >> + /* >> + * khugepaged is run from a kernel thread, so need to manually set the >> + * correct memcg so the allocation gets charged correctly. >> + */ >> + struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm); >> + struct mem_cgroup *old_memcg = set_active_memcg(memcg); >> + pgtable_t pgtable; >> + >> + pgtable = pte_alloc_one(mm); >> + >> + set_active_memcg(old_memcg); >> + mem_cgroup_put(memcg); >> + return pgtable; >> +} >> + >> /* >> * collapse_huge_page() expects the mmap_lock to be unlocked before entering and >> * will always return with the lock unlocked, to avoid holding the mmap_lock >> @@ -1293,7 +1310,7 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s >> LIST_HEAD(compound_pagelist); >> pmd_t *pmd, _pmd; >> pte_t *pte = NULL; >> - pgtable_t pgtable; >> + pgtable_t pgtable = NULL; >> struct folio *folio; >> spinlock_t *pmd_ptl, *pte_ptl; >> enum scan_result result = SCAN_FAIL; >> @@ -1310,6 +1327,12 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s >> goto out_nolock; >> } >> >> + if (is_pmd_order(order)) { >> + pgtable = alloc_deposit_pte(mm); >> + if (!pgtable) >> + goto out_nolock; > >Fixup is here: > >- if (!pgtable) >- goto out_nolock; >+ if (!pgtable) { >+ result = SCAN_ALLOC_HUGE_PAGE_FAIL; >+ goto out_nolock; >+ } > >Thanks! With that applied, Reviewed-by: Lance Yang <lance.yang@linux.dev>
On 9/22/26 17:35, Lorenzo Stoakes (ARM) wrote: > collapse_huge_page() deposits a PTE page table on PMD collapse in order > that it can be utilised for subsequent split operations, meaning that those > operations do not need to perform an allocation (as they are in a context > where it might be unwise). > > However the PTE page table which is deposited is the one which is currently > mapped by the PMD entry that is in the process of being collapsed. > > Once deposited, the PTE page table may be used in a split of any other > unrelated PMD entry. > > This is currently not an issue as this operation is performed with VMA/mmap > write lock + anon rmap locks held, so ordinary page table walkers will > never accidentally end up walking the wrong thing, and GUP-fast is > protected by an IPI via tlb_remove_table_sync_one(). > > However, the series to which this commit belongs implements RCU-safe page > table traversal, at which point this becomes problematic. > > This can be resolved by using pte_offset_map_lock() which gates on a PTE > PTL and a pmd_same() check, but lockless walks are unsafe as things stand. > > Resolve this by simply allocating a new, zeroed, PTE page table to deposit > at the point of collapse. > > This path is already costly and an allocation has already been performed > for the huge folio, so this allocation is statistical noise in terms of > performance and memory usage at this point. > > With this PTE page table deposited, RCU-free the existing PTE page table > so it is safe for page table walkers to traverse within a grace period. > > This also brings this deposit case in line with all other page table > deposit logic which deposit a fresh page table. > > Additionally, this was the only place in the kernel that displaced a page > table like this, so eliminating it also helps consistency. ack > > An edge case for deposit exists for powerpc and its hash-based MMU - it > stores hash slot data in deposited page tables and zeroes them on withdraw, > so a zeroed deposited page table works correctly for it. I think we would have zeroed out all page table entries either way already right now before depositing. See clear_ptes() in __collapse_huge_page_copy_succeeded(). So this paragraph might just be dropped. > > Since khugepaged runs as a kernel thread, do a little dance in > alloc_deposit_pte() to correctly charge the allocation. > > This is already done for the folio allocation via alloc_charge_folio() but > no such wrapper exists for a page table allocation. > > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org> > --- > mm/khugepaged.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index f49a6710933b..dab421f8233e 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -1278,6 +1278,23 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru > return SCAN_SUCCEED; > } > > +static pgtable_t alloc_deposit_pte(struct mm_struct *mm) alloc_deposit_pte_table or alloc_deposit_pgtable should be clearer. > +{ > + /* > + * khugepaged is run from a kernel thread, so need to manually set the > + * correct memcg so the allocation gets charged correctly. > + */ > + struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm); > + struct mem_cgroup *old_memcg = set_active_memcg(memcg); > + pgtable_t pgtable; > + > + pgtable = pte_alloc_one(mm); > +> + set_active_memcg(old_memcg); I'm no memcg expert but that looks good to me. > + mem_cgroup_put(memcg); > + return pgtable; > +} > + With the fixup Acked-by: David Hildenbrand (Arm) <david@kernel.org>
On Wed, Sep 23, 2026 at 10:54:07AM +0200, David Hildenbrand (Arm) wrote: > On 9/22/26 17:35, Lorenzo Stoakes (ARM) wrote: > > collapse_huge_page() deposits a PTE page table on PMD collapse in order > > that it can be utilised for subsequent split operations, meaning that those > > operations do not need to perform an allocation (as they are in a context > > where it might be unwise). > > > > However the PTE page table which is deposited is the one which is currently > > mapped by the PMD entry that is in the process of being collapsed. > > > > Once deposited, the PTE page table may be used in a split of any other > > unrelated PMD entry. > > > > This is currently not an issue as this operation is performed with VMA/mmap > > write lock + anon rmap locks held, so ordinary page table walkers will > > never accidentally end up walking the wrong thing, and GUP-fast is > > protected by an IPI via tlb_remove_table_sync_one(). > > > > However, the series to which this commit belongs implements RCU-safe page > > table traversal, at which point this becomes problematic. > > > > This can be resolved by using pte_offset_map_lock() which gates on a PTE > > PTL and a pmd_same() check, but lockless walks are unsafe as things stand. > > > > Resolve this by simply allocating a new, zeroed, PTE page table to deposit > > at the point of collapse. > > > > This path is already costly and an allocation has already been performed > > for the huge folio, so this allocation is statistical noise in terms of > > performance and memory usage at this point. > > > > With this PTE page table deposited, RCU-free the existing PTE page table > > so it is safe for page table walkers to traverse within a grace period. > > > > This also brings this deposit case in line with all other page table > > deposit logic which deposit a fresh page table. > > > > Additionally, this was the only place in the kernel that displaced a page > > table like this, so eliminating it also helps consistency. > > ack > > > > > An edge case for deposit exists for powerpc and its hash-based MMU - it > > stores hash slot data in deposited page tables and zeroes them on withdraw, > > so a zeroed deposited page table works correctly for it. > > I think we would have zeroed out all page table entries either way already right > now before depositing. See clear_ptes() in > __collapse_huge_page_copy_succeeded(). So this paragraph might just be dropped. Yup I know, but it was there just in case people wondered about it. But sure will drop. > > > > > Since khugepaged runs as a kernel thread, do a little dance in > > alloc_deposit_pte() to correctly charge the allocation. > > > > This is already done for the folio allocation via alloc_charge_folio() but > > no such wrapper exists for a page table allocation. > > > > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org> > > --- > > mm/khugepaged.c | 32 ++++++++++++++++++++++++++++++-- > > 1 file changed, 30 insertions(+), 2 deletions(-) > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > index f49a6710933b..dab421f8233e 100644 > > --- a/mm/khugepaged.c > > +++ b/mm/khugepaged.c > > @@ -1278,6 +1278,23 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru > > return SCAN_SUCCEED; > > } > > > > +static pgtable_t alloc_deposit_pte(struct mm_struct *mm) > > alloc_deposit_pte_table or alloc_deposit_pgtable > > should be clearer. The inevitable naming nit :) Yup agreed, I have tried to be consistent in saying 'pte page table' in commit msg etc. to differentiate an entry so that's fair, will fix up. > > > +{ > > + /* > > + * khugepaged is run from a kernel thread, so need to manually set the > > + * correct memcg so the allocation gets charged correctly. > > + */ > > + struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm); > > + struct mem_cgroup *old_memcg = set_active_memcg(memcg); > > + pgtable_t pgtable; > > + > > + pgtable = pte_alloc_one(mm); > > +> + set_active_memcg(old_memcg); > > I'm no memcg expert but that looks good to me. Yeah, it mirrors the pattern elsewhere. > > > + mem_cgroup_put(memcg); > > + return pgtable; > > +} > > + > > With the fixup > > Acked-by: David Hildenbrand (Arm) <david@kernel.org> Thanks! > > -- > Cheers, > > David -- Cheers, Lorenzo
On Wed, 23 Sep 2026 08:14:01 +0100 "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote: > Andrew - would it be possible to make one quick fix up below? > > Fixup is here: > > - if (!pgtable) > - goto out_nolock; > + if (!pgtable) { > + result = SCAN_ALLOC_HUGE_PAGE_FAIL; > + goto out_nolock; > + } --- a/mm/khugepaged.c~mm-khugepaged-deposit-a-newly-allocated-page-table-on-collapse-fix +++ a/mm/khugepaged.c @@ -1329,8 +1329,10 @@ static enum scan_result collapse_huge_pa if (is_pmd_order(order)) { pgtable = alloc_deposit_pte(mm); - if (!pgtable) + if (!pgtable) { + result = SCAN_ALLOC_HUGE_PAGE_FAIL; goto out_nolock; + } } mmap_read_lock(mm);
On 9/23/26 3:14 PM, Lorenzo Stoakes (ARM) wrote: > Andrew - would it be possible to make one quick fix up below? > > On Tue, Sep 22, 2026 at 04:35:32PM +0100, Lorenzo Stoakes (ARM) wrote: >> collapse_huge_page() deposits a PTE page table on PMD collapse in order >> that it can be utilised for subsequent split operations, meaning that those >> operations do not need to perform an allocation (as they are in a context >> where it might be unwise). >> >> However the PTE page table which is deposited is the one which is currently >> mapped by the PMD entry that is in the process of being collapsed. >> >> Once deposited, the PTE page table may be used in a split of any other >> unrelated PMD entry. >> >> This is currently not an issue as this operation is performed with VMA/mmap >> write lock + anon rmap locks held, so ordinary page table walkers will >> never accidentally end up walking the wrong thing, and GUP-fast is >> protected by an IPI via tlb_remove_table_sync_one(). >> >> However, the series to which this commit belongs implements RCU-safe page >> table traversal, at which point this becomes problematic. >> >> This can be resolved by using pte_offset_map_lock() which gates on a PTE >> PTL and a pmd_same() check, but lockless walks are unsafe as things stand. >> >> Resolve this by simply allocating a new, zeroed, PTE page table to deposit >> at the point of collapse. >> >> This path is already costly and an allocation has already been performed >> for the huge folio, so this allocation is statistical noise in terms of >> performance and memory usage at this point. >> >> With this PTE page table deposited, RCU-free the existing PTE page table >> so it is safe for page table walkers to traverse within a grace period. >> >> This also brings this deposit case in line with all other page table >> deposit logic which deposit a fresh page table. >> >> Additionally, this was the only place in the kernel that displaced a page >> table like this, so eliminating it also helps consistency. >> >> An edge case for deposit exists for powerpc and its hash-based MMU - it >> stores hash slot data in deposited page tables and zeroes them on withdraw, >> so a zeroed deposited page table works correctly for it. >> >> Since khugepaged runs as a kernel thread, do a little dance in >> alloc_deposit_pte() to correctly charge the allocation. >> >> This is already done for the folio allocation via alloc_charge_folio() but >> no such wrapper exists for a page table allocation. >> >> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org> >> --- >> mm/khugepaged.c | 32 ++++++++++++++++++++++++++++++-- >> 1 file changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c >> index f49a6710933b..dab421f8233e 100644 >> --- a/mm/khugepaged.c >> +++ b/mm/khugepaged.c >> @@ -1278,6 +1278,23 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru >> return SCAN_SUCCEED; >> } >> >> +static pgtable_t alloc_deposit_pte(struct mm_struct *mm) >> +{ >> + /* >> + * khugepaged is run from a kernel thread, so need to manually set the >> + * correct memcg so the allocation gets charged correctly. >> + */ >> + struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm); >> + struct mem_cgroup *old_memcg = set_active_memcg(memcg); >> + pgtable_t pgtable; >> + >> + pgtable = pte_alloc_one(mm); >> + >> + set_active_memcg(old_memcg); >> + mem_cgroup_put(memcg); >> + return pgtable; >> +} >> + >> /* >> * collapse_huge_page() expects the mmap_lock to be unlocked before entering and >> * will always return with the lock unlocked, to avoid holding the mmap_lock >> @@ -1293,7 +1310,7 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s >> LIST_HEAD(compound_pagelist); >> pmd_t *pmd, _pmd; >> pte_t *pte = NULL; >> - pgtable_t pgtable; >> + pgtable_t pgtable = NULL; >> struct folio *folio; >> spinlock_t *pmd_ptl, *pte_ptl; >> enum scan_result result = SCAN_FAIL; >> @@ -1310,6 +1327,12 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s >> goto out_nolock; >> } >> >> + if (is_pmd_order(order)) { >> + pgtable = alloc_deposit_pte(mm); >> + if (!pgtable) >> + goto out_nolock; > > Fixup is here: > > - if (!pgtable) > - goto out_nolock; > + if (!pgtable) { > + result = SCAN_ALLOC_HUGE_PAGE_FAIL; > + goto out_nolock; > + } With the fix applied, LGTM. Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
diff --git a/mm/khugepaged.c b/mm/khugepaged.c index f49a6710933b..dab421f8233e 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -1278,6 +1278,23 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru return SCAN_SUCCEED; } +static pgtable_t alloc_deposit_pte(struct mm_struct *mm) +{ + /* + * khugepaged is run from a kernel thread, so need to manually set the + * correct memcg so the allocation gets charged correctly. + */ + struct mem_cgroup *memcg = get_mem_cgroup_from_mm(mm); + struct mem_cgroup *old_memcg = set_active_memcg(memcg); + pgtable_t pgtable; + + pgtable = pte_alloc_one(mm); + + set_active_memcg(old_memcg); + mem_cgroup_put(memcg); + return pgtable; +} + /* * collapse_huge_page() expects the mmap_lock to be unlocked before entering and * will always return with the lock unlocked, to avoid holding the mmap_lock @@ -1293,7 +1310,7 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s LIST_HEAD(compound_pagelist); pmd_t *pmd, _pmd; pte_t *pte = NULL; - pgtable_t pgtable; + pgtable_t pgtable = NULL; struct folio *folio; spinlock_t *pmd_ptl, *pte_ptl; enum scan_result result = SCAN_FAIL; @@ -1310,6 +1327,12 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s goto out_nolock; } + if (is_pmd_order(order)) { + pgtable = alloc_deposit_pte(mm); + if (!pgtable) + goto out_nolock; + } + mmap_read_lock(mm); result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true, &vma, cc, order); @@ -1433,8 +1456,8 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s spin_lock(pmd_ptl); VM_WARN_ON_ONCE(!pmd_none(*pmd)); if (is_pmd_order(order)) { - pgtable = pmd_pgtable(_pmd); pgtable_trans_huge_deposit(mm, pmd, pgtable); + pgtable = NULL; map_anon_folio_pmd_nopf(folio, pmd, vma, pmd_addr); } else { /* @@ -1453,6 +1476,9 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s } spin_unlock(pmd_ptl); + if (is_pmd_order(order)) + pte_free_defer(mm, pmd_pgtable(_pmd)); + folio = NULL; result = SCAN_SUCCEED; @@ -1463,6 +1489,8 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s anon_vma_unlock_write(vma->anon_vma); mmap_write_unlock(mm); out_nolock: + if (pgtable) + pte_free(mm, pgtable); if (folio) folio_put(folio); trace_mm_collapse_huge_page(mm, result == SCAN_SUCCEED, result, order);
collapse_huge_page() deposits a PTE page table on PMD collapse in order that it can be utilised for subsequent split operations, meaning that those operations do not need to perform an allocation (as they are in a context where it might be unwise). However the PTE page table which is deposited is the one which is currently mapped by the PMD entry that is in the process of being collapsed. Once deposited, the PTE page table may be used in a split of any other unrelated PMD entry. This is currently not an issue as this operation is performed with VMA/mmap write lock + anon rmap locks held, so ordinary page table walkers will never accidentally end up walking the wrong thing, and GUP-fast is protected by an IPI via tlb_remove_table_sync_one(). However, the series to which this commit belongs implements RCU-safe page table traversal, at which point this becomes problematic. This can be resolved by using pte_offset_map_lock() which gates on a PTE PTL and a pmd_same() check, but lockless walks are unsafe as things stand. Resolve this by simply allocating a new, zeroed, PTE page table to deposit at the point of collapse. This path is already costly and an allocation has already been performed for the huge folio, so this allocation is statistical noise in terms of performance and memory usage at this point. With this PTE page table deposited, RCU-free the existing PTE page table so it is safe for page table walkers to traverse within a grace period. This also brings this deposit case in line with all other page table deposit logic which deposit a fresh page table. Additionally, this was the only place in the kernel that displaced a page table like this, so eliminating it also helps consistency. An edge case for deposit exists for powerpc and its hash-based MMU - it stores hash slot data in deposited page tables and zeroes them on withdraw, so a zeroed deposited page table works correctly for it. Since khugepaged runs as a kernel thread, do a little dance in alloc_deposit_pte() to correctly charge the allocation. This is already done for the folio allocation via alloc_charge_folio() but no such wrapper exists for a page table allocation. Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org> --- mm/khugepaged.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)