Date:   Tue, 14 Mar 2023 08:31:23 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Peng Zhang <zhangpeng362@huawei.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, mike.kravetz@oracle.com,
        vishal.moola@gmail.com, sidhartha.kumar@oracle.com,
        wangkefeng.wang@huawei.com, sunnanyong@huawei.com
Subject: Re: [PATCH 2/3] userfaultfd: convert __mcopy_atomic_hugetlb() to use
 a folio
Message-ID: <ZBAw21yANYPFKMi4@casper.infradead.org>
References: <20230314033734.481904-1-zhangpeng362@huawei.com>
 <20230314033734.481904-3-zhangpeng362@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230314033734.481904-3-zhangpeng362@huawei.com>
Precedence: bulk

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?