* [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
@ 2025-09-20 0:54 Wei Yang
2025-09-20 4:30 ` Lance Yang
2025-09-20 4:51 ` Dev Jain
0 siblings, 2 replies; 13+ messages in thread
From: Wei Yang @ 2025-09-20 0:54 UTC (permalink / raw)
To: akpm, david, lorenzo.stoakes, ziy, baolin.wang, Liam.Howlett,
npache, ryan.roberts, dev.jain, baohua, lance.yang
Cc: linux-mm, Wei Yang
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)
{
int swapped_in = 0;
vm_fault_t ret = 0;
- unsigned long address, end = haddr + (HPAGE_PMD_NR * PAGE_SIZE);
+ unsigned long pte_addr, end = pmd_addr + (HPAGE_PMD_NR * PAGE_SIZE);
int result;
pte_t *pte = NULL;
spinlock_t *ptl;
- for (address = haddr; address < end; address += PAGE_SIZE) {
+ for (pte_addr = pmd_addr; pte_addr < end; pte_addr += PAGE_SIZE) {
struct vm_fault vmf = {
.vma = vma,
- .address = address,
- .pgoff = linear_page_index(vma, address),
+ .address = pte_addr,
+ .pgoff = linear_page_index(vma, pte_addr),
.flags = FAULT_FLAG_ALLOW_RETRY,
.pmd = pmd,
};
@@ -1009,7 +1010,7 @@ static int __collapse_huge_page_swapin(struct mm_struct *mm,
* Here the ptl is only used to check pte_same() in
* do_swap_page(), so readonly version is enough.
*/
- pte = pte_offset_map_ro_nolock(mm, pmd, address, &ptl);
+ pte = pte_offset_map_ro_nolock(mm, pmd, pte_addr, &ptl);
if (!pte) {
mmap_read_unlock(mm);
result = SCAN_PMD_NULL;
@@ -1252,7 +1253,7 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address,
static int hpage_collapse_scan_pmd(struct mm_struct *mm,
struct vm_area_struct *vma,
- unsigned long address, bool *mmap_locked,
+ unsigned long pmd_addr, bool *mmap_locked,
struct collapse_control *cc)
{
pmd_t *pmd;
@@ -1261,26 +1262,26 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
int none_or_zero = 0, shared = 0;
struct page *page = NULL;
struct folio *folio = NULL;
- unsigned long _address;
+ unsigned long pte_addr;
spinlock_t *ptl;
int node = NUMA_NO_NODE, unmapped = 0;
- VM_BUG_ON(address & ~HPAGE_PMD_MASK);
+ VM_BUG_ON(pmd_addr & ~HPAGE_PMD_MASK);
- result = find_pmd_or_thp_or_none(mm, address, &pmd);
+ result = find_pmd_or_thp_or_none(mm, pmd_addr, &pmd);
if (result != SCAN_SUCCEED)
goto out;
memset(cc->node_load, 0, sizeof(cc->node_load));
nodes_clear(cc->alloc_nmask);
- pte = pte_offset_map_lock(mm, pmd, address, &ptl);
+ pte = pte_offset_map_lock(mm, pmd, pmd_addr, &ptl);
if (!pte) {
result = SCAN_PMD_NULL;
goto out;
}
- for (_address = address, _pte = pte; _pte < pte + HPAGE_PMD_NR;
- _pte++, _address += PAGE_SIZE) {
+ for (pte_addr = pmd_addr, _pte = pte; _pte < pte + HPAGE_PMD_NR;
+ _pte++, pte_addr += PAGE_SIZE) {
pte_t pteval = ptep_get(_pte);
if (is_swap_pte(pteval)) {
++unmapped;
@@ -1328,7 +1329,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
goto out_unmap;
}
- 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_unmap;
@@ -1397,7 +1398,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)))
+ mmu_notifier_test_young(vma->vm_mm, pte_addr)))
referenced++;
}
if (cc->is_khugepaged &&
@@ -1410,7 +1411,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
out_unmap:
pte_unmap_unlock(pte, ptl);
if (result == SCAN_SUCCEED) {
- result = collapse_huge_page(mm, address, referenced,
+ result = collapse_huge_page(mm, pmd_addr, referenced,
unmapped, cc);
/* collapse_huge_page will return with the mmap_lock released */
*mmap_locked = false;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-20 0:54 [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading Wei Yang
@ 2025-09-20 4:30 ` Lance Yang
2025-09-20 9:00 ` Wei Yang
2025-09-20 4:51 ` Dev Jain
1 sibling, 1 reply; 13+ messages in thread
From: Lance Yang @ 2025-09-20 4:30 UTC (permalink / raw)
To: Wei Yang
Cc: linux-mm, lorenzo.stoakes, baohua, ryan.roberts, Liam.Howlett,
dev.jain, david, npache, ziy, akpm, baolin.wang
On 2025/9/20 08:54, 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>
This renaming makes the code much easier to follow, but just
some minor style nits below :)
Acked-by: Lance Yang <lance.yang@linux.dev>
> ---
> 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;
Nit: could we refactor this block into the "reverse christmas tree" style?
>
> 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)
> {
> int swapped_in = 0;
> vm_fault_t ret = 0;
> - unsigned long address, end = haddr + (HPAGE_PMD_NR * PAGE_SIZE);
> + unsigned long pte_addr, end = pmd_addr + (HPAGE_PMD_NR * PAGE_SIZE);
> int result;
> pte_t *pte = NULL;
> spinlock_t *ptl;
Same nit as above.
>
> - for (address = haddr; address < end; address += PAGE_SIZE) {
> + for (pte_addr = pmd_addr; pte_addr < end; pte_addr += PAGE_SIZE) {
> struct vm_fault vmf = {
> .vma = vma,
> - .address = address,
> - .pgoff = linear_page_index(vma, address),
> + .address = pte_addr,
> + .pgoff = linear_page_index(vma, pte_addr),
> .flags = FAULT_FLAG_ALLOW_RETRY,
> .pmd = pmd,
> };
> @@ -1009,7 +1010,7 @@ static int __collapse_huge_page_swapin(struct mm_struct *mm,
> * Here the ptl is only used to check pte_same() in
> * do_swap_page(), so readonly version is enough.
> */
> - pte = pte_offset_map_ro_nolock(mm, pmd, address, &ptl);
> + pte = pte_offset_map_ro_nolock(mm, pmd, pte_addr, &ptl);
> if (!pte) {
> mmap_read_unlock(mm);
> result = SCAN_PMD_NULL;
> @@ -1252,7 +1253,7 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address,
>
> static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> struct vm_area_struct *vma,
> - unsigned long address, bool *mmap_locked,
> + unsigned long pmd_addr, bool *mmap_locked,
> struct collapse_control *cc)
> {
> pmd_t *pmd;
> @@ -1261,26 +1262,26 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> int none_or_zero = 0, shared = 0;
> struct page *page = NULL;
> struct folio *folio = NULL;
> - unsigned long _address;
> + unsigned long pte_addr;
> spinlock_t *ptl;
> int node = NUMA_NO_NODE, unmapped = 0;
Ditto.
>
> - VM_BUG_ON(address & ~HPAGE_PMD_MASK);
> + VM_BUG_ON(pmd_addr & ~HPAGE_PMD_MASK);
>
> - result = find_pmd_or_thp_or_none(mm, address, &pmd);
> + result = find_pmd_or_thp_or_none(mm, pmd_addr, &pmd);
> if (result != SCAN_SUCCEED)
> goto out;
>
> memset(cc->node_load, 0, sizeof(cc->node_load));
> nodes_clear(cc->alloc_nmask);
> - pte = pte_offset_map_lock(mm, pmd, address, &ptl);
> + pte = pte_offset_map_lock(mm, pmd, pmd_addr, &ptl);
> if (!pte) {
> result = SCAN_PMD_NULL;
> goto out;
> }
>
> - for (_address = address, _pte = pte; _pte < pte + HPAGE_PMD_NR;
> - _pte++, _address += PAGE_SIZE) {
> + for (pte_addr = pmd_addr, _pte = pte; _pte < pte + HPAGE_PMD_NR;
> + _pte++, pte_addr += PAGE_SIZE) {
> pte_t pteval = ptep_get(_pte);
> if (is_swap_pte(pteval)) {
> ++unmapped;
> @@ -1328,7 +1329,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> goto out_unmap;
> }
>
> - 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_unmap;
> @@ -1397,7 +1398,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)))
> + mmu_notifier_test_young(vma->vm_mm, pte_addr)))
> referenced++;
> }
> if (cc->is_khugepaged &&
> @@ -1410,7 +1411,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> out_unmap:
> pte_unmap_unlock(pte, ptl);
> if (result == SCAN_SUCCEED) {
> - result = collapse_huge_page(mm, address, referenced,
> + result = collapse_huge_page(mm, pmd_addr, referenced,
> unmapped, cc);
> /* collapse_huge_page will return with the mmap_lock released */
> *mmap_locked = false;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-20 0:54 [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading Wei Yang
2025-09-20 4:30 ` Lance Yang
@ 2025-09-20 4:51 ` Dev Jain
2025-09-20 9:02 ` Wei Yang
1 sibling, 1 reply; 13+ messages in thread
From: Dev Jain @ 2025-09-20 4:51 UTC (permalink / raw)
To: Wei Yang, akpm, david, lorenzo.stoakes, ziy, baolin.wang,
Liam.Howlett, npache, ryan.roberts, baohua, lance.yang
Cc: linux-mm
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.
> if (!pte) {
> mmap_read_unlock(mm);
> result = SCAN_PMD_NULL;
> @@ -1252,7 +1253,7 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address,
>
> static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> struct vm_area_struct *vma,
> - unsigned long address, bool *mmap_locked,
> + unsigned long pmd_addr, bool *mmap_locked,
> struct collapse_control *cc)
> {
> pmd_t *pmd;
> @@ -1261,26 +1262,26 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> int none_or_zero = 0, shared = 0;
> struct page *page = NULL;
> struct folio *folio = NULL;
> - unsigned long _address;
> + unsigned long pte_addr;
Here we can change the address parameter of hpage_collapse_scan_pmd to
haddr and _address to addr. And so on.
> spinlock_t *ptl;
> int node = NUMA_NO_NODE, unmapped = 0;
>
> - VM_BUG_ON(address & ~HPAGE_PMD_MASK);
> + VM_BUG_ON(pmd_addr & ~HPAGE_PMD_MASK);
>
> - result = find_pmd_or_thp_or_none(mm, address, &pmd);
> + result = find_pmd_or_thp_or_none(mm, pmd_addr, &pmd);
> if (result != SCAN_SUCCEED)
> goto out;
>
> memset(cc->node_load, 0, sizeof(cc->node_load));
> nodes_clear(cc->alloc_nmask);
> - pte = pte_offset_map_lock(mm, pmd, address, &ptl);
> + pte = pte_offset_map_lock(mm, pmd, pmd_addr, &ptl);
> if (!pte) {
> result = SCAN_PMD_NULL;
> goto out;
> }
>
> - for (_address = address, _pte = pte; _pte < pte + HPAGE_PMD_NR;
> - _pte++, _address += PAGE_SIZE) {
> + for (pte_addr = pmd_addr, _pte = pte; _pte < pte + HPAGE_PMD_NR;
> + _pte++, pte_addr += PAGE_SIZE) {
> pte_t pteval = ptep_get(_pte);
> if (is_swap_pte(pteval)) {
> ++unmapped;
> @@ -1328,7 +1329,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> goto out_unmap;
> }
>
> - 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_unmap;
> @@ -1397,7 +1398,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)))
> + mmu_notifier_test_young(vma->vm_mm, pte_addr)))
> referenced++;
> }
> if (cc->is_khugepaged &&
> @@ -1410,7 +1411,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> out_unmap:
> pte_unmap_unlock(pte, ptl);
> if (result == SCAN_SUCCEED) {
> - result = collapse_huge_page(mm, address, referenced,
> + result = collapse_huge_page(mm, pmd_addr, referenced,
> unmapped, cc);
> /* collapse_huge_page will return with the mmap_lock released */
> *mmap_locked = false;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-20 4:30 ` Lance Yang
@ 2025-09-20 9:00 ` Wei Yang
2025-09-20 12:42 ` Lance Yang
0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2025-09-20 9:00 UTC (permalink / raw)
To: Lance Yang
Cc: Wei Yang, linux-mm, lorenzo.stoakes, baohua, ryan.roberts,
Liam.Howlett, dev.jain, david, npache, ziy, akpm, baolin.wang
On Sat, Sep 20, 2025 at 12:30:52PM +0800, Lance Yang wrote:
>
>On 2025/9/20 08:54, 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>
>
>This renaming makes the code much easier to follow, but just
>some minor style nits below :)
>
>Acked-by: Lance Yang <lance.yang@linux.dev>
>
>> ---
>> 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;
>
>Nit: could we refactor this block into the "reverse christmas tree" style?
>
You mean sth like this?
int none_or_zero = 0, shared = 0, result = SCAN_FAIL, referenced = 0;
struct page *page = NULL;
struct folio *folio = NULL;
unsigned long pte_addr = pmd_addr;
pte_t *_pte;
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-20 4:51 ` Dev Jain
@ 2025-09-20 9:02 ` Wei Yang
2025-09-20 12:31 ` Dev Jain
0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2025-09-20 9:02 UTC (permalink / raw)
To: Dev Jain
Cc: Wei Yang, akpm, david, lorenzo.stoakes, ziy, baolin.wang,
Liam.Howlett, npache, ryan.roberts, baohua, lance.yang, linux-mm
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
right?
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-20 9:02 ` Wei Yang
@ 2025-09-20 12:31 ` Dev Jain
2025-09-22 8:12 ` Wei Yang
0 siblings, 1 reply; 13+ messages in thread
From: Dev Jain @ 2025-09-20 12:31 UTC (permalink / raw)
To: Wei Yang
Cc: akpm, david, lorenzo.stoakes, ziy, baolin.wang, Liam.Howlett,
npache, ryan.roberts, baohua, lance.yang, linux-mm
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?
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-20 9:00 ` Wei Yang
@ 2025-09-20 12:42 ` Lance Yang
0 siblings, 0 replies; 13+ messages in thread
From: Lance Yang @ 2025-09-20 12:42 UTC (permalink / raw)
To: Wei Yang
Cc: linux-mm, lorenzo.stoakes, baohua, ryan.roberts, Liam.Howlett,
dev.jain, david, npache, ziy, akpm, baolin.wang
On 2025/9/20 17:00, Wei Yang wrote:
> On Sat, Sep 20, 2025 at 12:30:52PM +0800, Lance Yang wrote:
>>
>> On 2025/9/20 08:54, 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>
>>
>> This renaming makes the code much easier to follow, but just
>> some minor style nits below :)
>>
>> Acked-by: Lance Yang <lance.yang@linux.dev>
>>
>>> ---
>>> 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;
>>
>> Nit: could we refactor this block into the "reverse christmas tree" style?
>>
>
> You mean sth like this?
>
> int none_or_zero = 0, shared = 0, result = SCAN_FAIL, referenced = 0;
> struct page *page = NULL;
> struct folio *folio = NULL;
> unsigned long pte_addr = pmd_addr;
> pte_t *_pte;
That's getting closer. The formatting would be something like this:
int none_or_zero = 0, shared = 0, result = SCAN_FAIL, referenced = 0;
unsigned long pte_addr = pmd_addr;
struct folio *folio = NULL;
struct page *page = NULL;
pte_t *_pte;
Feel free to change it :)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-20 12:31 ` Dev Jain
@ 2025-09-22 8:12 ` Wei Yang
2025-09-22 8:16 ` David Hildenbrand
0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2025-09-22 8:12 UTC (permalink / raw)
To: Dev Jain
Cc: Wei Yang, akpm, david, lorenzo.stoakes, ziy, baolin.wang,
Liam.Howlett, npache, ryan.roberts, baohua, lance.yang, linux-mm
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-22 8:12 ` Wei Yang
@ 2025-09-22 8:16 ` David Hildenbrand
2025-09-22 8:26 ` Dev Jain
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2025-09-22 8:16 UTC (permalink / raw)
To: Wei Yang, Dev Jain
Cc: akpm, lorenzo.stoakes, ziy, baolin.wang, Liam.Howlett, npache,
ryan.roberts, baohua, lance.yang, linux-mm
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?
--
Cheers
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-22 8:16 ` David Hildenbrand
@ 2025-09-22 8:26 ` Dev Jain
2025-09-22 13:02 ` Wei Yang
0 siblings, 1 reply; 13+ messages in thread
From: Dev Jain @ 2025-09-22 8:26 UTC (permalink / raw)
To: David Hildenbrand, Wei Yang
Cc: akpm, lorenzo.stoakes, ziy, baolin.wang, Liam.Howlett, npache,
ryan.roberts, baohua, lance.yang, linux-mm
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-22 8:26 ` Dev Jain
@ 2025-09-22 13:02 ` Wei Yang
2025-09-22 13:28 ` David Hildenbrand
0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2025-09-22 13:02 UTC (permalink / raw)
To: Dev Jain
Cc: David Hildenbrand, Wei Yang, akpm, lorenzo.stoakes, ziy,
baolin.wang, Liam.Howlett, npache, ryan.roberts, baohua,
lance.yang, linux-mm
On Mon, Sep 22, 2025 at 01:56:54PM +0530, Dev Jain wrote:
[...]
>> > >
>> >
>> > 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.
>
Including hpage_collapse_scan_pmd()?
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-22 13:02 ` Wei Yang
@ 2025-09-22 13:28 ` David Hildenbrand
2025-09-22 13:32 ` Wei Yang
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2025-09-22 13:28 UTC (permalink / raw)
To: Wei Yang, Dev Jain
Cc: akpm, lorenzo.stoakes, ziy, baolin.wang, Liam.Howlett, npache,
ryan.roberts, baohua, lance.yang, linux-mm
On 22.09.25 15:02, Wei Yang wrote:
> On Mon, Sep 22, 2025 at 01:56:54PM +0530, Dev Jain wrote:
> [...]
>>>>>
>>>>
>>>> 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.
>>
>
> Including hpage_collapse_scan_pmd()?
Yes. Anything is better than what we have right now :)
If we just want to express "this is the start of the new THP", simply
"start_addr" might also do?
--
Cheers
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading
2025-09-22 13:28 ` David Hildenbrand
@ 2025-09-22 13:32 ` Wei Yang
0 siblings, 0 replies; 13+ messages in thread
From: Wei Yang @ 2025-09-22 13:32 UTC (permalink / raw)
To: David Hildenbrand
Cc: Wei Yang, Dev Jain, akpm, lorenzo.stoakes, ziy, baolin.wang,
Liam.Howlett, npache, ryan.roberts, baohua, lance.yang, linux-mm
On Mon, Sep 22, 2025 at 03:28:32PM +0200, David Hildenbrand wrote:
>On 22.09.25 15:02, Wei Yang wrote:
>> On Mon, Sep 22, 2025 at 01:56:54PM +0530, Dev Jain wrote:
>> [...]
>> > > > >
>> > > >
>> > > > 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.
>> >
>>
>> Including hpage_collapse_scan_pmd()?
>
>Yes. Anything is better than what we have right now :)
>
>If we just want to express "this is the start of the new THP", simply
>"start_addr" might also do?
>
OK, start_addr/addr looks good to me.
Thanks
>--
>Cheers
>
>David / dhildenb
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-09-22 13:32 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-09-20 0:54 [PATCH] mm/khugepaged: use [pmd|pte]_addr for better reading Wei Yang
2025-09-20 4:30 ` Lance Yang
2025-09-20 9:00 ` Wei Yang
2025-09-20 12:42 ` Lance Yang
2025-09-20 4:51 ` Dev Jain
2025-09-20 9:02 ` Wei Yang
2025-09-20 12:31 ` Dev Jain
2025-09-22 8:12 ` Wei Yang
2025-09-22 8:16 ` David Hildenbrand
2025-09-22 8:26 ` Dev Jain
2025-09-22 13:02 ` Wei Yang
2025-09-22 13:28 ` David Hildenbrand
2025-09-22 13:32 ` Wei Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox