Re: [PATCH 2/8] mm/khugepaged: extract young page check into collapse_is_young() helper

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 7/15/26 2:04 PM, Nico Pache wrote:
> On Fri, Jul 10, 2026 at 1:41 AM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
>>
>>
>>
>> On 7/6/26 11:44 PM, Nico Pache wrote:
>>> The change deduplicates the "is this PTE young enough to count as
>>> referenced" condition that was repeated in both
>>> __collapse_huge_page_isolate() and collapse_scan_pmd(), extracting it into
>>> a single inline helper function.
>>>
>>> Also move the comment and use it as the function header. While we are at
>>> it, updated the comment to clarify that a young pte is a recently accessed
>>> one.
>>>
>>> Signed-off-by: Nico Pache <npache@redhat.com>
>>> ---
>>>    mm/khugepaged.c | 35 +++++++++++++++++++----------------
>>>    1 file changed, 19 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>> index b3985b854e77..48b008a3c891 100644
>>> --- a/mm/khugepaged.c
>>> +++ b/mm/khugepaged.c
>>> @@ -675,6 +675,23 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
>>>        }
>>>    }
>>>
>>> +/*
>>> + * collapse_is_young() - Check for enough young pte to justify collapsing
>>> + *
>>> + * If collapse was initiated by khugepaged, check that the page has been
>>> + * recently accessed (young pte) to justify collapsing the page.
>>> + *
>>> + * Return: true if the page has been recently accessed (young pte).
>>> + */
>>> +static inline bool collapse_is_young(struct collapse_control *cc, pte_t pteval,
>>> +     struct folio *folio, struct vm_area_struct *vma, unsigned long addr)
>>> +{
>>> +     return cc->is_khugepaged &&
>>> +            (pte_young(pteval) || folio_test_young(folio) ||
>>> +             folio_test_referenced(folio) ||
>>> +             mmu_notifier_test_young(vma->vm_mm, addr));
>>> +}
>>
>> collapse_is_young() is somewhat confusing to me. Would using
>> 'referenced' be a more appropriate name? collapse_folio_is_referenced()?
> 
> I went with collapse_is_referenced since it also checks the PTE not
> just the folio.
> 
>>
>> Also, it feels odd to put 'cc->is_khugepaged' in a helper whose purpose
>> is to check whether a folio has been accessed. I think this helper
>> should be more self-contained and focused solely on checking whether the
>> folio was accessed.
> 
> While I don't wholeheartedly disagree, the point is to clean up the
> code and abstract some of this logic away from the parent functions. I
> prefer doing the khugepaged check inside this new helper because the
> code is much cleaner.

Sure. I don't have a strong opinion, let's see what others perfer :)