Received-SPF: pass (google.com: domain of linux-kernel+bounces-171443-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <10484734-13e6-4c56-d7dc-6c4cf74bd8de@huawei.com>
Date: Tue, 7 May 2024 21:31:39 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH v2 -next] mm: memcg: make alloc_mem_cgroup_per_node_info()
 return bool
Content-Language: en-US
To: Michal Hocko <mhocko@suse.com>
CC: <hannes@cmpxchg.org>, <roman.gushchin@linux.dev>,
	<shakeel.butt@linux.dev>, <muchun.song@linux.dev>,
	<akpm@linux-foundation.org>, <cgroups@vger.kernel.org>, <linux-mm@kvack.org>,
	<linux-kernel@vger.kernel.org>
References: <20240507110832.1128370-1-xiujianfeng@huawei.com>
 <ZjooSnQ_vS30EXCT@tiehlicka>
From: xiujianfeng <xiujianfeng@huawei.com>
In-Reply-To: <ZjooSnQ_vS30EXCT@tiehlicka>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 2024/5/7 21:10, Michal Hocko wrote:
> On Tue 07-05-24 11:08:32, Xiu Jianfeng wrote:
>> Currently alloc_mem_cgroup_per_node_info() returns 1 if failed,
>> make it return bool, false for failure and true for success.
> 
> This describes what the patch does rather than why it is doing that.
> The former is clear from the diff while the motivation for this change
> is unclear. I would propose something like:
> 
> alloc_mem_cgroup_per_node_info() returns int that doesn't map to any
> errno error code. The only existing caller doesn't really need an error
> code so change the the function to return bool (true on success) because
> this is slightly less confusing and more consistent with the other code.

Thanks, it looks much better now.

> 
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> 
> With changelog clarified feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
>> ---
>>  mm/memcontrol.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index d11536ef59ef..69d70feb8e68 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -5653,13 +5653,13 @@ struct mem_cgroup *mem_cgroup_get_from_ino(unsigned long ino)
>>  }
>>  #endif
>>  
>> -static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
>> +static bool alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
>>  {
>>  	struct mem_cgroup_per_node *pn;
>>  
>>  	pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, node);
>>  	if (!pn)
>> -		return 1;
>> +		return false;
>>  
>>  	pn->lruvec_stats = kzalloc_node(sizeof(struct lruvec_stats), GFP_KERNEL,
>>  					node);
>> @@ -5675,11 +5675,11 @@ static int alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
>>  	pn->memcg = memcg;
>>  
>>  	memcg->nodeinfo[node] = pn;
>> -	return 0;
>> +	return true;
>>  fail:
>>  	kfree(pn->lruvec_stats);
>>  	kfree(pn);
>> -	return 1;
>> +	return false;
>>  }
>>  
>>  static void free_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
>> @@ -5751,7 +5751,7 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
>>  	}
>>  
>>  	for_each_node(node)
>> -		if (alloc_mem_cgroup_per_node_info(memcg, node))
>> +		if (!alloc_mem_cgroup_per_node_info(memcg, node))
>>  			goto fail;
>>  
>>  	if (memcg_wb_domain_init(memcg, GFP_KERNEL))
>> -- 
>> 2.34.1
>>
>