netdev - Re: [RFC v11 09/14] mm: page_frag: use __alloc_pages() to replace alloc_pages

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: <e9982acd0eba5d06d178d0157aedfba569d5a09a.camel@gmail.com>
Date: Sun, 21 Jul 2024 14:41:34 -0700
From: Alexander H Duyck <alexander.duyck@...il.com>
To: Yunsheng Lin <linyunsheng@...wei.com>, davem@...emloft.net,
 kuba@...nel.org,  pabeni@...hat.com
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, Andrew Morton
	 <akpm@...ux-foundation.org>, linux-mm@...ck.org
Subject: Re: [RFC v11 09/14] mm: page_frag: use __alloc_pages() to replace
 alloc_pages_node()

On Fri, 2024-07-19 at 17:33 +0800, Yunsheng Lin wrote:
> There are more new APIs calling __page_frag_cache_refill() in
> this patchset, which may cause compiler not being able to inline
> __page_frag_cache_refill() into __page_frag_alloc_va_align().
> 
> Not being able to do the inlining seems to casue some notiable
> performance degradation in arm64 system with 64K PAGE_SIZE after
> adding new API calling __page_frag_cache_refill().
> 
> It seems there is about 24Bytes binary size increase for
> __page_frag_cache_refill() and __page_frag_cache_refill() in
> arm64 system with 64K PAGE_SIZE. By doing the gdb disassembling,
> It seems we can have more than 100Bytes decrease for the binary
> size by using __alloc_pages() to replace alloc_pages_node(), as
> there seems to be some unnecessary checking for nid being
> NUMA_NO_NODE, especially when page_frag is still part of the mm
> system.
> 
> CC: Alexander Duyck <alexander.duyck@...il.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@...wei.com>
> ---
>  mm/page_frag_cache.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> index d9c9cad17af7..3f162e9d23ba 100644
> --- a/mm/page_frag_cache.c
> +++ b/mm/page_frag_cache.c
> @@ -59,11 +59,11 @@ static struct page *__page_frag_cache_refill(struct page_frag_cache *nc,
>  #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
>  	gfp_mask = (gfp_mask & ~__GFP_DIRECT_RECLAIM) |  __GFP_COMP |
>  		   __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC;
> -	page = alloc_pages_node(NUMA_NO_NODE, gfp_mask,
> -				PAGE_FRAG_CACHE_MAX_ORDER);
> +	page = __alloc_pages(gfp_mask, PAGE_FRAG_CACHE_MAX_ORDER,
> +			     numa_mem_id(), NULL);
>  #endif
>  	if (unlikely(!page)) {
> -		page = alloc_pages_node(NUMA_NO_NODE, gfp, 0);
> +		page = __alloc_pages(gfp, 0, numa_mem_id(), NULL);
>  		if (unlikely(!page)) {
>  			memset(nc, 0, sizeof(*nc));
>  			return NULL;

So if I am understanding correctly this is basically just stripping the
checks that were being performed since they aren't really needed to
verify the output of numa_mem_id.

Rather than changing the code here, it might make more sense to update
alloc_pages_node_noprof to move the lines from
__alloc_pages_node_noprof into it. Then you could put the VM_BUG_ON and
warn_if_node_offline into an else statement which would cause them to
be automatically stripped for this and all other callers. The benefit
would likely be much more significant and may be worthy of being
accepted on its own merit without being a part of this patch set as I
would imagine it would show slight gains in terms of performance and
binary size by dropping the unnecessary instructions.