Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230331093937.945725-1-zhangpeng362@huawei.com> <20230331093937.945725-4-zhangpeng362@huawei.com>
In-Reply-To: <20230331093937.945725-4-zhangpeng362@huawei.com>
From:   Vishal Moola <vishal.moola@gmail.com>
Date:   Thu, 6 Apr 2023 19:28:44 -0700
Message-ID: <CAOzc2pzr7VJRdsx1ud_ceBhbu2XP7Ay72jFETtN8eOt5yR7S=Q@mail.gmail.com>
Subject: Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()
To:     Peng Zhang <zhangpeng362@huawei.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, willy@infradead.org,
        mike.kravetz@oracle.com, sidhartha.kumar@oracle.com,
        muchun.song@linux.dev, wangkefeng.wang@huawei.com,
        sunnanyong@huawei.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Mar 31, 2023 at 2:41=E2=80=AFAM Peng Zhang <zhangpeng362@huawei.com=
> wrote:
>
> From: ZhangPeng <zhangpeng362@huawei.com>
>
> Replace copy_huge_page_from_user() with copy_folio_from_user().
> copy_folio_from_user() does the same as copy_huge_page_from_user(), but
> takes in a folio instead of a page. Convert page_kaddr to kaddr in
> copy_folio_from_user() to do indenting cleanup.
>
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  include/linux/mm.h |  7 +++----
>  mm/hugetlb.c       |  5 ++---
>  mm/memory.c        | 26 ++++++++++++--------------
>  mm/userfaultfd.c   |  6 ++----
>  4 files changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e249208f8fbe..cf4d773ca506 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3682,10 +3682,9 @@ extern void copy_user_huge_page(struct page *dst, =
struct page *src,
>                                 unsigned long addr_hint,
>                                 struct vm_area_struct *vma,
>                                 unsigned int pages_per_huge_page);
> -extern long copy_huge_page_from_user(struct page *dst_page,
> -                               const void __user *usr_src,
> -                               unsigned int pages_per_huge_page,
> -                               bool allow_pagefault);
> +long copy_folio_from_user(struct folio *dst_folio,
> +                          const void __user *usr_src,
> +                          bool allow_pagefault);
>
>  /**
>   * vma_is_special_huge - Are transhuge page-table entries considered spe=
cial?
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 7e4a80769c9e..aade1b513474 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6217,9 +6217,8 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
>                         goto out;
>                 }
>
> -               ret =3D copy_huge_page_from_user(&folio->page,
> -                                               (const void __user *) src=
_addr,
> -                                               pages_per_huge_page(h), f=
alse);
> +               ret =3D copy_folio_from_user(folio, (const void __user *)=
 src_addr,
> +                                          false);
>
>                 /* fallback to copy_from_user outside mmap_lock */
>                 if (unlikely(ret)) {
> diff --git a/mm/memory.c b/mm/memory.c
> index 808f354bce65..4976422b6979 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5868,35 +5868,33 @@ void copy_user_huge_page(struct page *dst, struct=
 page *src,
>         process_huge_page(addr_hint, pages_per_huge_page, copy_subpage, &=
arg);
>  }
>
> -long copy_huge_page_from_user(struct page *dst_page,
> -                               const void __user *usr_src,
> -                               unsigned int pages_per_huge_page,
> -                               bool allow_pagefault)
> +long copy_folio_from_user(struct folio *dst_folio,
> +                          const void __user *usr_src,
> +                          bool allow_pagefault)
>  {
> -       void *page_kaddr;
> +       void *kaddr;
>         unsigned long i, rc =3D 0;
> -       unsigned long ret_val =3D pages_per_huge_page * PAGE_SIZE;
> +       unsigned int nr_pages =3D folio_nr_pages(dst_folio);
> +       unsigned long ret_val =3D nr_pages * PAGE_SIZE;
>         struct page *subpage;
>
> -       for (i =3D 0; i < pages_per_huge_page; i++) {
> -               subpage =3D nth_page(dst_page, i);
> -               page_kaddr =3D kmap_local_page(subpage);
> +       for (i =3D 0; i < nr_pages; i++) {
> +               subpage =3D folio_page(dst_folio, i);
> +               kaddr =3D kmap_local_page(subpage);
>                 if (!allow_pagefault)
>                         pagefault_disable();
> -               rc =3D copy_from_user(page_kaddr,
> -                               usr_src + i * PAGE_SIZE, PAGE_SIZE);
> +               rc =3D copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAG=
E_SIZE);
>                 if (!allow_pagefault)
>                         pagefault_enable();
> -               kunmap_local(page_kaddr);
> +               kunmap_local(kaddr);
>
>                 ret_val -=3D (PAGE_SIZE - rc);
>                 if (rc)
>                         break;
>
> -               flush_dcache_page(subpage);
> -
>                 cond_resched();
>         }
> +       flush_dcache_folio(dst_folio);
>         return ret_val;
>  }

Moving the flush_dcache_page() outside the loop to be
flush_dcache_folio() changes the behavior of the function.

Initially, if it fails to copy the entire page, the function breaks out
of the loop and returns the number of unwritten bytes without
flushing the page from the cache. Now if it fails, it will still flush
out the page it failed on, as well as any later pages it may not
have gotten to yet.