Re: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg errorpath

[Vladimir Davydov]

 On 01/26/2014 08:39 AM, David Rientjes wrote:
> On Sat, 25 Jan 2014, Vladimir Davydov wrote:
>
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index 8e40321..499b53c 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -249,7 +249,6 @@ out_unlock:
>>  				name, err);
>>  			dump_stack();
>>  		}
>> -		return NULL;
>>  	}
>>  	return s;
>>  
>> @@ -257,6 +256,7 @@ out_free_cache:
>>  	memcg_free_cache_params(s);
>>  	kfree(s->name);
>>  	kmem_cache_free(kmem_cache, s);
>> +	s = NULL;
>>  	goto out_unlock;
>>  }
>>  
> I thought I left spaghetti code back in my BASIC 2.0 days.  It should be 
> much more readable to just do
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -233,14 +233,15 @@ out_unlock:
>  	mutex_unlock(&slab_mutex);
>  	put_online_cpus();
>  
> -	/*
> -	 * There is no point in flooding logs with warnings or especially
> -	 * crashing the system if we fail to create a cache for a memcg. In
> -	 * this case we will be accounting the memcg allocation to the root
> -	 * cgroup until we succeed to create its own cache, but it isn't that
> -	 * critical.
> -	 */
> -	if (err && !memcg) {
> +	if (err) {
> +		/*
> +		 * There is no point in flooding logs with warnings or
> +		 * especially crashing the system if we fail to create a cache
> +		 * for a memcg.
> +		 */
> +		if (memcg)
> +			return NULL;
> +
>  		if (flags & SLAB_PANIC)
>  			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
>  				name, err);
>
> and stop trying to remember what err, memcg, and s are in all possible 
> contexts.  Sheesh.

Hi, David,

Although it's rather a matter of personal preference, I tend to agree
with you.

Andrew,

The fix by David Rientjes is attached. It's up to you to decide, which
one looks better.

Thank you and sorry about the noise.