From: Lance Yang <lance.yang@linux.dev>
As pointed out by Dev, the PTE checks for disjoint conditions in the
scanning loops can be optimized. is_swap_pte, (pte_none && is_zero_pfn),
and pte_uffd_wp are mutually exclusive.
This patch refactors the loops in both __collapse_huge_page_isolate() and
hpage_collapse_scan_pmd() to use a continuous if-else-if-else-if chain
instead of separate if blocks. While at it, the redundant pte_present()
check before is_zero_pfn() is also removed.
Also, this is a preparatory step to make it easier to merge the
almost-duplicated scanning logic in these two functions, as suggested
by David.
Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Reviewed-by: Dev Jain <dev.jain@arm.com>
Reviewed-by: Zi Yan <ziy@nvidia.com>
Suggested-by: Dev Jain <dev.jain@arm.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Lance Yang <lance.yang@linux.dev>
---
mm/khugepaged.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index bec3e268dc76..e3e27223137a 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -548,8 +548,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
_pte++, addr += PAGE_SIZE) {
pte_t pteval = ptep_get(_pte);
- if (pte_none(pteval) || (pte_present(pteval) &&
- is_zero_pfn(pte_pfn(pteval)))) {
+ if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
++none_or_zero;
if (!userfaultfd_armed(vma) &&
(!cc->is_khugepaged ||
@@ -560,12 +559,10 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
goto out;
}
- }
- if (!pte_present(pteval)) {
+ } else if (!pte_present(pteval)) {
result = SCAN_PTE_NON_PRESENT;
goto out;
- }
- if (pte_uffd_wp(pteval)) {
+ } else if (pte_uffd_wp(pteval)) {
result = SCAN_PTE_UFFD_WP;
goto out;
}
@@ -1321,8 +1318,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
count_vm_event(THP_SCAN_EXCEED_SWAP_PTE);
goto out_unmap;
}
- }
- if (pte_uffd_wp(pteval)) {
+ } else if (pte_uffd_wp(pteval)) {
/*
* Don't collapse the page if any of the small
* PTEs are armed with uffd write protection.
--
2.49.0On Wed, Oct 08, 2025 at 12:37:46PM +0800, Lance Yang wrote:
> From: Lance Yang <lance.yang@linux.dev>
>
> As pointed out by Dev, the PTE checks for disjoint conditions in the
> scanning loops can be optimized. is_swap_pte, (pte_none && is_zero_pfn),
> and pte_uffd_wp are mutually exclusive.
But you're not using is_swap_pte anywhere :) This comes back to my review
quesiotn on the series this is dependent upon.
>
> This patch refactors the loops in both __collapse_huge_page_isolate() and
> hpage_collapse_scan_pmd() to use a continuous if-else-if-else-if chain
> instead of separate if blocks. While at it, the redundant pte_present()
> check before is_zero_pfn() is also removed.
I mean see review below, I don't see why you're doing this and I am
unconvinced by how redundant that check is.
Also this just feels like it should be part of the series where you change
these? I'm not sure why this is separate?
>
> Also, this is a preparatory step to make it easier to merge the
> almost-duplicated scanning logic in these two functions, as suggested
> by David.
>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Reviewed-by: Dev Jain <dev.jain@arm.com>
> Reviewed-by: Zi Yan <ziy@nvidia.com>
> Suggested-by: Dev Jain <dev.jain@arm.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> ---
> mm/khugepaged.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index bec3e268dc76..e3e27223137a 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -548,8 +548,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
> _pte++, addr += PAGE_SIZE) {
> pte_t pteval = ptep_get(_pte);
> - if (pte_none(pteval) || (pte_present(pteval) &&
> - is_zero_pfn(pte_pfn(pteval)))) {
> + if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
You can have non-pte_none() non-present entries no? Isn't pte_present() a
prerequisite for pfe_pfn() to be valid? If it's a swap entry couldn't you
end up accidentally (unlikely but still) hitting this?
Seems like this is required isn't it? I may be missing something here...
> ++none_or_zero;
> if (!userfaultfd_armed(vma) &&
> (!cc->is_khugepaged ||
> @@ -560,12 +559,10 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
> goto out;
> }
> - }
> - if (!pte_present(pteval)) {
> + } else if (!pte_present(pteval)) {
This seems pointless, since either the above logic will continue or goto
out right?
> result = SCAN_PTE_NON_PRESENT;
> goto out;
> - }
> - if (pte_uffd_wp(pteval)) {
> + } else if (pte_uffd_wp(pteval)) {
Again, what is the point of an else when the if() branch unconditionally
->out?
> result = SCAN_PTE_UFFD_WP;
> goto out;
> }
> @@ -1321,8 +1318,7 @@ static int hpage_collapse_scan_pmd(struct mm_struct *mm,
> count_vm_event(THP_SCAN_EXCEED_SWAP_PTE);
> goto out_unmap;
> }
> - }
> - if (pte_uffd_wp(pteval)) {
> + } else if (pte_uffd_wp(pteval)) {
Same comment as above, I'm really confused about the purpose of this logic?
> /*
> * Don't collapse the page if any of the small
> * PTEs are armed with uffd write protection.
> --
> 2.49.0
>
On 14.10.25 14:17, Lorenzo Stoakes wrote: > On Wed, Oct 08, 2025 at 12:37:46PM +0800, Lance Yang wrote: >> From: Lance Yang <lance.yang@linux.dev> >> >> As pointed out by Dev, the PTE checks for disjoint conditions in the >> scanning loops can be optimized. is_swap_pte, (pte_none && is_zero_pfn), >> and pte_uffd_wp are mutually exclusive. > > But you're not using is_swap_pte anywhere :) This comes back to my review > quesiotn on the series this is dependent upon. > >> >> This patch refactors the loops in both __collapse_huge_page_isolate() and >> hpage_collapse_scan_pmd() to use a continuous if-else-if-else-if chain >> instead of separate if blocks. While at it, the redundant pte_present() >> check before is_zero_pfn() is also removed. > > I mean see review below, I don't see why you're doing this and I am > unconvinced by how redundant that check is. > > Also this just feels like it should be part of the series where you change > these? I'm not sure why this is separate? I think Lance is trying to unify both scanning functions to look alike, such that when he refactors them out in patch #3 it looks more straight forward. The missing pte_present() check in hpage_collapse_scan_pmd() is interesting Likely there is one such check missing there? -- Cheers David / dhildenb
© 2016 - 2026 Red Hat, Inc.