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
>
