Re: [PATCH 2/3] userfaultfd: convert __mcopy_atomic_hugetlb() to use a folio

[Matthew Wilcox]

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'.

> +++ 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?