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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/26/26 10:35, 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:
>>>
>>> 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;
>  }

That looks good to me, thanks.

My memory is bad this week, so I have to ask with the fear of having asked
before: is tracing the PFN of any value?

What can someone tracing this on userspace reasonably do with the PFN? Inspect
kpageflags? I'd assume someone actually doing that is quite ... rare.

-- 
Cheers,

David