Re: [PATCH v5 5/6] mm: convert copy_user_huge_page() to copy_user_folio()

[zhangpeng (AS)]

On 2023/4/7 7:55, Mike Kravetz wrote:

> On 03/31/23 17:39, Peng Zhang wrote:
> > From: ZhangPeng <zhangpeng362@xxxxxxxxxx>
> >
> > Replace copy_user_huge_page() with copy_user_folio(). copy_user_folio()
> > does the same as copy_user_huge_page(), but takes in folios instead of
> > pages. Convert copy_user_gigantic_page() to take in folios.
> > Remove pages_per_huge_page from copy_user_folio(), because we can get
> > that from folio_nr_pages(dst).
> >
> > Signed-off-by: ZhangPeng <zhangpeng362@xxxxxxxxxx>
> > ---
> >   include/linux/mm.h |  7 +++----
> >   mm/hugetlb.c       | 10 ++++------
> >   mm/memory.c        | 28 ++++++++++++++--------------
> >   3 files changed, 21 insertions(+), 24 deletions(-)
> No technical problems with the patch, but ...
> >   
> > @@ -5847,15 +5847,15 @@ static void copy_subpage(unsigned long addr, int idx, void *arg)
> >   			   addr, copy_arg->vma);
> >   }
> >   
> > -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)
> > +void copy_user_folio(struct folio *dst, struct folio *src,
> > +		      unsigned long addr_hint, struct vm_area_struct *vma)
> >   {
> > +	unsigned int pages_per_huge_page = folio_nr_pages(dst);
> >   	unsigned long addr = addr_hint &
> >   		~(((unsigned long)pages_per_huge_page << PAGE_SHIFT) - 1);
> >   	struct copy_subpage_arg arg = {
> > -		.dst = dst,
> > -		.src = src,
> > +		.dst = &dst->page,
> > +		.src = &src->page,
> >   		.vma = vma,
> >   	};
> >   
> I seem to recall that Matthew suggested changing the function name to
> copy_user_folio.  My only concern is that the name now sounds like a
> general purpose routine for copying folios.  It certainly would work
> for a single page folio, but there is a bunch of unnecessary overhead
> in that case.
>
> That makes me think there should perhaps be an optimized path for single
> page folios that just does copy_user_highpage().  But, the argument addr_hint
> does not make much sense in the single page folio case.  So, I am not
> sure if I agree with leaving large/huge out of the function name.
>
> Just wondering if Matthew has any additional thoughts?

Agreed. In my opinion, it's better to leave large/huge out of the
function name.
Also wondering if Matthew has any additional considerations?

Best Regards,
Peng