Re: [PATCH v3 1/3] mm: khugepaged: fix swap entry value to folio_pfn()

[Vernon Yang]

On Wed, Aug 26, 2026 at 09:35:38AM +0100, Lorenzo Stoakes (ARM) wrote:
> On Wed, Aug 26, 2026 at 10:24:56AM +0200, David Hildenbrand (Arm) wrote:
> > On 8/26/26 10:16, David Hildenbrand (Arm) wrote:
> > > On 8/26/26 10:11, Lorenzo Stoakes (ARM) wrote:
> > >> On Wed, Aug 26, 2026 at 10:08:58AM +0200, David Hildenbrand (Arm) wrote:
> > >>>
> > >>> Elaborate.
> > >>
> > >> It's overly long, I read it and am confused as to what is 'problematic' or not,
> > >> it reads weirdly in English and pfn_xxx is the usual convention for naming of
> > >> pfn's anyway.
> > >
> > > Excuse me, what? Are you now just making up arguments?
> > To clarify, we have various users of "xxx_pfn" in the tree and I fail to see how
> > "this is a problematic pfn" -> "problematic_pfn" is odd and why
> > "pfn_problematic" would be any clearer.
> >
> > I do agree with the "problematic" aspect. "failed" might indeed be nicer.
>
> Right yeah. Mostly the push back is on the word being a bit confusing. Fair
> enough on the pfn thing, failed_pfn is actually the nicest name suggested so far
> :)

failed_pfn is good to me.

> I still think:
>
>        	if (result == SCAN_SUCCEED) {
>                	...
>                	trace_mm_khugepaged_scan_file(mm, -1, file, present, swap, result);
>        	} else {
>                	trace_mm_khugepaged_scan_file(mm, failed_pfn, file, present,
> 					      swap, result);
>        	}
>
> Is a little neater as then it's only on the failure path that we trace the
> failed pfn, and otherwise we explicitly -1.

I understand what you're trying to say, but personally it isn't necessary,
because failed_pfn defaults to -1, and one
trace_mm_khugepaged_scan_file() already covers it.

If everyone clearly expresses that they want two
trace_mm_khugepaged_scan_file(), please let me know explicitly. Thanks!

> But it's not exactly a show stopper this :)
>
> Very rough edit of your patch - if you're happy then let's go with this, if not
> then edit it + post so Vernon has a clear direction. I'm not feeling super
> strongly on this so don't want to block anything:
>
> ----8<----
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 75639298efc27..371ee0b16d10c 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2683,6 +2683,7 @@ static enum scan_result collapse_scan_file(struct
> mm_struct *mm,
>        	int present, swap;
>        	int node = NUMA_NO_NODE;
>        	enum scan_result result = SCAN_SUCCEED;
> +       unsigned long failed_pfn = -1;
>
>        	present = 0;
>        	swap = 0;
> @@ -2714,6 +2715,7 @@ static enum scan_result collapse_scan_file(struct
> mm_struct *mm,
>                	}
>
>                	if (is_pmd_order(folio_order(folio))) {
> +                       failed_pfn = folio_pfn(folio);
>                        	result = SCAN_PTE_MAPPED_HUGEPAGE;
>                        	/*
>                          * PMD-sized THP implies that we can only try
> @@ -2725,6 +2727,7 @@ static enum scan_result collapse_scan_file(struct
> mm_struct *mm,
>
>                	node = folio_nid(folio);
>                	if (collapse_scan_abort(node, cc)) {
> +                       failed_pfn = folio_pfn(folio);
>                        	result = SCAN_SCAN_ABORT;
>                        	folio_put(folio);
>                        	break;
> @@ -2732,12 +2735,14 @@ static enum scan_result collapse_scan_file(struct
> mm_struct *mm,
>                	cc->node_load[node]++;
>
>                	if (!folio_test_lru(folio)) {
> +                       failed_pfn = folio_pfn(folio);
>                        	result = SCAN_PAGE_LRU;
>                        	folio_put(folio);
>                        	break;
>                	}
>
>                 if (folio_expected_ref_count(folio) + 1 != folio_ref_count(folio)) {
> +                       failed_pfn = folio_pfn(folio);
>                         result = SCAN_PAGE_COUNT;
>                         folio_put(folio);
>                         break;
> @@ -2773,7 +2778,7 @@ static enum scan_result collapse_scan_file(struct
> mm_struct *mm,
>                	}
> -      	}
> -       trace_mm_khugepaged_scan_file(mm, folio, file, present, swap, result);
> +       	trace_mm_khugepaged_scan_file(mm, -1, file, present, swap,
> +				SCAN_SUCCEED);
> +	} else {
> +	       trace_mm_khugepaged_scan_file(mm, failed_pfn, file, present,
> +				swap, result);
> +	}
> +
>         return result;
>  }
>
> --
> Cheers, Lorenzo
>