linux-kernel - Re: [PATCH v5 5/6] mm: convert copy_user_huge_page() to copy_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230406235537.GF14244@monkey>
Date:   Thu, 6 Apr 2023 16:55:37 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Peng Zhang <zhangpeng362@...wei.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        akpm@...ux-foundation.org, willy@...radead.org,
        sidhartha.kumar@...cle.com, vishal.moola@...il.com,
        muchun.song@...ux.dev, wangkefeng.wang@...wei.com,
        sunnanyong@...wei.com
Subject: Re: [PATCH v5 5/6] mm: convert copy_user_huge_page() to
 copy_user_folio()

On 03/31/23 17:39, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@...wei.com>
> 
> Replace copy_user_huge_page() with copy_user_folio(). copy_user_folio()
> does the same as copy_user_huge_page(), but takes in folios instead of
> pages. Convert copy_user_gigantic_page() to take in folios.
> Remove pages_per_huge_page from copy_user_folio(), because we can get
> that from folio_nr_pages(dst).
> 
> Signed-off-by: ZhangPeng <zhangpeng362@...wei.com>
> ---
>  include/linux/mm.h |  7 +++----
>  mm/hugetlb.c       | 10 ++++------
>  mm/memory.c        | 28 ++++++++++++++--------------
>  3 files changed, 21 insertions(+), 24 deletions(-)

No technical problems with the patch, but ...
>  
> @@ -5847,15 +5847,15 @@ static void copy_subpage(unsigned long addr, int idx, void *arg)
>  			   addr, copy_arg->vma);
>  }
>  
> -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)
> +void copy_user_folio(struct folio *dst, struct folio *src,
> +		      unsigned long addr_hint, struct vm_area_struct *vma)
>  {
> +	unsigned int pages_per_huge_page = folio_nr_pages(dst);
>  	unsigned long addr = addr_hint &
>  		~(((unsigned long)pages_per_huge_page << PAGE_SHIFT) - 1);
>  	struct copy_subpage_arg arg = {
> -		.dst = dst,
> -		.src = src,
> +		.dst = &dst->page,
> +		.src = &src->page,
>  		.vma = vma,
>  	};
>  

I seem to recall that Matthew suggested changing the function name to
copy_user_folio.  My only concern is that the name now sounds like a
general purpose routine for copying folios.  It certainly would work
for a single page folio, but there is a bunch of unnecessary overhead
in that case.

That makes me think there should perhaps be an optimized path for single
page folios that just does copy_user_highpage().  But, the argument addr_hint
does not make much sense in the single page folio case.  So, I am not
sure if I agree with leaving large/huge out of the function name.

Just wondering if Matthew has any additional thoughts?
-- 
Mike Kravetz