Re: [PATCH 2/2] mm/page_alloc: remove unnecessary parameter batch of nr_pcp_free

[Kemeng Shi]

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@xxxxxxxxxxxxxxx>
>> ---
>>  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
>