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 <[email protected]> wrote:
> > > From: ZhangPeng <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Reviewed-by: Sidhartha Kumar <[email protected]>
> > > ---
> > > - 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
>
On 4/11/2023 5:26 AM, Mike Kravetz wrote:
> 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 <[email protected]> wrote:
>>>> From: ZhangPeng <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Reviewed-by: Sidhartha Kumar <[email protected]>
>>>> ---
>>>> - 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.
Maybe we should flush_dcache because this function returns the
bytes copied successfully? flushing cache to make sure the copied
pieces to RAM for sure.
For the range not copied yet, flushing cache or not doesn't make
difference. Thanks.
Regards
Yin, Fengwei