Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[Mike Kravetz]

On 04/08/23 12:43, zhangpeng (AS) wrote:
> On 2023/4/7 10:28, Vishal Moola wrote:
> 
> > On Fri, Mar 31, 2023 at 2:41 AM Peng Zhang <zhangpeng362@xxxxxxxxxx> wrote:
> > > From: ZhangPeng <zhangpeng362@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > Reviewed-by: Sidhartha Kumar <sidhartha.kumar@xxxxxxxxxx>
> > > ---
> > > -                               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 = 0;
> > > -       unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
> > > +       unsigned int nr_pages = folio_nr_pages(dst_folio);
> > > +       unsigned long ret_val = nr_pages * PAGE_SIZE;
> > >          struct page *subpage;
> > > 
> > > -       for (i = 0; i < pages_per_huge_page; i++) {
> > > -               subpage = nth_page(dst_page, i);
> > > -               page_kaddr = kmap_local_page(subpage);
> > > +       for (i = 0; i < nr_pages; i++) {
> > > +               subpage = folio_page(dst_folio, i);
> > > +               kaddr = kmap_local_page(subpage);
> > >                  if (!allow_pagefault)
> > >                          pagefault_disable();
> > > -               rc = copy_from_user(page_kaddr,
> > > -                               usr_src + i * PAGE_SIZE, PAGE_SIZE);
> > > +               rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
> > >                  if (!allow_pagefault)
> > >                          pagefault_enable();
> > > -               kunmap_local(page_kaddr);
> > > +               kunmap_local(kaddr);
> > > 
> > >                  ret_val -= (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.
> 
> Agreed. If it fails, could we just not flush the folio?

I believe that should be OK.  If returning an error, nobody should be
depending on any part of the page being present or not in the cache.
-- 
Mike Kravetz

> Like this:
> long copy_folio_from_user(...)
> {
> 	...
> 	for (i = 0; i < nr_pages; i++) {
> 		...
> 		rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
> 		...
> 		ret_val -= (PAGE_SIZE - rc);
> 		if (rc)
> -                       break;
> +                       return ret_val;
> 		cond_resched();
> 	}
> 	flush_dcache_folio(dst_folio);
> 	return ret_val;
> }
> 
> Thanks for your review.
> 
> Best Regards,
> Peng
>