Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On Wed, Jan 28, 2026 at 09:38:37AM -0700, Nico Pache wrote:
> Hi Andrew,
>
> could you please apply the following fixup to avoid potentially using a stale
> VMA in the new writeback-retry logic for madvise collapse.
>
> Thank you!
> -- Nico
>
> 8<
> commit a9ac3b1bfa926dd707ac3a785583f8d7a0579578
> Author: Nico Pache
> Date: Fri Jan 23 16:32:42 2026 -0700
>
> madvise writeback retry logic fix
>
> Signed-off-by: Nico Pache
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 59e5a5588d85..2b054f7d9753 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2418,6 +2418,14 @@ static enum scan_result collapse_single_pmd(unsigned
> long
> addr,
> mmap_read_unlock(mm);
> *mmap_locked = false;
> result = collapse_scan_file(mm, addr, file, pgoff, cc);
> +
> + if (!cc->is_khugepaged && result == SCAN_PAGE_DIRTY_OR_WRITEBACK &&
> + mapping_can_writeback(file->f_mapping)) {
> + const loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
> + const loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
> +
> + filemap_write_and_wait_range(file->f_mapping, lstart, lend);
> + }
> fput(file);
>
> if (result != SCAN_PTE_MAPPED_HUGEPAGE)
> @@ -2840,19 +2848,8 @@ int madvise_collapse(struct vm_area_struct *vma,
> unsigned
> long start,
> *lock_dropped = true;
>
> if (result == SCAN_PAGE_DIRTY_OR_WRITEBACK && !triggered_wb) {
> - struct file *file = get_file(vma->vm_file);
> - pgoff_t pgoff = linear_page_index(vma, addr);
> -
> - if (mapping_can_writeback(file->f_mapping)) {
> - loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
> - loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
> -
> - filemap_write_and_wait_range(file->f_mapping,
> lstart, lend);
> - triggered_wb = true;
> - fput(file);
> - goto retry;
> - }
> - fput(file);
> + triggered_wb = true;
> + goto retry;
> }
LGTM, with this in place you can add back my tag to the patch.
It'd be good to reference this in the commit message, but you can do that
on a respin.
So:
Reviewed-by: Lorenzo Stoakes
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
Another trivial point on this one, you're missing the prefix in the subject here. In general I think better to say mm/khugepaged: rather than khugepaged: also. Cheers, Lorenzo
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
Hi Andrew,
could you please apply the following fixup to avoid potentially using a stale
VMA in the new writeback-retry logic for madvise collapse.
Thank you!
-- Nico
8<
commit a9ac3b1bfa926dd707ac3a785583f8d7a0579578
Author: Nico Pache
Date: Fri Jan 23 16:32:42 2026 -0700
madvise writeback retry logic fix
Signed-off-by: Nico Pache
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 59e5a5588d85..2b054f7d9753 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2418,6 +2418,14 @@ static enum scan_result collapse_single_pmd(unsigned long
addr,
mmap_read_unlock(mm);
*mmap_locked = false;
result = collapse_scan_file(mm, addr, file, pgoff, cc);
+
+ if (!cc->is_khugepaged && result == SCAN_PAGE_DIRTY_OR_WRITEBACK &&
+ mapping_can_writeback(file->f_mapping)) {
+ const loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
+ const loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
+
+ filemap_write_and_wait_range(file->f_mapping, lstart, lend);
+ }
fput(file);
if (result != SCAN_PTE_MAPPED_HUGEPAGE)
@@ -2840,19 +2848,8 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned
long start,
*lock_dropped = true;
if (result == SCAN_PAGE_DIRTY_OR_WRITEBACK && !triggered_wb) {
- struct file *file = get_file(vma->vm_file);
- pgoff_t pgoff = linear_page_index(vma, addr);
-
- if (mapping_can_writeback(file->f_mapping)) {
- loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
- loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
-
- filemap_write_and_wait_range(file->f_mapping,
lstart, lend);
- triggered_wb = true;
- fput(file);
- goto retry;
- }
- fput(file);
+ triggered_wb = true;
+ goto retry;
}
switch (result) {
--
2.52.0
On 1/22/26 12:28 PM, Nico Pache wrote:
> The khugepaged daemon and madvise_collapse have two different
> implementations that do almost the same thing.
>
> Create collapse_single_pmd to increase code reuse and create an entry
> point to these two users.
>
> Refactor madvise_collapse and collapse_scan_mm_slot to use the new
> collapse_single_pmd function. This introduces a minor behavioral change
> that is most likely an undiscovered bug. The current implementation of
> khugepaged tests collapse_test_exit_or_disable before calling
> collapse_pte_mapped_thp, but we weren't doing it in the madvise_collapse
> case. By unifying these two callers madvise_collapse now also performs
> this check. We also modify the return value to be SCAN_ANY_PROCESS which
> properly indicates that this process is no longer valid to operate on.
>
> We also guard the khugepaged_pages_collapsed variable to ensure its only
> incremented for khugepaged.
>
> Reviewed-by: Wei Yang
> Reviewed-by: Lance Yang
> Reviewed-by: Lorenzo Stoakes
> Reviewed-by: Baolin Wang
> Reviewed-by: Zi Yan
> Acked-by: David Hildenbrand
> Signed-off-by: Nico Pache
> ---
> mm/khugepaged.c | 106 +++-
> 1 file changed, 60 insertions(+), 46 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index fefcbdca4510..59e5a5588d85 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2394,6 +2394,54 @@ static enum scan_result collapse_scan_file(struct
> mm_struct *mm, unsigned long a
> return result;
> }
>
> +/*
> + * Try to collapse a single PMD starting at a PMD aligned addr, and return
> + * the results.
> + */
> +static enum scan_result collapse_single_pmd(unsigned long addr,
> + struct vm_area_struct *vma, bool *mmap_locked,
> + struct collapse_control *cc)
> +{
> + struct mm_struct *mm = vma->vm_mm;
> + enum scan_result result;
> + struct file *file;
> + pgoff_t pgoff;
> +
> + if (vma_is_anonymous(vma)) {
> + result = collapse_scan_pmd(mm, vma, addr, mmap_locked, cc);
> + goto end;
> + }
> +
> + file = get_file(vma->vm_file);
> + pgoff = linear_page_index(vma, addr);
> +
> + mmap_read_unlock(mm);
> + *mmap_locked = false;
> + result = collapse_scan_file(mm, addr, file, pgoff, cc);
> + fput(file);
> +
> + if (result != SCAN_PTE_MAPPED_HUGEPAGE)
> + goto end;
> +
> + mmap_read_lock(mm);
> + *mmap_locked = true;
> + if (collapse_test_exit_or_disable(mm)) {
> + mmap_read_unlock(mm);
> + *mmap_locked = false;
> + return SCAN_ANY_PROCESS;
> + }
> + result = try_collapse_pte_mapped_thp(mm, addr, !cc->is_khugepaged);
> + if (result == SCAN_PMD_MAPPED)
> + result = SCAN_SUCCEED;
> + mmap_read_u
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On Mon, Jan 26, 2026 at 07:09:18AM -0800, Andrew Morton wrote: > On Mon, 26 Jan 2026 11:40:21 + Lorenzo Stoakes > wrote: > > > Andrew - when this goes into mm-new if there isn't a respin between, please > > drop all tags except any obviously sent re: the fix-patch. > > > > I've been believing this is next -rc1 material. Was that mistaken? Yeah this isn't ready yet sorry. I did hope we could get this in this cycle but there's too much to check (esp. given this change for isntance0 and we need more time to stabilise it, so please keep this out of mm-(un)stable for now. It's a really huge change to THP so we need to take our time with it. So we're aiming for 6.21-rc1 / 7.1-rc1 or whatever deems it to be :) i.e. next+1-rc1. Cheers, Lorenzo
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On Mon, 26 Jan 2026 11:40:21 + Lorenzo Stoakes wrote: > Andrew - when this goes into mm-new if there isn't a respin between, please > drop all tags except any obviously sent re: the fix-patch. > I've been believing this is next -rc1 material. Was that mistaken?
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On Fri, Jan 23, 2026 at 04:26:09PM -0700, Nico Pache wrote: > On Thu, Jan 22, 2026 at 10:08 PM Lance Yang wrote: > > > > > > > > On 2026/1/23 03:28, Nico Pache wrote: > > > The khugepaged daemon and madvise_collapse have two different > > > implementations that do almost the same thing. > > > > > > Create collapse_single_pmd to increase code reuse and create an entry > > > point to these two users. > > > > > > Refactor madvise_collapse and collapse_scan_mm_slot to use the new > > > collapse_single_pmd function. This introduces a minor behavioral change > > > that is most likely an undiscovered bug. The current implementation of > > > khugepaged tests collapse_test_exit_or_disable before calling > > > collapse_pte_mapped_thp, but we weren't doing it in the madvise_collapse > > > case. By unifying these two callers madvise_collapse now also performs > > > this check. We also modify the return value to be SCAN_ANY_PROCESS which > > > properly indicates that this process is no longer valid to operate on. > > > > > > We also guard the khugepaged_pages_collapsed variable to ensure its only > > > incremented for khugepaged. > > > > > > Reviewed-by: Wei Yang > > > Reviewed-by: Lance Yang > > > Reviewed-by: Lorenzo Stoakes > > > Reviewed-by: Baolin Wang > > > Reviewed-by: Zi Yan > > > Acked-by: David Hildenbrand > > > Signed-off-by: Nico Pache > > > --- > > > > I think this patch introduces some functional changes compared to previous > > version[1] ... > > > > Maybe we should drop the r-b tags and let folks take another look? > > > > There might be an issue with the vma access in madvise_collapse(). See > > below: > > > > [1] > > https://lore.kernel.org/linux-mm/[email protected]/ > > > > > mm/khugepaged.c | 106 +++- > > > 1 file changed, 60 insertions(+), 46 deletions(-) > > > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > > index fefcbdca4510..59e5a5588d85 100644 > > > --- a/mm/khugepaged.c > > > +++ b/mm/khugepaged.c > > > @@ -2394,6 +2394,54 @@ static enum scan_result collapse_scan_file(struct > > > mm_struct *mm, unsigned long a > > > return result; > > > } > > > > > > +/* > > > + * Try to collapse a single PMD starting at a PMD aligned addr, and > > > return > > > + * the results. > > > + */ > > > +static enum scan_result collapse_single_pmd(unsigned long addr, > > > + struct vm_area_struct *vma, bool *mmap_locked, > > > + struct collapse_control *cc) > > > +{ > > > + struct mm_struct *mm = vma->vm_mm; > > > + enum scan_result result; > > > + struct file *file; > > > + pgoff_t pgoff; > > > + > > > + if (vma_is_anonymous(vma)) { > > > + result = collapse_scan_pmd(mm, vma, addr, mmap_locked, cc); > > > + goto end; > > > + } > > > + > > > + file = get_file(vma->vm_file); > > > + pgoff = linear_page_index(vma, addr); > > > + > > > + mmap_read_unlock(mm); > > > + *mmap_locked = false; > > > + result = collapse_scan_file(mm, addr, file, pgoff, cc); > > > + fput(file); > > > + > > > + if (result != SCAN_PTE_MAPPED_HUGEPAGE) > > > + goto end; > > > + > > > + mmap_read_lock(mm); > > > + *mmap_locked = true; > > > + if (collapse_test_exit_or_disable(mm)) { > > > + mmap_read_unlock(mm); > > > + *mmap_locked = false; > > > + return SCAN_ANY_PROCESS; > > > + } > > > + result = try_collapse_pte_mapped_thp(mm, addr, !cc->is_khugepaged); > > > + if (result == SCAN_PMD_MAPPED) > > > + result = SCAN_SUCCEED; > > > + mmap_read_unlock(mm); > > > + *mmap_locked = false; > > > + > > > +end: > > > + if (cc->is_khugepaged && result == SCAN_SUCCEED) > > > + ++khugepaged_pages_collapsed; > > > + return result; > > > +} > > > + > > > static unsigned int collapse_scan_mm_slot(unsigned int pages, enum > > > scan_result *result, > > > struct collapse_control *cc) > > > __releases(&khugepaged_mm_lock) > > > @@ -2466,34 +2514,9 @@ static unsigned int collapse_scan_mm_slot(unsigned > > > int pages, enum scan_result * > > > VM_BUG_ON(khugepaged_scan.address < hstart || > > > khugepaged_scan.address + HPAGE_PMD_SIZE > > > > hend); > > > - if (!vma_is_anonymous(vma)) { > > > - struct file *file = get_file(vma->vm_file); > > > - pgoff_t pgoff = linear_page_index(vma, > > > - khugepaged_scan.address); > > > - > > > - mmap_read_unlock(mm); > > > - mmap_locked = false; > > > - *result = collapse_scan_file(mm, > > > - khugepaged_scan.address, file, > > > pgoff, cc); > > >
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On Fri, Jan 23, 2026 at 05:31:17PM +0800, Baolin Wang wrote: > > > On 1/23/26 1:07 PM, Lance Yang wrote: > > > > > > After collapse_single_pmd() returns, mmap_lock might have been released. > > Between > > that unlock and here, another thread could unmap/remap the VMA, making > > the vma > > pointer stale when we access vma->vm_file? > > > > Would it be safer to get the file reference before calling > > collapse_single_pmd()? > > Or we need to revalidate the VMA after getting the lock back? > Good catch. I think we can move the filemap_write_and_wait_range() related > logic into collapse_single_pmd(), after we get a file reference. Good suggestion, is what Nico did in the suggested patch :) Agreed better there. Thanks, Lorenzo
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
Andrew - when this goes into mm-new if there isn't a respin between, please drop all tags except any obviously sent re: the fix-patch. Thanks! On Fri, Jan 23, 2026 at 01:07:16PM +0800, Lance Yang wrote: > > > On 2026/1/23 03:28, Nico Pache wrote: > > The khugepaged daemon and madvise_collapse have two different > > implementations that do almost the same thing. > > > > Create collapse_single_pmd to increase code reuse and create an entry > > point to these two users. > > > > Refactor madvise_collapse and collapse_scan_mm_slot to use the new > > collapse_single_pmd function. This introduces a minor behavioral change > > that is most likely an undiscovered bug. The current implementation of > > khugepaged tests collapse_test_exit_or_disable before calling > > collapse_pte_mapped_thp, but we weren't doing it in the madvise_collapse > > case. By unifying these two callers madvise_collapse now also performs > > this check. We also modify the return value to be SCAN_ANY_PROCESS which > > properly indicates that this process is no longer valid to operate on. > > > > We also guard the khugepaged_pages_collapsed variable to ensure its only > > incremented for khugepaged. > > > > Reviewed-by: Wei Yang > > Reviewed-by: Lance Yang > > Reviewed-by: Lorenzo Stoakes > > Reviewed-by: Baolin Wang > > Reviewed-by: Zi Yan > > Acked-by: David Hildenbrand > > Signed-off-by: Nico Pache > > --- > > I think this patch introduces some functional changes compared to previous > version[1] ... > > Maybe we should drop the r-b tags and let folks take another look? Yes thanks Lance, absolutely this should happen. Especially on a small-iteration respin (I really wanted to get to v13 but the rebase issue killed that). I know it wasn't intentional, not suggesting that of course :) just obviously as a process thing - it's _very_ important to make clear what you've changed and what you haven't. For truly minor changes no need to drop the tags, but often my workflow is: - Check which patches I haven't reviewed yet. - Go review those. So I might well have missed that. I often try to do a git range-diff, but in this case I probably wouldn't have on basis of the v13 having merge conflicts. But obviously given the above I went and fixed them up and applied v13 locally so I could check everything :) > > There might be an issue with the vma access in madvise_collapse(). See > below: > > [1] > https://lore.kernel.org/linux-mm/[email protected]/ > > > mm/khugepaged.c | 106 +++- > > 1 file changed, 60 insertions(+), 46 deletions(-) > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > index fefcbdca4510..59e5a5588d85 100644 > > --- a/mm/khugepaged.c > > +++ b/mm/khugepaged.c > > @@ -2394,6 +2394,54 @@ static enum scan_result collapse_scan_file(struct > > mm_struct *mm, unsigned long a > > return result; > > } > > +/* > > + * Try to collapse a single PMD starting at a PMD aligned addr, and return > > + * the results. > > + */ > > +static enum scan_result collapse_single_pmd(unsigned long addr, > > + struct vm_area_struct *vma, bool *mmap_locked, > > + struct collapse_control *cc) > > +{ > > + struct mm_struct *mm = vma->vm_mm; > > + enum scan_result result; > > + struct file *file; > > + pgoff_t pgoff; > > + > > + if (vma_is_anonymous(vma)) { > > + result = collapse_scan_pmd(mm, vma, addr, mmap_locked, cc); > > + goto end; > > + } > > + > > + file = get_file(vma->vm_file); > > + pgoff = linear_page_index(vma, addr); > > + > > + mmap_read_unlock(mm); > > + *mmap_locked = false; > > + result = collapse_scan_file(mm, addr, file, pgoff, cc); > > + fput(file); > > + > > + if (result != SCAN_PTE_MAPPED_HUGEPAGE) > > + goto end; > > + > > + mmap_read_lock(mm); > > + *mmap_locked = true; > > + if (collapse_test_exit_or_disable(mm)) { > > + mmap_read_unlock(mm); > > + *mmap_locked = false; > > + return SCAN_ANY_PROCESS; > > + } > > + result = try_collapse_pte_mapped_thp(mm, addr, !cc->is_khugepaged); > > + if (result == SCAN_PMD_MAPPED) > > + result = SCAN_SUCCEED; > > + mmap_read_unlock(mm); > > + *mmap_locked = false; > > + > > +end: > > + if (cc->is_khugepaged && result == SCAN_SUCCEED) > > + ++khugepaged_pages_collapsed; > > + return result; > > +} > > + > > static unsigned int collapse_scan_mm_slot(unsigned int pages, enum > > scan_result *result, > > struct collapse_control *cc) > > __releases(&khugepaged_mm_lock) > > @@ -2466,34 +2514,9 @@ static unsigned int collapse_scan_mm_slot(unsigned > > int pages, enum scan_result * > > VM_BUG_ON(khugepaged_scan.address < hstart || > > khugepaged_scan.address + HPAGE_PMD_SIZE > > > hend); > > - if (!vma_is_anonymous(vma)) { > >
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On 2026/1/24 07:26, Nico Pache wrote: On Thu, Jan 22, 2026 at 10:08 PM Lance Yang wrote: On 2026/1/23 03:28, Nico Pache wrote: The khugepaged daemon and madvise_collapse have two different implementations that do almost the same thing. Create collapse_single_pmd to increase code reuse and create an entry point to these two users. Refactor madvise_collapse and collapse_scan_mm_slot to use the new collapse_single_pmd function. This introduces a minor behavioral change that is most likely an undiscovered bug. The current implementation of khugepaged tests collapse_test_exit_or_disable before calling collapse_pte_mapped_thp, but we weren't doing it in the madvise_collapse case. By unifying these two callers madvise_collapse now also performs this check. We also modify the return value to be SCAN_ANY_PROCESS which properly indicates that this process is no longer valid to operate on. We also guard the khugepaged_pages_collapsed variable to ensure its only incremented for khugepaged. Reviewed-by: Wei Yang Reviewed-by: Lance Yang Reviewed-by: Lorenzo Stoakes Reviewed-by: Baolin Wang Reviewed-by: Zi Yan Acked-by: David Hildenbrand Signed-off-by: Nico Pache --- I think this patch introduces some functional changes compared to previous version[1] ... Maybe we should drop the r-b tags and let folks take another look? There might be an issue with the vma access in madvise_collapse(). See below: [1] https://lore.kernel.org/linux-mm/[email protected]/ mm/khugepaged.c | 106 +++- 1 file changed, 60 insertions(+), 46 deletions(-) diff --git a/mm/khugepaged.c b/mm/khugepaged.c index fefcbdca4510..59e5a5588d85 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -2394,6 +2394,54 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm, unsigned long a return result; } +/* + * Try to collapse a single PMD starting at a PMD aligned addr, and return + * the results. + */ +static enum scan_result collapse_single_pmd(unsigned long addr, + struct vm_area_struct *vma, bool *mmap_locked, + struct collapse_control *cc) +{ + struct mm_struct *mm = vma->vm_mm; + enum scan_result result; + struct file *file; + pgoff_t pgoff; + + if (vma_is_anonymous(vma)) { + result = collapse_scan_pmd(mm, vma, addr, mmap_locked, cc); + goto end; + } + + file = get_file(vma->vm_file); + pgoff = linear_page_index(vma, addr); + + mmap_read_unlock(mm); + *mmap_locked = false; + result = collapse_scan_file(mm, addr, file, pgoff, cc); + fput(file); + + if (result != SCAN_PTE_MAPPED_HUGEPAGE) + goto end; + + mmap_read_lock(mm); + *mmap_locked = true; + if (collapse_test_exit_or_disable(mm)) { + mmap_read_unlock(mm); + *mmap_locked = false; + return SCAN_ANY_PROCESS; + } + result = try_collapse_pte_mapped_thp(mm, addr, !cc->is_khugepaged); + if (result == SCAN_PMD_MAPPED) + result = SCAN_SUCCEED; + mmap_read_unlock(mm); + *mmap_locked = false; + +end: + if (cc->is_khugepaged && result == SCAN_SUCCEED) + ++khugepaged_pages_collapsed; + return result; +} + static unsigned int collapse_scan_mm_slot(unsigned int pages, enum scan_result *result, struct collapse_control *cc) __releases(&khugepaged_mm_lock) @@ -2466,34 +2514,9 @@ static unsigned int collapse_scan_mm_slot(unsigned int pages, enum scan_result * VM_BUG_ON(khugepaged_scan.address < hstart || khugepaged_scan.address + HPAGE_PMD_SIZE > hend); - if (!vma_is_anonymous(vma)) { - struct file *file = get_file(vma->vm_file); - pgoff_t pgoff = linear_page_index(vma, - khugepaged_scan.address); - - mmap_read_unlock(mm); - mmap_locked = false; - *result = collapse_scan_file(mm, - khugepaged_scan.address, file, pgoff, cc); - fput(file); - if (*result == SCAN_PTE_MAPPED_HUGEPAGE) { - mmap_read_lock(mm); - if (collapse_test_exit_or_disable(mm)) - goto breakouterloop; - *result = try_collapse_pte_mapped_thp(mm, - khugepaged_scan.address, false); - if (*result == SCAN_PMD_MAPPED) - *result = SCAN_SUCCEED; - mmap_read_unlock(mm); -
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On Thu, Jan 22, 2026 at 10:08 PM Lance Yang wrote: > > > > On 2026/1/23 03:28, Nico Pache wrote: > > The khugepaged daemon and madvise_collapse have two different > > implementations that do almost the same thing. > > > > Create collapse_single_pmd to increase code reuse and create an entry > > point to these two users. > > > > Refactor madvise_collapse and collapse_scan_mm_slot to use the new > > collapse_single_pmd function. This introduces a minor behavioral change > > that is most likely an undiscovered bug. The current implementation of > > khugepaged tests collapse_test_exit_or_disable before calling > > collapse_pte_mapped_thp, but we weren't doing it in the madvise_collapse > > case. By unifying these two callers madvise_collapse now also performs > > this check. We also modify the return value to be SCAN_ANY_PROCESS which > > properly indicates that this process is no longer valid to operate on. > > > > We also guard the khugepaged_pages_collapsed variable to ensure its only > > incremented for khugepaged. > > > > Reviewed-by: Wei Yang > > Reviewed-by: Lance Yang > > Reviewed-by: Lorenzo Stoakes > > Reviewed-by: Baolin Wang > > Reviewed-by: Zi Yan > > Acked-by: David Hildenbrand > > Signed-off-by: Nico Pache > > --- > > I think this patch introduces some functional changes compared to previous > version[1] ... > > Maybe we should drop the r-b tags and let folks take another look? > > There might be an issue with the vma access in madvise_collapse(). See > below: > > [1] > https://lore.kernel.org/linux-mm/[email protected]/ > > > mm/khugepaged.c | 106 +++- > > 1 file changed, 60 insertions(+), 46 deletions(-) > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > index fefcbdca4510..59e5a5588d85 100644 > > --- a/mm/khugepaged.c > > +++ b/mm/khugepaged.c > > @@ -2394,6 +2394,54 @@ static enum scan_result collapse_scan_file(struct > > mm_struct *mm, unsigned long a > > return result; > > } > > > > +/* > > + * Try to collapse a single PMD starting at a PMD aligned addr, and return > > + * the results. > > + */ > > +static enum scan_result collapse_single_pmd(unsigned long addr, > > + struct vm_area_struct *vma, bool *mmap_locked, > > + struct collapse_control *cc) > > +{ > > + struct mm_struct *mm = vma->vm_mm; > > + enum scan_result result; > > + struct file *file; > > + pgoff_t pgoff; > > + > > + if (vma_is_anonymous(vma)) { > > + result = collapse_scan_pmd(mm, vma, addr, mmap_locked, cc); > > + goto end; > > + } > > + > > + file = get_file(vma->vm_file); > > + pgoff = linear_page_index(vma, addr); > > + > > + mmap_read_unlock(mm); > > + *mmap_locked = false; > > + result = collapse_scan_file(mm, addr, file, pgoff, cc); > > + fput(file); > > + > > + if (result != SCAN_PTE_MAPPED_HUGEPAGE) > > + goto end; > > + > > + mmap_read_lock(mm); > > + *mmap_locked = true; > > + if (collapse_test_exit_or_disable(mm)) { > > + mmap_read_unlock(mm); > > + *mmap_locked = false; > > + return SCAN_ANY_PROCESS; > > + } > > + result = try_collapse_pte_mapped_thp(mm, addr, !cc->is_khugepaged); > > + if (result == SCAN_PMD_MAPPED) > > + result = SCAN_SUCCEED; > > + mmap_read_unlock(mm); > > + *mmap_locked = false; > > + > > +end: > > + if (cc->is_khugepaged && result == SCAN_SUCCEED) > > + ++khugepaged_pages_collapsed; > > + return result; > > +} > > + > > static unsigned int collapse_scan_mm_slot(unsigned int pages, enum > > scan_result *result, > > struct collapse_control *cc) > > __releases(&khugepaged_mm_lock) > > @@ -2466,34 +2514,9 @@ static unsigned int collapse_scan_mm_slot(unsigned > > int pages, enum scan_result * > > VM_BUG_ON(khugepaged_scan.address < hstart || > > khugepaged_scan.address + HPAGE_PMD_SIZE > > > hend); > > - if (!vma_is_anonymous(vma)) { > > - struct file *file = get_file(vma->vm_file); > > - pgoff_t pgoff = linear_page_index(vma, > > - khugepaged_scan.address); > > - > > - mmap_read_unlock(mm); > > - mmap_locked = false; > > - *result = collapse_scan_file(mm, > > - khugepaged_scan.address, file, pgoff, > > cc); > > - fput(file); > > - if (*result == SCAN_PTE_MAPPED_HUGEPAGE) { > > - mmap_read_lock(mm); > > - if (collapse_test_exit_or_disable(mm)) > > -
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On 1/23/26 1:07 PM, Lance Yang wrote: On 2026/1/23 03:28, Nico Pache wrote: The khugepaged daemon and madvise_collapse have two different implementations that do almost the same thing. Create collapse_single_pmd to increase code reuse and create an entry point to these two users. Refactor madvise_collapse and collapse_scan_mm_slot to use the new collapse_single_pmd function. This introduces a minor behavioral change that is most likely an undiscovered bug. The current implementation of khugepaged tests collapse_test_exit_or_disable before calling collapse_pte_mapped_thp, but we weren't doing it in the madvise_collapse case. By unifying these two callers madvise_collapse now also performs this check. We also modify the return value to be SCAN_ANY_PROCESS which properly indicates that this process is no longer valid to operate on. We also guard the khugepaged_pages_collapsed variable to ensure its only incremented for khugepaged. Reviewed-by: Wei Yang Reviewed-by: Lance Yang Reviewed-by: Lorenzo Stoakes Reviewed-by: Baolin Wang Reviewed-by: Zi Yan Acked-by: David Hildenbrand Signed-off-by: Nico Pache --- I think this patch introduces some functional changes compared to previous version[1] ... Maybe we should drop the r-b tags and let folks take another look? There might be an issue with the vma access in madvise_collapse(). See below: [1] https://lore.kernel.org/linux-mm/20251201174627.23295-3- [email protected]/ mm/khugepaged.c | 106 +++- 1 file changed, 60 insertions(+), 46 deletions(-) diff --git a/mm/khugepaged.c b/mm/khugepaged.c index fefcbdca4510..59e5a5588d85 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -2394,6 +2394,54 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm, unsigned long a return result; } +/* + * Try to collapse a single PMD starting at a PMD aligned addr, and return + * the results. + */ +static enum scan_result collapse_single_pmd(unsigned long addr, + struct vm_area_struct *vma, bool *mmap_locked, + struct collapse_control *cc) +{ + struct mm_struct *mm = vma->vm_mm; + enum scan_result result; + struct file *file; + pgoff_t pgoff; + + if (vma_is_anonymous(vma)) { + result = collapse_scan_pmd(mm, vma, addr, mmap_locked, cc); + goto end; + } + + file = get_file(vma->vm_file); + pgoff = linear_page_index(vma, addr); + + mmap_read_unlock(mm); + *mmap_locked = false; + result = collapse_scan_file(mm, addr, file, pgoff, cc); + fput(file); + + if (result != SCAN_PTE_MAPPED_HUGEPAGE) + goto end; + + mmap_read_lock(mm); + *mmap_locked = true; + if (collapse_test_exit_or_disable(mm)) { + mmap_read_unlock(mm); + *mmap_locked = false; + return SCAN_ANY_PROCESS; + } + result = try_collapse_pte_mapped_thp(mm, addr, !cc->is_khugepaged); + if (result == SCAN_PMD_MAPPED) + result = SCAN_SUCCEED; + mmap_read_unlock(mm); + *mmap_locked = false; + +end: + if (cc->is_khugepaged && result == SCAN_SUCCEED) + ++khugepaged_pages_collapsed; + return result; +} + static unsigned int collapse_scan_mm_slot(unsigned int pages, enum scan_result *result, struct collapse_control *cc) __releases(&khugepaged_mm_lock) @@ -2466,34 +2514,9 @@ static unsigned int collapse_scan_mm_slot(unsigned int pages, enum scan_result * VM_BUG_ON(khugepaged_scan.address < hstart || khugepaged_scan.address + HPAGE_PMD_SIZE > hend); - if (!vma_is_anonymous(vma)) { - struct file *file = get_file(vma->vm_file); - pgoff_t pgoff = linear_page_index(vma, - khugepaged_scan.address); - - mmap_read_unlock(mm); - mmap_locked = false; - *result = collapse_scan_file(mm, - khugepaged_scan.address, file, pgoff, cc); - fput(file); - if (*result == SCAN_PTE_MAPPED_HUGEPAGE) { - mmap_read_lock(mm); - if (collapse_test_exit_or_disable(mm)) - goto breakouterloop; - *result = try_collapse_pte_mapped_thp(mm, - khugepaged_scan.address, false); - if (*result == SCAN_PMD_MAPPED) - *result = SCAN_SUCCEED; - mmap_read_unlock(mm); - } - } else { - *result = collapse_scan_pmd(mm, vma, - khugepaged_scan.address, &mmap_locked, cc); - } - - if (*result == SCAN_SUCCEED) - ++khugepaged_pages_collapsed; + *result = collapse_single_pmd(khugepaged_scan.address, + vma, &mmap_locked, cc); /* move to next address */ khugepaged_scan.address += HPAGE_PMD_SIZ
Re: [PATCH mm-unstable v14 03/16] introduce collapse_single_pmd to unify khugepaged and madvise_collapse
On 2026/1/23 03:28, Nico Pache wrote: The khugepaged daemon and madvise_collapse have two different implementations that do almost the same thing. Create collapse_single_pmd to increase code reuse and create an entry point to these two users. Refactor madvise_collapse and collapse_scan_mm_slot to use the new collapse_single_pmd function. This introduces a minor behavioral change that is most likely an undiscovered bug. The current implementation of khugepaged tests collapse_test_exit_or_disable before calling collapse_pte_mapped_thp, but we weren't doing it in the madvise_collapse case. By unifying these two callers madvise_collapse now also performs this check. We also modify the return value to be SCAN_ANY_PROCESS which properly indicates that this process is no longer valid to operate on. We also guard the khugepaged_pages_collapsed variable to ensure its only incremented for khugepaged. Reviewed-by: Wei Yang Reviewed-by: Lance Yang Reviewed-by: Lorenzo Stoakes Reviewed-by: Baolin Wang Reviewed-by: Zi Yan Acked-by: David Hildenbrand Signed-off-by: Nico Pache --- I think this patch introduces some functional changes compared to previous version[1] ... Maybe we should drop the r-b tags and let folks take another look? There might be an issue with the vma access in madvise_collapse(). See below: [1] https://lore.kernel.org/linux-mm/[email protected]/ mm/khugepaged.c | 106 +++- 1 file changed, 60 insertions(+), 46 deletions(-) diff --git a/mm/khugepaged.c b/mm/khugepaged.c index fefcbdca4510..59e5a5588d85 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -2394,6 +2394,54 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm, unsigned long a return result; } +/* + * Try to collapse a single PMD starting at a PMD aligned addr, and return + * the results. + */ +static enum scan_result collapse_single_pmd(unsigned long addr, + struct vm_area_struct *vma, bool *mmap_locked, + struct collapse_control *cc) +{ + struct mm_struct *mm = vma->vm_mm; + enum scan_result result; + struct file *file; + pgoff_t pgoff; + + if (vma_is_anonymous(vma)) { + result = collapse_scan_pmd(mm, vma, addr, mmap_locked, cc); + goto end; + } + + file = get_file(vma->vm_file); + pgoff = linear_page_index(vma, addr); + + mmap_read_unlock(mm); + *mmap_locked = false; + result = collapse_scan_file(mm, addr, file, pgoff, cc); + fput(file); + + if (result != SCAN_PTE_MAPPED_HUGEPAGE) + goto end; + + mmap_read_lock(mm); + *mmap_locked = true; + if (collapse_test_exit_or_disable(mm)) { + mmap_read_unlock(mm); + *mmap_locked = false; + return SCAN_ANY_PROCESS; + } + result = try_collapse_pte_mapped_thp(mm, addr, !cc->is_khugepaged); + if (result == SCAN_PMD_MAPPED) + result = SCAN_SUCCEED; + mmap_read_unlock(mm); + *mmap_locked = false; + +end: + if (cc->is_khugepaged && result == SCAN_SUCCEED) + ++khugepaged_pages_collapsed; + return result; +} + static unsigned int collapse_scan_mm_slot(unsigned int pages, enum scan_result *result, struct collapse_control *cc) __releases(&khugepaged_mm_lock) @@ -2466,34 +2514,9 @@ static unsigned int collapse_scan_mm_slot(unsigned int pages, enum scan_result * VM_BUG_ON(khugepaged_scan.address < hstart || khugepaged_scan.address + HPAGE_PMD_SIZE > hend); - if (!vma_is_anonymous(vma)) { - struct file *file = get_file(vma->vm_file); - pgoff_t pgoff = linear_page_index(vma, - khugepaged_scan.address); - - mmap_read_unlock(mm); - mmap_locked = false; - *result = collapse_scan_file(mm, - khugepaged_scan.address, file, pgoff, cc); - fput(file); - if (*result == SCAN_PTE_MAPPED_HUGEPAGE) { - mmap_read_lock(mm); - if (collapse_test_exit_or_disable(mm)) - goto breakouterloop; - *result = try_collapse_pte_mapped_thp(mm, - khugepaged_scan.address, false); - if (*result == SCAN_PMD_MAPPED) - *result = SCAN_SUCCEED; - mmap_read_unlock(mm); -
