The khugepaged daemon and madvise_collapse have two different
implementations that do almost the same thing.

Create khugepaged_collapse_single_pmd to increase code reuse and create an
entry point these two users.

Refactor madvise_collapse and khugepaged_scan_mm_slot to use the new
khugepaged_collapse_single_pmd function. This introduces a minor
behavioral change that is most likely an undiscovered bug. The current
implementation of khugepaged tests khugepaged_test_exit_or_disable
before calling khugepaged_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.

Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Signed-off-by: Nico Pache <npache@redhat.com>
---
 mm/khugepaged.c | 95 +++++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 46 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index f27fe7ca9b86..bf69e81a3d82 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2354,6 +2354,50 @@ static int khugepaged_scan_file(struct mm_struct *mm, unsigned long addr,
 	return result;
 }
 
+/*
+ * Try to collapse a single PMD starting at a PMD aligned addr, and return
+ * the results.
+ */
+static int khugepaged_collapse_single_pmd(unsigned long addr,
+				   struct vm_area_struct *vma, bool *mmap_locked,
+				   struct collapse_control *cc)
+{
+	int result = SCAN_FAIL;
+	struct mm_struct *mm = vma->vm_mm;
+
+	if (IS_ENABLED(CONFIG_SHMEM) && !vma_is_anonymous(vma)) {
+		struct file *file = get_file(vma->vm_file);
+		pgoff_t pgoff = linear_page_index(vma, addr);
+
+		mmap_read_unlock(mm);
+		*mmap_locked = false;
+		result = khugepaged_scan_file(mm, addr, file, pgoff, cc);
+		fput(file);
+		if (result == SCAN_PTE_MAPPED_HUGEPAGE) {
+			mmap_read_lock(mm);
+			*mmap_locked = true;
+			if (khugepaged_test_exit_or_disable(mm)) {
+				mmap_read_unlock(mm);
+				*mmap_locked = false;
+				result = SCAN_ANY_PROCESS;
+				goto end;
+			}
+			result = collapse_pte_mapped_thp(mm, addr,
+							 !cc->is_khugepaged);
+			if (result == SCAN_PMD_MAPPED)
+				result = SCAN_SUCCEED;
+			mmap_read_unlock(mm);
+			*mmap_locked = false;
+		}
+	} else {
+		result = khugepaged_scan_pmd(mm, vma, addr, mmap_locked, cc);
+	}
+	if (cc->is_khugepaged && result == SCAN_SUCCEED)
+		++khugepaged_pages_collapsed;
+end:
+	return result;
+}
+
 static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
 					    struct collapse_control *cc)
 	__releases(&khugepaged_mm_lock)
@@ -2428,34 +2472,9 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *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 = hpage_collapse_scan_file(mm,
-					khugepaged_scan.address, file, pgoff, cc);
-				fput(file);
-				if (*result == SCAN_PTE_MAPPED_HUGEPAGE) {
-					mmap_read_lock(mm);
-					if (hpage_collapse_test_exit_or_disable(mm))
-						goto breakouterloop;
-					*result = collapse_pte_mapped_thp(mm,
-						khugepaged_scan.address, false);
-					if (*result == SCAN_PMD_MAPPED)
-						*result = SCAN_SUCCEED;
-					mmap_read_unlock(mm);
-				}
-			} else {
-				*result = hpage_collapse_scan_pmd(mm, vma,
-					khugepaged_scan.address, &mmap_locked, cc);
-			}
-
-			if (*result == SCAN_SUCCEED)
-				++khugepaged_pages_collapsed;
 
+			*result = khugepaged_collapse_single_pmd(khugepaged_scan.address,
+						vma, &mmap_locked, cc);
 			/* move to next address */
 			khugepaged_scan.address += HPAGE_PMD_SIZE;
 			progress += HPAGE_PMD_NR;
