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

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

On 22/09/25 1:46 pm, David Hildenbrand wrote:
> On 22.09.25 10:12, Wei Yang wrote:
>> 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.
>
> haddr vs. addr is just nasty.
>
> Can we call the aligned one "aligned_addr" or something like that?

This works. Let us use aligned_addr/addr everywhere then.