[PATCH 1/5] mm, slab: Remove silly function slab_buffer_size()

[PATCH 1/5] mm, slab: Remove silly function slab_buffer_size()

[Ezequiel Garcia]

This function is seldom used, and can be simply replaced with cachep->size.

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 mm/slab.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 3b4587b..53e41de 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -498,14 +498,6 @@ static void **dbg_userword(struct kmem_cache *cachep, void *objp)
 
 #endif
 
-#ifdef CONFIG_TRACING
-size_t slab_buffer_size(struct kmem_cache *cachep)
-{
-	return cachep->size;
-}
-EXPORT_SYMBOL(slab_buffer_size);
-#endif
-
 /*
  * Do not go above this order unless 0 objects fit into the slab or
  * overridden on the command line.
@@ -3849,7 +3841,7 @@ kmem_cache_alloc_trace(size_t size, struct kmem_cache *cachep, gfp_t flags)
 	ret = __cache_alloc(cachep, flags, __builtin_return_address(0));
 
 	trace_kmalloc(_RET_IP_, ret,
-		      size, slab_buffer_size(cachep), flags);
+		      size, cachep->size, flags);
 	return ret;
 }
 EXPORT_SYMBOL(kmem_cache_alloc_trace);
@@ -3880,7 +3872,7 @@ void *kmem_cache_alloc_node_trace(size_t size,
 	ret = __cache_alloc_node(cachep, flags, nodeid,
 				  __builtin_return_address(0));
 	trace_kmalloc_node(_RET_IP_, ret,
-			   size, slab_buffer_size(cachep),
+			   size, cachep->size,
 			   flags, nodeid);
 	return ret;
 }
-- 
1.7.8.6

--
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>

[PATCH 2/5] mm, slob: Add support for kmalloc_track_caller()

[Ezequiel Garcia]

Currently slob falls back to regular kmalloc for this case.
With this patch kmalloc_track_caller() is correctly implemented,
thus tracing the specified caller.

This is important to trace accurately allocations performed by
krealloc, kstrdup, kmemdup, etc.

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 include/linux/slab.h |    6 ++++--
 mm/slob.c            |   27 ++++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 0dd2dfa..83d1a14 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -321,7 +321,8 @@ static inline void *kmem_cache_alloc_node(struct kmem_cache *cachep,
  * request comes from.
  */
 #if defined(CONFIG_DEBUG_SLAB) || defined(CONFIG_SLUB) || \
-	(defined(CONFIG_SLAB) && defined(CONFIG_TRACING))
+	(defined(CONFIG_SLAB) && defined(CONFIG_TRACING)) || \
+	(defined(CONFIG_SLOB) && defined(CONFIG_TRACING))
 extern void *__kmalloc_track_caller(size_t, gfp_t, unsigned long);
 #define kmalloc_track_caller(size, flags) \
 	__kmalloc_track_caller(size, flags, _RET_IP_)
@@ -340,7 +341,8 @@ extern void *__kmalloc_track_caller(size_t, gfp_t, unsigned long);
  * allocation request comes from.
  */
 #if defined(CONFIG_DEBUG_SLAB) || defined(CONFIG_SLUB) || \
