Messages in this thread | | | Date | Tue, 14 Mar 2023 19:47:20 +0800 | Subject | Re: [PATCH 2/3] userfaultfd: convert __mcopy_atomic_hugetlb() to use a folio | From | "zhangpeng (AS)" <> |
| |
On 2023/3/14 16:31, Matthew Wilcox wrote:
> On Tue, Mar 14, 2023 at 03:37:33AM +0000, Peng Zhang wrote: >> +++ b/include/linux/mm.h >> @@ -3546,9 +3546,8 @@ 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, >> +extern long copy_large_folio_from_user(struct folio *dst_folio, > You can drop the 'extern'.
Got it.
>> +++ b/mm/memory.c >> @@ -5769,26 +5769,28 @@ 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, >> +long copy_large_folio_from_user(struct folio *dst_folio, >> const void __user *usr_src, >> - unsigned int pages_per_huge_page, >> bool allow_pagefault) >> { >> void *page_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; >> + struct folio *inner_folio; > What is an 'inner folio'? > >> - for (i = 0; i < pages_per_huge_page; i++) { >> - subpage = nth_page(dst_page, i); >> + for (i = 0; i < nr_pages; i++) { >> + subpage = folio_page(dst_folio, i); >> + inner_folio = page_folio(subpage); >> if (allow_pagefault) >> - page_kaddr = kmap(subpage); >> + page_kaddr = kmap_local_folio(inner_folio, 0); > This doesn't do what you think it does. Did you test this? > >> else >> page_kaddr = kmap_atomic(subpage); > Pretty sure all this should be converted to kmap_local and the atomic > bits should go away. > >> rc = copy_from_user(page_kaddr, >> usr_src + i * PAGE_SIZE, PAGE_SIZE); >> if (allow_pagefault) >> - kunmap(subpage); >> + kunmap_local(page_kaddr); >> else >> kunmap_atomic(page_kaddr); >> >> @@ -5796,7 +5798,7 @@ long copy_huge_page_from_user(struct page *dst_page, >> if (rc) >> break; >> >> - flush_dcache_page(subpage); >> + flush_dcache_folio(inner_folio); > The flush should probably be pulled outside the loop. > >> + err = copy_large_folio_from_user(folio, >> + (const void __user *) src_addr, true); > I wonder if this shouldn't be 'copy_folio_from_user()'. after all, > it'll work for any size folio, right?
Thanks for your review.
I'll rename copy_large_folio_from_user() to copy_folio_from_user(). I'll delete the inner_folio. kmap() and kmap_atomic() will be converted to the following code.
page_kaddr = kmap_local_page(subpage); if (!allow_pagefault) pagefault_disable(); rc = copy_from_user(page_kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE); if (!allow_pagefault) pagefault_enable(); kunmap_local(page_kaddr);
flush_dcache_folio() will be placed outside the loop.
I'll fix all this in a v2 of this patch series.
Thanks, Peng.
| |