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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Wed, Aug 26, 2026 at 10:08:58AM +0200, David Hildenbrand (Arm) wrote:
> On 8/26/26 10:07, Lorenzo Stoakes (ARM) wrote:
> > On Wed, Aug 26, 2026 at 09:57:05AM +0200, David Hildenbrand (Arm) wrote:
> >> On 8/26/26 04:44, Vernon Yang wrote:
> >>>
> >>> When the `xas_for_each()` iteration to terminate and the folio operation
> >>> preceding is normal, but pfn will be incorrect.
> >>
> >> The PFN is only relevant when a folio participated in the failure. Maybe the
> >> following would be cleanest?
> >>
> >> 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 problematic_pfn = -1;
> >
> > I find this name... problematic :)
>
> 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.

I made a suggestion in the reply as to how to refactor this to avoid the need
for assigning the name like that anyway, if you branch the result ==
SCAN_SUCCEED then it's implied.

I did wonder about pfn_folio but then that's confusing wrt the function, maybe
pfn_fail if you really want the name not to be pfn.

>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo