Re: [PATCH v5 01/13] mm/khugepaged: record SCAN_PMD_MAPPED when scan_pmd() finds THP

[Peter Xu <peterx@redhat.com>]

On Wed, May 04, 2022 at 02:44:25PM -0700, Zach O'Keefe wrote:
> +static int find_pmd_or_thp_or_none(struct mm_struct *mm,
> +				   unsigned long address,
> +				   pmd_t **pmd)
> +{
> +	pmd_t pmde;
> +
> +	*pmd = mm_find_pmd_raw(mm, address);
> +	if (!*pmd)
> +		return SCAN_PMD_NULL;
> +
> +	pmde = pmd_read_atomic(*pmd);

It seems to be correct on using the atomic fetcher here.  Though irrelevant
to this patchset.. does it also mean that we miss that on mm_find_pmd()?  I
meant a separate fix like this one:

---8<---
diff --git a/mm/rmap.c b/mm/rmap.c
index 69416072b1a6..61309718640f 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -785,7 +785,7 @@ pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address)
         * without holding anon_vma lock for write.  So when looking for a
         * genuine pmde (in which to find pte), test present and !THP together.
         */
-       pmde = *pmd;
+       pmde = pmd_read_atomic(pmd);
        barrier();
        if (!pmd_present(pmde) || pmd_trans_huge(pmde))
                pmd = NULL;
---8<---

As otherwise it seems it's also prone to PAE race conditions when reading
pmd out, but I could be missing something.

> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +	/* See comments in pmd_none_or_trans_huge_or_clear_bad() */
> +	barrier();
> +#endif
> +	if (!pmd_present(pmde))
> +		return SCAN_PMD_NULL;
> +	if (pmd_trans_huge(pmde))
> +		return SCAN_PMD_MAPPED;

Would it be safer to check pmd_bad()?  I think not all mm pmd paths check
that, frankly I don't really know what's the major cause of a bad pmd
(either software bugs or corrupted mem), but just to check with you,
because potentially a bad pmd can be read as SCAN_SUCCEED and go through.

> +	return SCAN_SUCCEED;
> +}

The rest looks good to me, thanks.

-- 
Peter Xu