Re: [PATCH] mm/khugepaged: fix the address passed to notifier on testing young

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Aug 26, 2025 at 10:53:45AM +0200, David Hildenbrand wrote:
> On 22.08.25 08:33, Wei Yang wrote:
> > Commit 8ee53820edfd ("thp: mmu_notifier_test_young") introduced
> > mmu_notifier_test_young(), but we should pass the address need to test.
>
> ... "but we are passing the wrong address".
>
> > In xxx_scan_pmd(), the actual iteration address is "_address" not
> > "address". We seem to misuse the variable on the very beginning.
> >
> > Change it to the right one.
> >
> > Fixes: 8ee53820edfd ("thp: mmu_notifier_test_young")
> > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > Cc: Zi Yan <ziy@nvidia.com>
> > Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> > Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
> > Cc: Nico Pache <npache@redhat.com>
> > Cc: Ryan Roberts <ryan.roberts@arm.com>
> > Cc: Dev Jain <dev.jain@arm.com>
> > Cc: Barry Song <baohua@kernel.org>
> > CC: <stable@vger.kernel.org>
> >
> > ---
> > The original commit 8ee53820edfd is at 2011.
> > Then the code is moved to khugepaged.c in commit b46e756f5e470 ("thp:
> > extract khugepaged from mm/huge_memory.c") in 2022.
> > ---
> >   mm/khugepaged.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 24e18a7f8a93..b000942250d1 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -1418,7 +1418,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> >   		if (cc->is_khugepaged &&
> >   		    (pte_young(pteval) || folio_test_young(folio) ||
> >   		     folio_test_referenced(folio) || mmu_notifier_test_young(vma->vm_mm,
> > -								     address)))
> > +								     _address)))
>
> Please just put that into a single line, that's a perfectly reasonable case
> to exceed 80 chars.
>
>
> Acked-by: David Hildenbrand <david@redhat.com>
>
> >   			referenced++;
> >   	}
> >   	if (!writable) {
>
> Maybe, just maybe, it's because of *horrible* variable naming.
>
> Can someone please send a cleanup to rename address -> pmd_addr and
> _address -> pte_addr or sth like that?

YES THIS.

>
> pretty much any naming is better than this.

I despise it, and I realyl underlined this on review in Nico's series because
it's just beyond belief.

It's terrible. I mean maybe even I will do something about this, if my review
load eases up at some point...

>
> --
> Cheers
>
> David / dhildenb
>

Cheers, Lorenzo