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

[Michal Hocko <mhocko@suse.cz>]

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
> 

-- 
Michal Hocko
SUSE Labs