Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading

[Wei Yang <richard.weiyang@gmail.com>]

On Sat, Sep 20, 2025 at 06:01:59PM +0530, Dev Jain wrote:
>
>On 20/09/25 2:32 pm, Wei Yang wrote:
>> On Sat, Sep 20, 2025 at 10:21:56AM +0530, Dev Jain wrote:
>> > On 20/09/25 6:24 am, Wei Yang wrote:
>> > > When collapse a pmd, there are two address in use:
>> > > 
>> > >     * address points to the start of pmd
>> > >     * address points to each individual page
>> > > 
>> > > Current naming is not easy to distinguish these two and error prone.
>> > > 
>> > > Name the first one to pmd_addr and second one to pte_addr.
>> > > 
>> > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> > > Suggested-by: David Hildenbrand <david@redhat.com>
>> > > ---
>> > >    mm/khugepaged.c | 43 ++++++++++++++++++++++---------------------
>> > >    1 file changed, 22 insertions(+), 21 deletions(-)
>> > > 
>> > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> > > index 4c957ce788d1..6d03072c1a92 100644
>> > > --- a/mm/khugepaged.c
>> > > +++ b/mm/khugepaged.c
>> > > @@ -537,18 +537,19 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
>> > >    }
>> > >    static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>> > > -					unsigned long address,
>> > > +					unsigned long pmd_addr,
>> > >    					pte_t *pte,
>> > >    					struct collapse_control *cc,
>> > >    					struct list_head *compound_pagelist)
>> > >    {
>> > >    	struct page *page = NULL;
>> > >    	struct folio *folio = NULL;
>> > > +	unsigned long pte_addr = pmd_addr;
>> > >    	pte_t *_pte;
>> > >    	int none_or_zero = 0, shared = 0, result = SCAN_FAIL, referenced = 0;
>> > >    	for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
>> > > -	     _pte++, address += PAGE_SIZE) {
>> > > +	     _pte++, pte_addr += PAGE_SIZE) {
>> > >    		pte_t pteval = ptep_get(_pte);
>> > >    		if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
>> > >    			++none_or_zero;
>> > > @@ -570,7 +571,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>> > >    			result = SCAN_PTE_UFFD_WP;
>> > >    			goto out;
>> > >    		}
>> > > -		page = vm_normal_page(vma, address, pteval);
>> > > +		page = vm_normal_page(vma, pte_addr, pteval);
>> > >    		if (unlikely(!page) || unlikely(is_zone_device_page(page))) {
>> > >    			result = SCAN_PAGE_NULL;
>> > >    			goto out;
>> > > @@ -655,8 +656,8 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>> > >    		 */
>> > >    		if (cc->is_khugepaged &&
>> > >    		    (pte_young(pteval) || folio_test_young(folio) ||
>> > > -		     folio_test_referenced(folio) || mmu_notifier_test_young(vma->vm_mm,
>> > > -								     address)))
>> > > +		     folio_test_referenced(folio) ||
>> > > +		     mmu_notifier_test_young(vma->vm_mm, pte_addr)))
>> > >    			referenced++;
>> > >    	}
>> > > @@ -985,21 +986,21 @@ static int check_pmd_still_valid(struct mm_struct *mm,
>> > >     */
>> > >    static int __collapse_huge_page_swapin(struct mm_struct *mm,
>> > >    				       struct vm_area_struct *vma,
>> > > -				       unsigned long haddr, pmd_t *pmd,
>> > > +				       unsigned long pmd_addr, pmd_t *pmd,
>> > >    				       int referenced)
>> > Will this be a problem when mTHP collapse is in? You may have the starting
>> > address lying in the PTE table.
>> > 
>> > Personally "haddr" is pretty clear to me - I read it as "huge-aligned addr".
>> > I will vote for naming the starting addresses everywhere as "haddr", and use
>> > addr as the loop iterator. This is a short name and haddr will imply that it
>> > is aligned to the huge order we are collapsing for.
>> > 
>> So your suggestion is
>> 
>>      pmd_addr -> haddr
>>      pte_addr -> address
>
>pmd_addr -> haddr
>pte_addr -> addr
>

Take another look into the code.

The change touch three functions:

  __collapse_huge_page_isolate()
  __collapse_huge_page_swapin()
  hpage_collapse_scan_pmd()

Use pmd_addr/pte_addr look reasonable for hpage_collapse_scan_pmd().
And haddr/addr would be suitable for the other two.

Is this ok for you?

-- 
Wei Yang
Help you, Help me