Re: [PATCH v2 -next] mm: memcg: make alloc_mem_cgroup_per_node_info() return bool

[xiujianfeng <xiujianfeng@huawei.com>]

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