-	(defined(CONFIG_SLAB) && defined(CONFIG_TRACING))
+	(defined(CONFIG_SLAB) && defined(CONFIG_TRACING)) || \
+	(defined(CONFIG_SLOB) && defined(CONFIG_TRACING))
 extern void *__kmalloc_node_track_caller(size_t, gfp_t, int, unsigned long);
 #define kmalloc_node_track_caller(size, flags, node) \
 	__kmalloc_node_track_caller(size, flags, node, \
diff --git a/mm/slob.c b/mm/slob.c
index ae46edc..083959a 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -424,7 +424,8 @@ out:
  * End of slob allocator proper. Begin kmem_cache_alloc and kmalloc frontend.
  */
 
-void *__kmalloc_node(size_t size, gfp_t gfp, int node)
+static __always_inline void *
+__do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
 {
 	unsigned int *m;
 	int align = max(ARCH_KMALLOC_MINALIGN, ARCH_SLAB_MINALIGN);
@@ -445,7 +446,7 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
 		*m = size;
 		ret = (void *)m + align;
 
-		trace_kmalloc_node(_RET_IP_, ret,
+		trace_kmalloc_node(caller, ret,
 				   size, size + align, gfp, node);
 	} else {
 		unsigned int order = get_order(size);
@@ -454,15 +455,35 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
 			gfp |= __GFP_COMP;
 		ret = slob_new_pages(gfp, order, node);
 
-		trace_kmalloc_node(_RET_IP_, ret,
+		trace_kmalloc_node(caller, ret,
 				   size, PAGE_SIZE << order, gfp, node);
 	}
 
 	kmemleak_alloc(ret, size, 1, gfp);
 	return ret;
 }
+
+void *__kmalloc_node(size_t size, gfp_t gfp, int node)
+{
+	return __do_kmalloc_node(size, gfp, node, _RET_IP_);
+}
 EXPORT_SYMBOL(__kmalloc_node);
 
+#ifdef CONFIG_TRACING
+void *__kmalloc_track_caller(size_t size, gfp_t gfp, unsigned long caller)
+{
+	return __do_kmalloc_node(size, gfp, -1, caller);
+}
+
+#ifdef CONFIG_NUMA
+void *__kmalloc_node_track_caller(size_t size, gfp_t gfpflags,
+					int node, unsigned long caller)
+{
+	return __do_kmalloc_node(size, gfp, node, caller);
+}
+#endif
+#endif
+
 void kfree(const void *block)
 {
 	struct page *sp;
-- 
1.7.8.6

--
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>

[PATCH 3/5] mm, util: Do strndup_user allocation directly, instead of through memdup_user

[Ezequiel Garcia]

Since the allocation was being done throug memdup_user, the caller
is wrongly traced as being strndup_user (the correct trace should
report the caller of strndup_user).

This is a common problem: in order to get accurate callsite tracing,
a utils function can't allocate through another utils function,
but instead do the allocation himself.

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
I'm not sure this is the best solution,
but creating another function to reuse between strndup_user
and memdup_user seemed like an overkill.

 mm/util.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/mm/util.c b/mm/util.c
index dc3036c..87ff667 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -214,10 +214,19 @@ char *strndup_user(const char __user *s, long n)
 	if (length > n)
 		return ERR_PTR(-EINVAL);
 
-	p = memdup_user(s, length);
+	/*
+	 * Always use GFP_KERNEL, since copy_from_user() can sleep and
+	 * cause pagefault, which makes it pointless to use GFP_NOFS
+	 * or GFP_ATOMIC.
+	 */
+	p = kmalloc_track_caller(length, GFP_KERNEL);
+	if (!p)
+		return ERR_PTR(-ENOMEM);
 
-	if (IS_ERR(p))
-		return p;
+	if (copy_from_user(p, s, length)) {
+		kfree(p);
+		return ERR_PTR(-EFAULT);
+	}
 
 	p[length - 1] = '\0';
 
-- 
1.7.8.6

--
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>

[PATCH 4/5] mm, slob: Use only 'ret' variable for both slob object and returned pointer

[Ezequiel Garcia]

There's no need to use two variables, 'ret' and 'm'.
This is a minor cleanup patch, but it will allow next patch to clean
the way tracing is done.

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 mm/slob.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/mm/slob.c b/mm/slob.c
index 083959a..3f4dc9a 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -427,7 +427,6 @@ out:
 static __always_inline void *
 __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
 {
-	unsigned int *m;
 	int align = max(ARCH_KMALLOC_MINALIGN, ARCH_SLAB_MINALIGN);
 	void *ret;
 
@@ -439,12 +438,12 @@ __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
 		if (!size)
 			return ZERO_SIZE_PTR;
 
-		m = slob_alloc(size + align, gfp, align, node);
+		ret = slob_alloc(size + align, gfp, align, node);
 
-		if (!m)
+		if (!ret)
 			return NULL;
-		*m = size;
-		ret = (void *)m + align;
+		*(unsigned int *)ret = size;
+		ret += align;
 
 		trace_kmalloc_node(caller, ret,
 				   size, size + align, gfp, node);
-- 
1.7.8.6

--
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>

[PATCH 5/5] mm, slob: Trace allocation failures consistently

[Ezequiel Garcia]

This patch cleans how we trace kmalloc and kmem_cache_alloc.
In particular, it fixes out-of-memory tracing: now every failed
allocation will trace reporting non-zero requested bytes, zero obtained bytes.

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
 mm/slob.c |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/mm/slob.c b/mm/slob.c
index 3f4dc9a..73f16ca 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -428,6 +428,7 @@ static __always_inline void *
 __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
 {
 	int align = max(ARCH_KMALLOC_MINALIGN, ARCH_SLAB_MINALIGN);
+	size_t alloc_size = 0;
 	void *ret;
 
 	gfp &= gfp_allowed_mask;
@@ -441,24 +442,25 @@ __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
 		ret = slob_alloc(size + align, gfp, align, node);
 
 		if (!ret)
-			return NULL;
+			goto trace_out;
 		*(unsigned int *)ret = size;
 		ret += align;
-
-		trace_kmalloc_node(caller, ret,
-				   size, size + align, gfp, node);
+		alloc_size = size + align;
 	} else {
 		unsigned int order = get_order(size);
 
 		if (likely(order))
 			gfp |= __GFP_COMP;
 		ret = slob_new_pages(gfp, order, node);
+		if (!ret)
+			goto trace_out;
 
-		trace_kmalloc_node(caller, ret,
-				   size, PAGE_SIZE << order, gfp, node);
+		alloc_size = PAGE_SIZE << order;
 	}
 
 	kmemleak_alloc(ret, size, 1, gfp);
+trace_out:
+	trace_kmalloc_node(caller, ret, size, alloc_size, gfp, node);
 	return ret;
 }
 
@@ -565,6 +567,7 @@ EXPORT_SYMBOL(kmem_cache_destroy);
 
 void *kmem_cache_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
 {
+	size_t alloc_size = 0;
 	void *b;
 
 	flags &= gfp_allowed_mask;
@@ -573,20 +576,23 @@ void *kmem_cache_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
 
 	if (c->size < PAGE_SIZE) {
 		b = slob_alloc(c->size, flags, c->align, node);
-		trace_kmem_cache_alloc_node(_RET_IP_, b, c->size,
-					    SLOB_UNITS(c->size) * SLOB_UNIT,
-					    flags, node);
+		if (!b)
+			goto trace_out;
+		alloc_size = SLOB_UNITS(c->size) * SLOB_UNIT;
 	} else {
 		b = slob_new_pages(flags, get_order(c->size), node);
-		trace_kmem_cache_alloc_node(_RET_IP_, b, c->size,
-					    PAGE_SIZE << get_order(c->size),
-					    flags, node);
+		if (!b)
+			goto trace_out;
+		alloc_size = PAGE_SIZE << get_order(c->size);
 	}
 
 	if (c->ctor)
 		c->ctor(b);
 
 	kmemleak_alloc_recursive(b, c->size, 1, c->flags, flags);
+trace_out:
+	trace_kmem_cache_alloc_node(_RET_IP_, b, c->size, alloc_size,
+				    flags, node);
 	return b;
 }
 EXPORT_SYMBOL(kmem_cache_alloc_node);
-- 
1.7.8.6

--
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>

Re: [PATCH 1/5] mm, slab: Remove silly function slab_buffer_size()

[David Rientjes]

On Wed, 5 Sep 2012, Ezequiel Garcia wrote:

> This function is seldom used, and can be simply replaced with cachep->size.
> 

You didn't remove the declaration of this function in the header file.

--
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>

Re: [PATCH 2/5] mm, slob: Add support for kmalloc_track_caller()

[David Rientjes]

On Wed, 5 Sep 2012, Ezequiel Garcia wrote:

> @@ -454,15 +455,35 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
>  			gfp |= __GFP_COMP;
>  		ret = slob_new_pages(gfp, order, node);
>  
> -		trace_kmalloc_node(_RET_IP_, ret,
> +		trace_kmalloc_node(caller, ret,
>  				   size, PAGE_SIZE << order, gfp, node);
>  	}
>  
>  	kmemleak_alloc(ret, size, 1, gfp);
>  	return ret;
>  }
> +
> +void *__kmalloc_node(size_t size, gfp_t gfp, int node)
> +{
> +	return __do_kmalloc_node(size, gfp, node, _RET_IP_);
> +}
>  EXPORT_SYMBOL(__kmalloc_node);
>  
> +#ifdef CONFIG_TRACING
> +void *__kmalloc_track_caller(size_t size, gfp_t gfp, unsigned long caller)
> +{
> +	return __do_kmalloc_node(size, gfp, -1, caller);

NUMA_NO_NODE.

> +}
> +
> +#ifdef CONFIG_NUMA
> +void *__kmalloc_node_track_caller(size_t size, gfp_t gfpflags,
> +					int node, unsigned long caller)
> +{
> +	return __do_kmalloc_node(size, gfp, node, caller);
> +}
> +#endif
> +#endif
> +
>  void kfree(const void *block)
>  {
>  	struct page *sp;

--
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>

Re: [PATCH 3/5] mm, util: Do strndup_user allocation directly, instead of through memdup_user

[David Rientjes]

On Wed, 5 Sep 2012, Ezequiel Garcia wrote:

> I'm not sure this is the best solution,
> but creating another function to reuse between strndup_user
> and memdup_user seemed like an overkill.
> 

It's not, so you'd need to do two things to fix this:

 - provide a reason why strndup_user() is special compared to other 
   common library functions that also allocate memory, and

 - provide a __stndup_user() to pass the _RET_IP_.

--
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>

Re: [PATCH 3/5] mm, util: Do strndup_user allocation directly, instead of through memdup_user

[Ezequiel Garcia]

Hi David,

On Wed, Sep 5, 2012 at 9:59 PM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 5 Sep 2012, Ezequiel Garcia wrote:
>
>> I'm not sure this is the best solution,
>> but creating another function to reuse between strndup_user
>> and memdup_user seemed like an overkill.
>>
>
> It's not, so you'd need to do two things to fix this:
>
>  - provide a reason why strndup_user() is special compared to other
>    common library functions that also allocate memory, and
>

Sorry, I don't understand what you mean.
strndup_user is *not* special than any other function, simply if you use
memdup_user for the allocation you will get traces with strndup_user
as the caller,
and that's not desirable.

Thanks for reviewing,
Ezequiel.

--
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>

Re: [PATCH 1/5] mm, slab: Remove silly function slab_buffer_size()

[Ezequiel Garcia]

On Wed, Sep 5, 2012 at 9:54 PM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 5 Sep 2012, Ezequiel Garcia wrote:
>
>> This function is seldom used, and can be simply replaced with cachep->size.
>>
>
> You didn't remove the declaration of this function in the header file.

Yes, you're right.

Thanks for reviewing,
Ezequiel.

--
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>

Re: [PATCH 2/5] mm, slob: Add support for kmalloc_track_caller()

[Ezequiel Garcia]

Hi David,

On Wed, Sep 5, 2012 at 9:57 PM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 5 Sep 2012, Ezequiel Garcia wrote:
>
>> @@ -454,15 +455,35 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
>>                       gfp |= __GFP_COMP;
>>               ret = slob_new_pages(gfp, order, node);
>>
>> -             trace_kmalloc_node(_RET_IP_, ret,
>> +             trace_kmalloc_node(caller, ret,
>>                                  size, PAGE_SIZE << order, gfp, node);
>>       }
>>
>>       kmemleak_alloc(ret, size, 1, gfp);
>>       return ret;
>>  }
>> +
>> +void *__kmalloc_node(size_t size, gfp_t gfp, int node)
>> +{
>> +     return __do_kmalloc_node(size, gfp, node, _RET_IP_);
>> +}
>>  EXPORT_SYMBOL(__kmalloc_node);
>>
>> +#ifdef CONFIG_TRACING
>> +void *__kmalloc_track_caller(size_t size, gfp_t gfp, unsigned long caller)
>> +{
>> +     return __do_kmalloc_node(size, gfp, -1, caller);
>
> NUMA_NO_NODE.
>

Mmm, you bring an interesting issue. If you look at mm/slob.c and
include/linux/slob_def.h
there are lots of places with -1 instead of NUMA_NO_NODE.

Do you think it's worth to prepare a patch fixing all of those?

Thanks,
Ezequiel.

--
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>

Re: [PATCH 2/5] mm, slob: Add support for kmalloc_track_caller()

[Pekka Enberg]

On Wed, 5 Sep 2012, Ezequiel Garcia wrote:
> Mmm, you bring an interesting issue. If you look at mm/slob.c and
> include/linux/slob_def.h
> there are lots of places with -1 instead of NUMA_NO_NODE.
> 
> Do you think it's worth to prepare a patch fixing all of those?

Yes.

			Pekka

--
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>

Re: [PATCH 4/5] mm, slob: Use only 'ret' variable for both slob object and returned pointer

[Christoph Lameter]

On Wed, 5 Sep 2012, Ezequiel Garcia wrote:

> There's no need to use two variables, 'ret' and 'm'.
> This is a minor cleanup patch, but it will allow next patch to clean
> the way tracing is done.

The compiler will fold those variables into one if possible. No need to
worry about having multiple declarations.

>  		if (!size)
>  			return ZERO_SIZE_PTR;
>
> -		m = slob_alloc(size + align, gfp, align, node);
> +		ret = slob_alloc(size + align, gfp, align, node);
>
> -		if (!m)
> +		if (!ret)
>  			return NULL;
> -		*m = size;
> -		ret = (void *)m + align;
> +		*(unsigned int *)ret = size;

An ugly cast...

--
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>

Re: [PATCH 4/5] mm, slob: Use only 'ret' variable for both slob object and returned pointer

[Ezequiel Garcia]

Hi Christoph,

On Thu, Sep 6, 2012 at 11:18 AM, Christoph Lameter <cl@linux.com> wrote:
> On Wed, 5 Sep 2012, Ezequiel Garcia wrote:
>
>> There's no need to use two variables, 'ret' and 'm'.
>> This is a minor cleanup patch, but it will allow next patch to clean
>> the way tracing is done.
>
> The compiler will fold those variables into one if possible. No need to
> worry about having multiple declarations.
>

I wasn't worried about size or anything, it's a just a clean-n-prepare patch,
necesarry for the next patch:

"mm, slob: Trace allocation failures consistently"

Could you take a look at it?

Thanks,
Ezequiel.

--
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>

Re: [PATCH 5/5] mm, slob: Trace allocation failures consistently

[JoonSoo Kim]

2012/9/6 Ezequiel Garcia <elezegarcia@gmail.com>:
> This patch cleans how we trace kmalloc and kmem_cache_alloc.
> In particular, it fixes out-of-memory tracing: now every failed
> allocation will trace reporting non-zero requested bytes, zero obtained bytes.

Other SLAB allocators(slab, slub) doesn't consider zero obtained bytes
in tracing.
These just return "addr = 0, obtained size = cache size"
Why does the slob print a different output?

> @@ -573,20 +576,23 @@ void *kmem_cache_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
>
>         if (c->size < PAGE_SIZE) {
>                 b = slob_alloc(c->size, flags, c->align, node);
> -               trace_kmem_cache_alloc_node(_RET_IP_, b, c->size,
> -                                           SLOB_UNITS(c->size) * SLOB_UNIT,
> -                                           flags, node);
> +               if (!b)
> +                       goto trace_out;
> +               alloc_size = SLOB_UNITS(c->size) * SLOB_UNIT;
>         } else {
>                 b = slob_new_pages(flags, get_order(c->size), node);
> -               trace_kmem_cache_alloc_node(_RET_IP_, b, c->size,
> -                                           PAGE_SIZE << get_order(c->size),
> -                                           flags, node);
> +               if (!b)
> +                       goto trace_out;
> +               alloc_size = PAGE_SIZE << get_order(c->size);
>         }
>         if (c->ctor)
>                 c->ctor(b);

Regardless of tracing, "if (!b)" test is needed for skip "c->ctor".

--
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>

Re: [PATCH 3/5] mm, util: Do strndup_user allocation directly, instead of through memdup_user

[JoonSoo Kim]

2012/9/6 Ezequiel Garcia <elezegarcia@gmail.com>:
> Hi David,
>
> On Wed, Sep 5, 2012 at 9:59 PM, David Rientjes <rientjes@google.com> wrote:
>> On Wed, 5 Sep 2012, Ezequiel Garcia wrote:
>>
>>> I'm not sure this is the best solution,
>>> but creating another function to reuse between strndup_user
>>> and memdup_user seemed like an overkill.
>>>
>>
>> It's not, so you'd need to do two things to fix this:
>>
>>  - provide a reason why strndup_user() is special compared to other
>>    common library functions that also allocate memory, and
>>
>
> Sorry, I don't understand what you mean.
> strndup_user is *not* special than any other function, simply if you use
> memdup_user for the allocation you will get traces with strndup_user
> as the caller,
> and that's not desirable.

I'm not sure that this changed should be needed.
But, if you want to fix this properly, why don't change __krealloc() ?
It is called by krealloc(), and may return krealloc()'s address.

--
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>

Re: [PATCH 3/5] mm, util: Do strndup_user allocation directly, instead of through memdup_user

[Ezequiel Garcia]

Hi Joonsoo,

On Thu, Sep 6, 2012 at 4:27 PM, JoonSoo Kim <js1304@gmail.com> wrote:
> 2012/9/6 Ezequiel Garcia <elezegarcia@gmail.com>:
>> Hi David,
>>
>> On Wed, Sep 5, 2012 at 9:59 PM, David Rientjes <rientjes@google.com> wrote:
>>> On Wed, 5 Sep 2012, Ezequiel Garcia wrote:
>>>
>>>> I'm not sure this is the best solution,
>>>> but creating another function to reuse between strndup_user
>>>> and memdup_user seemed like an overkill.
>>>>
>>>
>>> It's not, so you'd need to do two things to fix this:
>>>
>>>  - provide a reason why strndup_user() is special compared to other
>>>    common library functions that also allocate memory, and
>>>
>>
>> Sorry, I don't understand what you mean.
>> strndup_user is *not* special than any other function, simply if you use
>> memdup_user for the allocation you will get traces with strndup_user
>> as the caller,
>> and that's not desirable.
>
> I'm not sure that this changed should be needed.

Why do you think this?

> But, if you want to fix this properly, why don't change __krealloc() ?
> It is called by krealloc(), and may return krealloc()'s address.

That's already fixed and applied on Pekka's tree, it's this one:
mm: Use __do_krealloc to do the krealloc job

I think this kind of issues are important, yet overlooked, for kmem
tracing to become
useful. There's a reason we have kmalloc_track_caller, and it would be nice
to have them all trace properly.

Regards,
Ezequiel.

--
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>

Re: [PATCH 5/5] mm, slob: Trace allocation failures consistently

[Ezequiel Garcia]

Hi Joonso,

On Thu, Sep 6, 2012 at 4:09 PM, JoonSoo Kim <js1304@gmail.com> wrote:
> 2012/9/6 Ezequiel Garcia <elezegarcia@gmail.com>:
>> This patch cleans how we trace kmalloc and kmem_cache_alloc.
>> In particular, it fixes out-of-memory tracing: now every failed
>> allocation will trace reporting non-zero requested bytes, zero obtained bytes.
>
> Other SLAB allocators(slab, slub) doesn't consider zero obtained bytes
> in tracing.
> These just return "addr = 0, obtained size = cache size"
> Why does the slob print a different output?
>

I plan to fix slab, slub in a future patchset. I think it would be nice to have
a trace event reporting this event. But, perhaps it's not worth it.

I'd love to hear what others think about this.

Thanks,
Ezequiel.

--
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>

Re: [PATCH 3/5] mm, util: Do strndup_user allocation directly, instead of through memdup_user

[JoonSoo Kim]

Hi, Ezequiel.

2012/9/7 Ezequiel Garcia <elezegarcia@gmail.com>:
>> But, if you want to fix this properly, why don't change __krealloc() ?
>> It is called by krealloc(), and may return krealloc()'s address.
>
> That's already fixed and applied on Pekka's tree, it's this one:
> mm: Use __do_krealloc to do the krealloc job

Okay. Thanks.

--
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>

Re: [PATCH 5/5] mm, slob: Trace allocation failures consistently

[JoonSoo Kim]

Hi, Ezequiel.

2012/9/7 Ezequiel Garcia <elezegarcia@gmail.com>:
> Hi Joonso,
>
> On Thu, Sep 6, 2012 at 4:09 PM, JoonSoo Kim <js1304@gmail.com> wrote:
>> 2012/9/6 Ezequiel Garcia <elezegarcia@gmail.com>:
>>> This patch cleans how we trace kmalloc and kmem_cache_alloc.
>>> In particular, it fixes out-of-memory tracing: now every failed
>>> allocation will trace reporting non-zero requested bytes, zero obtained bytes.
>>
>> Other SLAB allocators(slab, slub) doesn't consider zero obtained bytes
>> in tracing.
>> These just return "addr = 0, obtained size = cache size"
>> Why does the slob print a different output?
>>
>
> I plan to fix slab, slub in a future patchset. I think it would be nice to have
> a trace event reporting this event. But, perhaps it's not worth it.

I think that output "addr =  0" is sufficient to trace out-of-memory situation.
Why do we need a output "addr = 0, obtained size = 0"?

Thanks.

--
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>

Re: [PATCH 5/5] mm, slob: Trace allocation failures consistently

[JoonSoo Kim]

2012/9/6 Ezequiel Garcia <elezegarcia@gmail.com>:
> This patch cleans how we trace kmalloc and kmem_cache_alloc.
> In particular, it fixes out-of-memory tracing: now every failed
> allocation will trace reporting non-zero requested bytes, zero obtained bytes.
>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: Christoph Lameter <cl@linux.com>
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
>  mm/slob.c |   30 ++++++++++++++++++------------
>  1 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/mm/slob.c b/mm/slob.c
> index 3f4dc9a..73f16ca 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -428,6 +428,7 @@ static __always_inline void *
>  __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
>  {
>         int align = max(ARCH_KMALLOC_MINALIGN, ARCH_SLAB_MINALIGN);
> +       size_t alloc_size = 0;
>         void *ret;
>
>         gfp &= gfp_allowed_mask;
> @@ -441,24 +442,25 @@ __do_kmalloc_node(size_t size, gfp_t gfp, int node, unsigned long caller)
>                 ret = slob_alloc(size + align, gfp, align, node);
>
>                 if (!ret)
> -                       return NULL;
> +                       goto trace_out;
>                 *(unsigned int *)ret = size;
>                 ret += align;
> -
> -               trace_kmalloc_node(caller, ret,
> -                                  size, size + align, gfp, node);
> +               alloc_size = size + align;
>         } else {
>                 unsigned int order = get_order(size);
>
>                 if (likely(order))
>                         gfp |= __GFP_COMP;
>                 ret = slob_new_pages(gfp, order, node);
> +               if (!ret)
> +                       goto trace_out;
>
> -               trace_kmalloc_node(caller, ret,
> -                                  size, PAGE_SIZE << order, gfp, node);
> +               alloc_size = PAGE_SIZE << order;
>         }
>
>         kmemleak_alloc(ret, size, 1, gfp);
> +trace_out:
> +       trace_kmalloc_node(caller, ret, size, alloc_size, gfp, node);
>         return ret;
>  }
>
> @@ -565,6 +567,7 @@ EXPORT_SYMBOL(kmem_cache_destroy);
>
>  void *kmem_cache_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
>  {
> +       size_t alloc_size = 0;
>         void *b;
>
>         flags &= gfp_allowed_mask;
> @@ -573,20 +576,23 @@ void *kmem_cache_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
>
>         if (c->size < PAGE_SIZE) {
>                 b = slob_alloc(c->size, flags, c->align, node);
> -               trace_kmem_cache_alloc_node(_RET_IP_, b, c->size,
> -                                           SLOB_UNITS(c->size) * SLOB_UNIT,
> -                                           flags, node);
> +               if (!b)
> +                       goto trace_out;
> +               alloc_size = SLOB_UNITS(c->size) * SLOB_UNIT;
>         } else {
>                 b = slob_new_pages(flags, get_order(c->size), node);
> -               trace_kmem_cache_alloc_node(_RET_IP_, b, c->size,
> -                                           PAGE_SIZE << get_order(c->size),
> -                                           flags, node);
> +               if (!b)
> +                       goto trace_out;
> +               alloc_size = PAGE_SIZE << get_order(c->size);
>         }
>
>         if (c->ctor)
>                 c->ctor(b);
>
>         kmemleak_alloc_recursive(b, c->size, 1, c->flags, flags);
> +trace_out:
> +       trace_kmem_cache_alloc_node(_RET_IP_, b, c->size, alloc_size,
> +                                   flags, node);
>         return b;
>  }
>  EXPORT_SYMBOL(kmem_cache_alloc_node);
> --
> 1.7.8.6
>
> --
> 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>

If we don't enable tracing, "unused variable warning" may occurs.

--
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>

Re: [PATCH 5/5] mm, slob: Trace allocation failures consistently

[JoonSoo Kim]

>> 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>
>
> If we don't enable tracing, "unused variable warning" may occurs.

Oops, Sorry. Mistake.

--
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>

Re: [PATCH 5/5] mm, slob: Trace allocation failures consistently

[Ezequiel Garcia]

On Fri, Sep 7, 2012 at 6:23 PM, JoonSoo Kim <js1304@gmail.com> wrote:
> Hi, Ezequiel.
>
> 2012/9/7 Ezequiel Garcia <elezegarcia@gmail.com>:
>> Hi Joonso,
>>
>> On Thu, Sep 6, 2012 at 4:09 PM, JoonSoo Kim <js1304@gmail.com> wrote:
>>> 2012/9/6 Ezequiel Garcia <elezegarcia@gmail.com>:
>>>> This patch cleans how we trace kmalloc and kmem_cache_alloc.
>>>> In particular, it fixes out-of-memory tracing: now every failed
>>>> allocation will trace reporting non-zero requested bytes, zero obtained bytes.
>>>
>>> Other SLAB allocators(slab, slub) doesn't consider zero obtained bytes
>>> in tracing.
>>> These just return "addr = 0, obtained size = cache size"
>>> Why does the slob print a different output?
>>>
>>
>> I plan to fix slab, slub in a future patchset. I think it would be nice to have
>> a trace event reporting this event. But, perhaps it's not worth it.
>
> I think that output "addr =  0" is sufficient to trace out-of-memory situation.
> Why do we need a output "addr = 0, obtained size = 0"?
>

You're absolutely right.

Thanks,
Ezequiel.

--
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>