Re: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path

2014-01-26 Thread 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(_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.
>From 371649294d90b65a2e86d0873f79bab454285d1a Mon Sep 17 00:00:00 2001
From: David Rientjes 
Date: Sun, 26 Jan 2014 11:49:39 +0400
Subject: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path

On kmem_cache_create_memcg() error path we set 'err', but leave 's' (the
new cache ptr) undefined. The latter can be NULL if we could not
allocate the cache, or pointing to a freed area if we failed somewhere
later while trying to initialize it. Initially we checked 'err'
immediately before exiting the function and returned NULL if it was set
ignoring the value of 's':

out_unlock:
...
if (err) {
/* report error */
return NULL;
}
return s;

Recently this check was, in fact, broken by commit f717eb3abb5e ("slab:
do not panic if we fail to create memcg cache"), which turned it to:

out_unlock:
...
if (err && !memcg) {
/* report error */
return NULL;
}
return s;

As a result, if we are failing creating a cache for a memcg, we will
skip the check and return 's' that can contain crap. Obviously, commit
f717eb3abb5e intended not to return crap on error allocating a cache for
a memcg, but only to remove the error reporting in this case, so the
check should look like this:

out_unlock:
...
if (err) {
if (!memcg)
return NULL;
/* report error */
return NULL;
}
return s;

Signed-off-by: David Rientjes 
Signed-off-by: Vladimir Davydov 
Reported-by: Dave Jones 
Cc: Pekka Enberg 
Cc: Christoph Lameter 
---
 mm/slab_common.c |   19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 8e40321..1ec3c61 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -233,14 +233,17 @@ out_unlock:
 	mutex_unlock(_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. 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 (!memcg)
+			return NULL;
+
 		if (flags & SLAB_PANIC)
 			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
 name, err);
-- 
1.7.10.4

Re: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path

2014-01-26 Thread 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.
From 371649294d90b65a2e86d0873f79bab454285d1a Mon Sep 17 00:00:00 2001
From: David Rientjes rient...@google.com
Date: Sun, 26 Jan 2014 11:49:39 +0400
Subject: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path

On kmem_cache_create_memcg() error path we set 'err', but leave 's' (the
new cache ptr) undefined. The latter can be NULL if we could not
allocate the cache, or pointing to a freed area if we failed somewhere
later while trying to initialize it. Initially we checked 'err'
immediately before exiting the function and returned NULL if it was set
ignoring the value of 's':

out_unlock:
...
if (err) {
/* report error */
return NULL;
}
return s;

Recently this check was, in fact, broken by commit f717eb3abb5e (slab:
do not panic if we fail to create memcg cache), which turned it to:

out_unlock:
...
if (err  !memcg) {
/* report error */
return NULL;
}
return s;

As a result, if we are failing creating a cache for a memcg, we will
skip the check and return 's' that can contain crap. Obviously, commit
f717eb3abb5e intended not to return crap on error allocating a cache for
a memcg, but only to remove the error reporting in this case, so the
check should look like this:

out_unlock:
...
if (err) {
if (!memcg)
return NULL;
/* report error */
return NULL;
}
return s;

Signed-off-by: David Rientjes rient...@google.com
Signed-off-by: Vladimir Davydov vdavy...@parallels.com
Reported-by: Dave Jones da...@redhat.com
Cc: Pekka Enberg penb...@kernel.org
Cc: Christoph Lameter c...@linux.com
---
 mm/slab_common.c |   19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 8e40321..1ec3c61 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -233,14 +233,17 @@ 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. 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 (!memcg)
+			return NULL;
+
 		if (flags  SLAB_PANIC)
 			panic(kmem_cache_create: Failed to create slab '%s'. Error %d\n,
 name, err);
-- 
1.7.10.4

Re: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path

2014-01-25 Thread David Rientjes 
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(_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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path

2014-01-25 Thread David Rientjes 
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path

2014-01-24 Thread Vladimir Davydov 
From: Dave Jones 

On kmem_cache_create_memcg() error path we set 'err', but leave 's' (the
new cache ptr) undefined. The latter can be NULL if we could not
allocate the cache, or pointing to a freed area if we failed somewhere
later while trying to initialize it. Initially we checked 'err'
immediately before exiting the function and returned NULL if it was set
ignoring the value of 's':

out_unlock:
...
if (err) {
...
return NULL;
}
return s;

Recently this check was, in fact, broken by commit f717eb3abb5e ("slab:
do not panic if we fail to create memcg cache"), which turned it to:

out_unlock:
...
if (err && !memcg) {
...
return NULL;
}
return s;

As a result, if we are failing creating a cache for a memcg, we will
skip the check and return 's' that can contain crap. Let's fix it by
assuring that on error path there are always two conditions satisfied at
the same time, err != 0 and s == NULL, by explicitly zeroing 's' after
freeing it on error path.

Signed-off-by: Dave Jones 
Signed-off-by: Vladimir Davydov 
Cc: Pekka Enberg 
Cc: Christoph Lameter 
---
 mm/slab_common.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
 }
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path

2014-01-24 Thread Vladimir Davydov 
From: Dave Jones da...@redhat.com

On kmem_cache_create_memcg() error path we set 'err', but leave 's' (the
new cache ptr) undefined. The latter can be NULL if we could not
allocate the cache, or pointing to a freed area if we failed somewhere
later while trying to initialize it. Initially we checked 'err'
immediately before exiting the function and returned NULL if it was set
ignoring the value of 's':

out_unlock:
...
if (err) {
...
return NULL;
}
return s;

Recently this check was, in fact, broken by commit f717eb3abb5e (slab:
do not panic if we fail to create memcg cache), which turned it to:

out_unlock:
...
if (err  !memcg) {
...
return NULL;
}
return s;

As a result, if we are failing creating a cache for a memcg, we will
skip the check and return 's' that can contain crap. Let's fix it by
assuring that on error path there are always two conditions satisfied at
the same time, err != 0 and s == NULL, by explicitly zeroing 's' after
freeing it on error path.

Signed-off-by: Dave Jones da...@redhat.com
Signed-off-by: Vladimir Davydov vdavy...@parallels.com
Cc: Pekka Enberg penb...@kernel.org
Cc: Christoph Lameter c...@linux.com
---
 mm/slab_common.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
 }
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/