On Tue, Mar 8, 2022 at 1:35 PM Zach O'Keefe <zokeefe@xxxxxxxxxx> wrote:
>
> When scanning an anon pmd to see if it's eligible for collapse, return
> SCAN_PAGE_COMPOUND if the pmd already maps a thp. This is consistent
> with handling when scanning file-backed memory.
I'm not quite keen that we have to keep anon consistent with file for
this case. SCAN_PAGE_COMPOUND typically means the page is compound
page, but PTE mapped.
And even SCAN_PMD_NULL is not returned every time when mm_find_pmd()
returns NULL. In addition, SCAN_PMD_NULL seems ambiguous to me. The
khugepaged actually sees non-present (migration) entry or trans huge
entry, so may rename it to SCAN_PMD_NOT_SUITABLE?
>
> Signed-off-by: Zach O'Keefe <zokeefe@xxxxxxxxxx>
> ---
> mm/khugepaged.c | 41 +++++++++++++++++++++++++++++++++++------
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index ecbd3fc41c80..403578161a3b 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1011,6 +1011,38 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
> vm_flags_ignore, vmap);
> }
>
> +/*
> + * If returning NULL (meaning the pmd isn't mapped, isn't present, or thp),
> + * write the reason to *result.
> + */
> +static pmd_t *find_pmd_or_thp_or_none(struct mm_struct *mm,
> + unsigned long address,
> + int *result)
> +{
> + pmd_t *pmd = mm_find_pmd_raw(mm, address);
> + pmd_t pmde;
> +
> + if (!pmd) {
> + *result = SCAN_PMD_NULL;
> + return NULL;
> + }
> +
> + pmde = pmd_read_atomic(pmd);
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> + /* See comments in pmd_none_or_trans_huge_or_clear_bad() */
> + barrier();
> +#endif
> + if (!pmd_present(pmde) || !pmd_none(pmde)) {
> + *result = SCAN_PMD_NULL;
> + return NULL;
> + } else if (pmd_trans_huge(pmde)) {
> + *result = SCAN_PAGE_COMPOUND;
> + return NULL;
> + }
> + return pmd;
> +}
> +
> /*
> * Bring missing pages in from swap, to complete THP collapse.
> * Only done if khugepaged_scan_pmd believes it is worthwhile.
> @@ -1212,9 +1244,8 @@ static void collapse_huge_page(struct mm_struct *mm,
> goto out_nolock;
> }
>
> - pmd = mm_find_pmd(mm, address);
> + pmd = find_pmd_or_thp_or_none(mm, address, &result);
> if (!pmd) {
> - result = SCAN_PMD_NULL;
> mmap_read_unlock(mm);
> goto out_nolock;
> }
> @@ -1287,11 +1318,9 @@ static void scan_pmd(struct mm_struct *mm,
> mmap_assert_locked(mm);
> VM_BUG_ON(address & ~HPAGE_PMD_MASK);
>
> - pmd = mm_find_pmd(mm, address);
> - if (!pmd) {
> - scan_result->result = SCAN_PMD_NULL;
> + pmd = find_pmd_or_thp_or_none(mm, address, &scan_result->result);
> + if (!pmd)
> goto out;
> - }
>
> memset(cc->node_load, 0, sizeof(cc->node_load));
> pte = pte_offset_map_lock(mm, pmd, address, &ptl);
> --
> 2.35.1.616.g0bdcbb4464-goog
>