@@ -2772,35 +2791,19 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
 		mmap_assert_locked(mm);
 		memset(cc->node_load, 0, sizeof(cc->node_load));
 		nodes_clear(cc->alloc_nmask);
-		if (!vma_is_anonymous(vma)) {
-			struct file *file = get_file(vma->vm_file);
-			pgoff_t pgoff = linear_page_index(vma, addr);
 
-			mmap_read_unlock(mm);
-			mmap_locked = false;
-			result = hpage_collapse_scan_file(mm, addr, file, pgoff,
-							  cc);
-			fput(file);
-		} else {
-			result = hpage_collapse_scan_pmd(mm, vma, addr,
-							 &mmap_locked, cc);
-		}
+		result = khugepaged_collapse_single_pmd(addr, vma, &mmap_locked, cc);
+
 		if (!mmap_locked)
 			*lock_dropped = true;
 
-handle_result:
 		switch (result) {
 		case SCAN_SUCCEED:
 		case SCAN_PMD_MAPPED:
 			++thps;
 			break;
-		case SCAN_PTE_MAPPED_HUGEPAGE:
-			BUG_ON(mmap_locked);
-			mmap_read_lock(mm);
-			result = collapse_pte_mapped_thp(mm, addr, true);
-			mmap_read_unlock(mm);
-			goto handle_result;
 		/* Whitelisted set of results where continuing OK */
+		case SCAN_PTE_MAPPED_HUGEPAGE:
 		case SCAN_PMD_NULL:
 		case SCAN_PTE_NON_PRESENT:
 		case SCAN_PTE_UFFD_WP:
-- 
2.49.0

On 2025/7/2 13:57, Nico Pache wrote:
> The khugepaged daemon and madvise_collapse have two different
> implementations that do almost the same thing.
> 
> Create khugepaged_collapse_single_pmd to increase code reuse and create an
> entry point these two users.
> 
> Refactor madvise_collapse and khugepaged_scan_mm_slot to use the new
> khugepaged_collapse_single_pmd function. This introduces a minor
> behavioral change that is most likely an undiscovered bug. The current
> implementation of khugepaged tests khugepaged_test_exit_or_disable
> before calling khugepaged_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.
> 
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Signed-off-by: Nico Pache <npache@redhat.com>
> ---
>   mm/khugepaged.c | 95 +++++++++++++++++++++++++------------------------
>   1 file changed, 49 insertions(+), 46 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index f27fe7ca9b86..bf69e81a3d82 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2354,6 +2354,50 @@ static int khugepaged_scan_file(struct mm_struct *mm, unsigned long addr,
>   	return result;
>   }
>   
> +/*
> + * Try to collapse a single PMD starting at a PMD aligned addr, and return
> + * the results.
> + */
> +static int khugepaged_collapse_single_pmd(unsigned long addr,
> +				   struct vm_area_struct *vma, bool *mmap_locked,
> +				   struct collapse_control *cc)
> +{
> +	int result = SCAN_FAIL;
> +	struct mm_struct *mm = vma->vm_mm;
> +
> +	if (IS_ENABLED(CONFIG_SHMEM) && !vma_is_anonymous(vma)) {

Seems you still forgot to drop 'IS_ENABLED(CONFIG_SHMEM)' :)

On Thu, Jul 3, 2025 at 9:51 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 2025/7/2 13:57, Nico Pache wrote:
> > The khugepaged daemon and madvise_collapse have two different
> > implementations that do almost the same thing.
> >
> > Create khugepaged_collapse_single_pmd to increase code reuse and create an
> > entry point these two users.
> >
> > Refactor madvise_collapse and khugepaged_scan_mm_slot to use the new
> > khugepaged_collapse_single_pmd function. This introduces a minor
> > behavioral change that is most likely an undiscovered bug. The current
> > implementation of khugepaged tests khugepaged_test_exit_or_disable
> > before calling khugepaged_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.
> >
> > Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> > Signed-off-by: Nico Pache <npache@redhat.com>
> > ---
> >   mm/khugepaged.c | 95 +++++++++++++++++++++++++------------------------
> >   1 file changed, 49 insertions(+), 46 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index f27fe7ca9b86..bf69e81a3d82 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -2354,6 +2354,50 @@ static int khugepaged_scan_file(struct mm_struct *mm, unsigned long addr,
> >       return result;
> >   }
> >
> > +/*
> > + * Try to collapse a single PMD starting at a PMD aligned addr, and return
> > + * the results.
> > + */
> > +static int khugepaged_collapse_single_pmd(unsigned long addr,
> > +                                struct vm_area_struct *vma, bool *mmap_locked,
> > +                                struct collapse_control *cc)
> > +{
> > +     int result = SCAN_FAIL;
> > +     struct mm_struct *mm = vma->vm_mm;
> > +
> > +     if (IS_ENABLED(CONFIG_SHMEM) && !vma_is_anonymous(vma)) {
>
> Seems you still forgot to drop 'IS_ENABLED(CONFIG_SHMEM)' :)
Hi Baolin,

You had me questioning my sanity for a moment because I SWEAR I did do
this! but now I see the issue, I accidentally merged the fixup into
patch 6 (not entirely sure how or why I would have done that given my
notes say it belongs in this patch...)

I'm sorry about that :< I'll see if I can work with Andrew to get some
fixups into mm-new (this was merged a few hours ago) for this patch
and patch 6.
>

On 2025/7/4 12:20, Nico Pache wrote:
> On Thu, Jul 3, 2025 at 9:51 PM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
>>
>>
>>
>> On 2025/7/2 13:57, Nico Pache wrote:
>>> The khugepaged daemon and madvise_collapse have two different
>>> implementations that do almost the same thing.
>>>
>>> Create khugepaged_collapse_single_pmd to increase code reuse and create an
>>> entry point these two users.
>>>
>>> Refactor madvise_collapse and khugepaged_scan_mm_slot to use the new
>>> khugepaged_collapse_single_pmd function. This introduces a minor
>>> behavioral change that is most likely an undiscovered bug. The current
>>> implementation of khugepaged tests khugepaged_test_exit_or_disable
>>> before calling khugepaged_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.
>>>
>>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>> Signed-off-by: Nico Pache <npache@redhat.com>
>>> ---
>>>    mm/khugepaged.c | 95 +++++++++++++++++++++++++------------------------
>>>    1 file changed, 49 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>> index f27fe7ca9b86..bf69e81a3d82 100644
>>> --- a/mm/khugepaged.c
>>> +++ b/mm/khugepaged.c
>>> @@ -2354,6 +2354,50 @@ static int khugepaged_scan_file(struct mm_struct *mm, unsigned long addr,
>>>        return result;
>>>    }
>>>
>>> +/*
>>> + * Try to collapse a single PMD starting at a PMD aligned addr, and return
>>> + * the results.
>>> + */
>>> +static int khugepaged_collapse_single_pmd(unsigned long addr,
>>> +                                struct vm_area_struct *vma, bool *mmap_locked,
>>> +                                struct collapse_control *cc)
>>> +{
>>> +     int result = SCAN_FAIL;
>>> +     struct mm_struct *mm = vma->vm_mm;
>>> +
>>> +     if (IS_ENABLED(CONFIG_SHMEM) && !vma_is_anonymous(vma)) {
>>
>> Seems you still forgot to drop 'IS_ENABLED(CONFIG_SHMEM)' :)
> Hi Baolin,
> 
> You had me questioning my sanity for a moment because I SWEAR I did do
> this! but now I see the issue, I accidentally merged the fixup into
> patch 6 (not entirely sure how or why I would have done that given my
> notes say it belongs in this patch...)

Ah, yes.

> I'm sorry about that :< I'll see if I can work with Andrew to get some
> fixups into mm-new (this was merged a few hours ago) for this patch
> and patch 6.

OK. Thanks.