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

[Lorenzo Stoakes (ARM)]

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
:)

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.

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