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

[Dev Jain <dev.jain@arm.com>]

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


>
> right?
>