lkml.org 
[lkml]   [2023]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 2/3] userfaultfd: convert __mcopy_atomic_hugetlb() to use a folio
    From
    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.

    \
     
     \ /
      Last update: 2023-03-27 00:59    [W:6.819 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site