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

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

[Michal Hocko]

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

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

[Michal Hocko]

On Mon 29-06-15 11:23:08, Nicholas Krause wrote:
[...]
> 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. 

I am not sure this path really needs a cleanup.

> Furthermore I apologize about this and do have actual useful
> patches but will my rep it's hard to get replies from maintainers.

You can hardly expect somebody will be thrilled about your patches when
their fault rate is close to 100%. Reviewing each patch takes time and
that is a scarce resource. If you want people to follow your patches
make sure you are offering something that might be interesting or
useful. Cleanups like these usually are not interesting without
either building something bigger on top of them or when they improve
readability considerably.

[...]
-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Mon 29-06-15 11:44:24, nick wrote:
[...]
> Here is a patch series I have been trying to merge for a bug in the
> gma500 other the last few patches. There are other patches I have like
> this lying around.

I am not interested in looking at random and unrelated patches. I am
also not thrilled about random cleanups which do not have an additional
value. I will not repeat that again and will start ignoring patches like
that. I have tried to help you but this seems pointless investment of my
time.
-- 
Michal Hocko
SUSE Labs