Re: [PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[Nicholas Krause <xerofoify@gmail.com>]

On June 29, 2015 11:03:11 AM EDT, Michal Hocko <mhocko@suse.cz> wrote:
>On Mon 29-06-15 10:13:53, Nicholas Krause wrote:
>[...]
>> -static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg,
>int node)
>> +static bool alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg,
>int node)
>>  {
>>  	struct mem_cgroup_per_node *pn;
>>  	struct mem_cgroup_per_zone *mz;
>> @@ -4442,7 +4442,7 @@ static int
>alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
>>  		tmp = -1;
>>  	pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp);
>>  	if (!pn)
>> -		return 1;
>> +		return true;
>
>Have you tried to think about the semantic of the function? The
>function
>has returned 0 to signal the success which is pretty common. It could
>have
>returned -ENOMEM for the allocation failure which would be much more
>nicer than 1.
>
>After your change we have bool semantic where the success is reported
>by
>false while failure is true. Doest this make any sense to you? Because
>it doesn't make to me and it only shows that this is a mechanical
>conversion without deeper thinking about consequences.
>
>Nacked-by: Michal Hocko <mhocko@suse.cz>
>
>Btw. I can see your other patches which trying to do similar. I would
>strongly discourage you from this path. Try to understand the code and
>focus on changes which would actually make any improvements to the code
>base. Doing stylist changes which do not help readability and neither
>help compiler to generate a better code is simply waste of your and
>reviewers time.
>
>>  	for (zone = 0; zone < MAX_NR_ZONES; zone++) {
>>  		mz = &pn->zoneinfo[zone];
>> @@ -4452,7 +4452,7 @@ static int
>alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
>>  		mz->memcg = memcg;
>>  	}
>>  	memcg->nodeinfo[node] = pn;
>> -	return 0;
>> +	return false;
>>  }
>>  
>>  static void free_mem_cgroup_per_zone_info(struct mem_cgroup *memcg,
>int node)
>> -- 
>> 2.1.4
>> 
I agree with and looked into the callers about this wasn't sure if you you wanted me to return - ENOMEM.  I will rewrite this patch the other way.  Furthermore I apologize about this and do have actual useful patches but will my rep it's hard to get replies from maintainers. If you would like to take a look at them please let know. 
Nick 
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>