linux-kernel - Re: [PATCH 2/2] mm/page_alloc: remove unnecessary parameter batch of nr_pcp

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: <29681039-7a68-609e-4d3c-0bd91ca632bd@huaweicloud.com>
Date:   Thu, 17 Aug 2023 10:43:37 +0800
From:   Kemeng Shi <shikemeng@...weicloud.com>
To:     Chris Li <chrisl@...nel.org>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        akpm@...ux-foundation.org, baolin.wang@...ux.alibaba.com,
        mgorman@...hsingularity.net, david@...hat.com, willy@...radead.org
Subject: Re: [PATCH 2/2] mm/page_alloc: remove unnecessary parameter batch of
 nr_pcp_free



on 8/16/2023 1:46 AM, Chris Li wrote:
> Hi Kemeng,
> 
> Since I am discussing the other patch in this series, I might just commend on this one
> as well.
> 
> On Wed, Aug 09, 2023 at 06:07:54PM +0800, Kemeng Shi wrote:
>> We get batch from pcp and just pass it to nr_pcp_free immediately. Get
>> batch from pcp inside nr_pcp_free to remove unnecessary parameter batch.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@...weicloud.com>
>> ---
>>  mm/page_alloc.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 1ddcb2707d05..bb1d14e806ad 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -2376,10 +2376,10 @@ static bool free_unref_page_prepare(struct page *page, unsigned long pfn,
>>  	return true;
>>  }
>>  
>> -static int nr_pcp_free(struct per_cpu_pages *pcp, int high, int batch,
>> -		       bool free_high)
>> +static int nr_pcp_free(struct per_cpu_pages *pcp, int high, bool free_high)
>>  {
>>  	int min_nr_free, max_nr_free;
>> +	int batch = READ_ONCE(pcp->batch);
> 
> Because nr_pcp_free is static and has only one caller. This function gets inlined
> at the caller's side. I verified that on X86_64 compiled code.
> 
> So this fix in my opinion is not worthwhile to fix. It will produce the same
> machine code. One minor side effect is that it will hide the commit under it
> in "git blame".
> 
Hi Chris, thank for the reply. Except to reduce argument to pass, this patch also
tries make code look little cleaner. I think it's always better to reduce variable
scope and keep relevant code tight. In this case, we know batch is from
per_cpu_pages during reading nr_pcp_free alone rather than search caller to find it
out. And more callers of nr_pcp_free in fulture is free from pass pcp->batch. And so
on. Anyway, this patch definely gains a little without lost in my opinion.:) With it
makes sense to you.

> Chris
>