Re: [PATCH v2 01/46] hugetlb: don't set PageUptodate for UFFDIO_CONTINUE

[Mina Almasry]

On Fri, Feb 17, 2023 at 4:28 PM James Houghton <jthoughton@xxxxxxxxxx> wrote:
>
> If would be bad if we actually set PageUptodate with UFFDIO_CONTINUE;
> PageUptodate indicates that the page has been zeroed, and we don't want
> to give a non-zeroed page to the user.
>
> The reason this change is being made now is because UFFDIO_CONTINUEs on
> subpages definitely shouldn't set this page flag on the head page.
>
> Signed-off-by: James Houghton <jthoughton@xxxxxxxxxx>
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 07abcb6eb203..792cb2e67ce5 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6256,7 +6256,16 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>          * preceding stores to the page contents become visible before
>          * the set_pte_at() write.
>          */
> -       __folio_mark_uptodate(folio);
> +       if (!is_continue)
> +               __folio_mark_uptodate(folio);
> +       else if (!folio_test_uptodate(folio)) {
> +               /*
> +                * This should never happen; HugeTLB pages are always Uptodate
> +                * as soon as they are allocated.
> +                */

if (is_continue) then we grab a page from the page cache, no? Are
pages in page caches always uptodate? Why? I guess that means they're
mapped hence uptodate?

Also this comment should explain why pages in the page cache are
always uptodate, no? Because this error branch is hit if (is_continue
&& !folio_test_uptodate()), not when pages are freshly allocated.

> +               ret = -EFAULT;
> +               goto out_release_nounlock;
> +       }
>
>         /* Add shared, newly allocated pages to the page cache. */
>         if (vm_shared && !is_continue) {
> --
> 2.39.2.637.g21b0678d19-goog
>