[PATCH] slab: introduce kmem_cache_zalloc allocator

[PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

This patch introduces a memory-zeroing variant of kmem_cache_alloc. The
allocator already exits in XFS and there are potential users for it so
this patch makes the allocator available for the general public.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>

---

 include/linux/slab.h |    2 ++
 mm/slab.c            |   17 +++++++++++++++++
 mm/slob.c            |   10 ++++++++++
 3 files changed, 29 insertions(+), 0 deletions(-)

7ebeed21971a6a24749a4966db1ccc69c6806e15
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8cf5293..b595c09 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -64,6 +64,7 @@ extern kmem_cache_t *kmem_cache_create(c
 extern int kmem_cache_destroy(kmem_cache_t *);
 extern int kmem_cache_shrink(kmem_cache_t *);
 extern void *kmem_cache_alloc(kmem_cache_t *, gfp_t);
+extern void *kmem_cache_zalloc(struct kmem_cache *, gfp_t);
 extern void kmem_cache_free(kmem_cache_t *, void *);
 extern unsigned int kmem_cache_size(kmem_cache_t *);
 extern const char *kmem_cache_name(kmem_cache_t *);
@@ -155,6 +156,7 @@ struct kmem_cache *kmem_cache_create(con
 	void (*)(void *, struct kmem_cache *, unsigned long));
 int kmem_cache_destroy(struct kmem_cache *c);
 void *kmem_cache_alloc(struct kmem_cache *c, gfp_t flags);
+void *kmem_cache_zalloc(struct kmem_cache *, gfp_t);
 void kmem_cache_free(struct kmem_cache *c, void *b);
 const char *kmem_cache_name(struct kmem_cache *);
 void *kmalloc(size_t size, gfp_t flags);
diff --git a/mm/slab.c b/mm/slab.c
index d0bd7f0..5f3e14b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3056,6 +3056,23 @@ void *kmem_cache_alloc(struct kmem_cache
 EXPORT_SYMBOL(kmem_cache_alloc);
 
 /**
+ * kmem_cache_alloc - Allocate an object. The memory is set to zero.
+ * @cache: The cache to allocate from.
+ * @flags: See kmalloc().
+ *
+ * Allocate an object from this cache and set the allocated memory to zero.
+ * The flags are only relevant if the cache has no available objects.
+ */
+void *kmem_cache_zalloc(struct kmem_cache *cache, gfp_t flags)
+{
+	void *ret = __cache_alloc(cache, flags, __builtin_return_address(0));
+	if (ret)
+		memset(ret, 0, obj_size(cache));
+	return ret;
+}
+EXPORT_SYMBOL(kmem_cache_zalloc);
+
+/**
  * kmem_ptr_validate - check if an untrusted pointer might
  *	be a slab entry.
  * @cachep: the cache we're checking against
diff --git a/mm/slob.c b/mm/slob.c
index a1f42bd..9bcc7e2 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -294,6 +294,16 @@ void *kmem_cache_alloc(struct kmem_cache
 }
 EXPORT_SYMBOL(kmem_cache_alloc);
 
+void *kmem_cache_zalloc(struct kmem_cache *c, gfp_t flags)
+{
+	void *ret = kmem_cache_alloc(c, flags);
+	if (ret)
+		memset(ret, 0, c->size);
+
+	return ret;
+}
+EXPORT_SYMBOL(kmem_cache_zalloc);
+
 void kmem_cache_free(struct kmem_cache *c, void *b)
 {
 	if (c->dtor)
-- 
1.2.3

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Eric Dumazet]

Pekka J Enberg a écrit :
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> This patch introduces a memory-zeroing variant of kmem_cache_alloc. The
> allocator already exits in XFS and there are potential users for it so
> this patch makes the allocator available for the general public.
> 

Excellent.

Please change zalloc() so that a zalloc(constant_value) uses your 
kmem_cache_zalloc on the appropriate cache.

This way we can really introduce zalloc() *everywhere* without paying the cost 
of runtime lookup to find the right cache.

Eric

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka J Enberg]

On Mon, 20 Mar 2006, Eric Dumazet wrote:
> Excellent.
> 
> Please change zalloc() so that a zalloc(constant_value) uses your
> kmem_cache_zalloc on the appropriate cache.
> 
> This way we can really introduce zalloc() *everywhere* without paying the cost
> of runtime lookup to find the right cache.

Something like this? For some reason, the below increases kernel text.

				Pekka

diff --git a/include/linux/slab.h b/include/linux/slab.h
index b595c09..db3b302 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -109,7 +109,30 @@ found:
 	return __kmalloc(size, flags);
 }
 
-extern void *kzalloc(size_t, gfp_t);
+extern void *__kzalloc(size_t, gfp_t);
+
+static inline void *kzalloc(size_t size, gfp_t flags)
+{
+	if (__builtin_constant_p(size)) {
+		int i = 0;
+#define CACHE(x) \
+		if (size <= x) \
+			goto found; \
+		else \
+			i++;
+#include "kmalloc_sizes.h"
+#undef CACHE
+		{
+			extern void __you_cannot_kzalloc_that_much(void);
+			__you_cannot_kzalloc_that_much();
+		}
+found:
+		return kmem_cache_zalloc((flags & GFP_DMA) ?
+			malloc_sizes[i].cs_dmacachep :
+			malloc_sizes[i].cs_cachep, flags);
+	}
+	return __kzalloc(size, flags);
+}
 
 /**
  * kcalloc - allocate memory for an array. The memory is set to zero.
@@ -160,14 +183,14 @@ void *kmem_cache_zalloc(struct kmem_cach
 void kmem_cache_free(struct kmem_cache *c, void *b);
 const char *kmem_cache_name(struct kmem_cache *);
 void *kmalloc(size_t size, gfp_t flags);
-void *kzalloc(size_t size, gfp_t flags);
+void *__kzalloc(size_t size, gfp_t flags);
 void kfree(const void *m);
 unsigned int ksize(const void *m);
 unsigned int kmem_cache_size(struct kmem_cache *c);
 
 static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
 {
-	return kzalloc(n * size, flags);
+	return __kzalloc(n * size, flags);
 }
 
 #define kmem_cache_shrink(d) (0)
@@ -175,6 +198,7 @@ static inline void *kcalloc(size_t n, si
 #define kmem_ptr_validate(a, b) (0)
 #define kmem_cache_alloc_node(c, f, n) kmem_cache_alloc(c, f)
 #define kmalloc_node(s, f, n) kmalloc(s, f)
+#define kzalloc(s, f) __kzalloc(s, f)
 
 #endif /* CONFIG_SLOB */
 
diff --git a/mm/util.c b/mm/util.c
index 5f4bb59..fd78ee4 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -3,18 +3,18 @@
 #include <linux/module.h>
 
 /**
- * kzalloc - allocate memory. The memory is set to zero.
+ * __kzalloc - allocate memory. The memory is set to zero.
  * @size: how many bytes of memory are required.
  * @flags: the type of memory to allocate.
  */
-void *kzalloc(size_t size, gfp_t flags)
+void *__kzalloc(size_t size, gfp_t flags)
 {
 	void *ret = kmalloc(size, flags);
 	if (ret)
 		memset(ret, 0, size);
 	return ret;
 }
-EXPORT_SYMBOL(kzalloc);
+EXPORT_SYMBOL(__kzalloc);
 
 /*
  * kstrdup - allocate space for and copy an existing string

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Balbir Singh]

<snip>

>  /**
> + * kmem_cache_alloc - Allocate an object. The memory is set to zero.
> + * @cache: The cache to allocate from.
> + * @flags: See kmalloc().
> + *
> + * Allocate an object from this cache and set the allocated memory to zero.
> + * The flags are only relevant if the cache has no available objects.
> + */
> +void *kmem_cache_zalloc(struct kmem_cache *cache, gfp_t flags)
> +{
> +	void *ret = __cache_alloc(cache, flags, __builtin_return_address(0));
> +	if (ret)
> +		memset(ret, 0, obj_size(cache));
> +	return ret;
> +}
> +EXPORT_SYMBOL(kmem_cache_zalloc);
> +
> +/**
>   * kmem_ptr_validate - check if an untrusted pointer might
>   *	be a slab entry.
>   * @cachep: the cache we're checking against
> diff --git a/mm/slob.c b/mm/slob.c
> index a1f42bd..9bcc7e2 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -294,6 +294,16 @@ void *kmem_cache_alloc(struct kmem_cache
>  }
>  EXPORT_SYMBOL(kmem_cache_alloc);
>  
> +void *kmem_cache_zalloc(struct kmem_cache *c, gfp_t flags)
> +{
> +	void *ret = kmem_cache_alloc(c, flags);
> +	if (ret)
> +		memset(ret, 0, c->size);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(kmem_cache_zalloc);
> +
>  void kmem_cache_free(struct kmem_cache *c, void *b)
>  {
>  	if (c->dtor)

Could we please create a more generic variation of this patch -- may be a
function called kmem_cache_alloc_set(). The function would not only
memset the data to 0, but instead to any specified pattern passed as
an argument.

This could be used to poison allocated memory. Passing 0 would make
this equivalent to kmem_cache_zalloc(). Basically, instead of doing

mem = __cache_alloc(...)
memset(mem, 0, size)

I would prefer if we could have

mem = __cache_alloc(...)
memset(mem, X, size)

Balbir

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka Enberg]

On Mon, 2006-03-20 at 21:35 +0530, Balbir Singh wrote:
> Could we please create a more generic variation of this patch -- may be a
> function called kmem_cache_alloc_set(). The function would not only
> memset the data to 0, but instead to any specified pattern passed as
> an argument.

No, no, no! I am introducing kmem_cache_zalloc() because there are
existing users in the tree. I plan to kill the slab wrappers from XFS
completely which is why I need this. We already have object constructors
for what you're describing.

On Mon, 2006-03-20 at 21:35 +0530, Balbir Singh wrote:
> This could be used to poison allocated memory. Passing 0 would make
> this equivalent to kmem_cache_zalloc(). Basically, instead of doing

I am not sure I understand what you mean. We already have slab poisoning
and that's in mm/slab.c. Why would you want to make the callers aware of
that?

				Pekka

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Balbir Singh]

> No, no, no! I am introducing kmem_cache_zalloc() because there are
> existing users in the tree. I plan to kill the slab wrappers from XFS
> completely which is why I need this. We already have object constructors
> for what you're describing.

Ok, please keep the interface - build kmem_cache_zalloc() on top of
what I suggest.

>
> On Mon, 2006-03-20 at 21:35 +0530, Balbir Singh wrote:
> > This could be used to poison allocated memory. Passing 0 would make
> > this equivalent to kmem_cache_zalloc(). Basically, instead of doing
>
> I am not sure I understand what you mean. We already have slab poisoning
> and that's in mm/slab.c. Why would you want to make the callers aware of
> that?
>

 Basically I am not asking for poisoning support as described by
slab.c. I am talking about general poisoining. Let me try and further
clarify with an example.

Lets say I have a structure resp that is allocated on heap. It is a
part of a response structure for a device (say 1394) and is returned
by the device to the driver.

When I allocate the structure - I would like to do

kmem_cache_alloc_set(&resp, GFP_XXXXX, 0xEE)

The device should ideally fill all fields of resp. Fields that look
0xEE after receiving the response -- would indicate that they were not
filled by the device. This would be extremely useful in debugging.
With kmem_cache_zalloc() - 0 is usually almost always a valid value.
It is useful in some cases and no so much in other cases.

I could easily achieve the same thing by doing a

memset(&resp, 0xEE, size)

after the kmem_cache_alloc(). But since there is an API to zero out
allocated memory, I thought we could make it more generic and more
useful.

Lets say we add an API

kmem_cache_alloc_set(cache, flags, byte)
{
    mem = __cache_alloc(...)
    memset(mem, byte, size)
}

we could have

inline kmem_cache_zalloc(cache, flags)
{
   return kmem_cache_alloc_set(cache, flags, 0)
}

Balbir

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka J Enberg]

On Mon, 20 Mar 2006, Balbir Singh wrote:
> When I allocate the structure - I would like to do
> 
> kmem_cache_alloc_set(&resp, GFP_XXXXX, 0xEE)
> 
> The device should ideally fill all fields of resp. Fields that look
> 0xEE after receiving the response -- would indicate that they were not
> filled by the device. This would be extremely useful in debugging.
> With kmem_cache_zalloc() - 0 is usually almost always a valid value.
> It is useful in some cases and no so much in other cases.
> 
> I could easily achieve the same thing by doing a
> 
> memset(&resp, 0xEE, size)
> 
> after the kmem_cache_alloc(). But since there is an API to zero out
> allocated memory, I thought we could make it more generic and more
> useful.

Yeah, but if it's a debugging thing, I don't see much point in adding yet 
another API call. The main point in introducing kmem_cache_zalloc() is to 
move existing API into slab proper.

				Pekka

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Andrew Morton]

Pekka J Enberg <penberg@cs.Helsinki.FI> wrote:
>
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> This patch introduces a memory-zeroing variant of kmem_cache_alloc. The
> allocator already exits in XFS and there are potential users for it so
> this patch makes the allocator available for the general public.
> 

hmm.

> + * kmem_cache_alloc - Allocate an object. The memory is set to zero.
> + * @cache: The cache to allocate from.
> + * @flags: See kmalloc().
> + *
> + * Allocate an object from this cache and set the allocated memory to zero.
> + * The flags are only relevant if the cache has no available objects.
> + */
> +void *kmem_cache_zalloc(struct kmem_cache *cache, gfp_t flags)
> +{
> +	void *ret = __cache_alloc(cache, flags, __builtin_return_address(0));
> +	if (ret)
> +		memset(ret, 0, obj_size(cache));
> +	return ret;
> +}
> +EXPORT_SYMBOL(kmem_cache_zalloc);

The way this is supposed to work in slab is that the owner of the cache
provides a constructor which zeroes out newly-allocated storage and the
owner of the cache is supposed to free objects in a constructed (ie:
zeroed) state.

I've always felt that this was an odd design.  Because

a) All that cache-warmth which we get from the constructor's zeroing can
   be lost by the time we get around to using an individual object and

b) The object may be cache-cold by the time we free it, and we'll take
   cache misses just putting it back into a constructed state for
   kmem_cache_free().  And we'll lose that cache warmth by the time we use
   this object again.

So from that POV I think (in my simple way) that this is a good patch.  But
IIRC, Manfred has reasons why it might not be?

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka J Enberg]

Hi Andrew,

On Tue, 21 Mar 2006, Andrew Morton wrote:
> The way this is supposed to work in slab is that the owner of the cache
> provides a constructor which zeroes out newly-allocated storage and the
> owner of the cache is supposed to free objects in a constructed (ie:
> zeroed) state.

Constructors work well for things like lock and and list initialization 
where the object state is naturally in 'initial' state when free'd. They 
don't work well for unconditional zeroing for the exact reasons you 
outline below.

On Tue, 21 Mar 2006, Andrew Morton wrote:
> I've always felt that this was an odd design.  Because
> 
> a) All that cache-warmth which we get from the constructor's zeroing can
>    be lost by the time we get around to using an individual object and
> 
> b) The object may be cache-cold by the time we free it, and we'll take
>    cache misses just putting it back into a constructed state for
>    kmem_cache_free().  And we'll lose that cache warmth by the time we use
>    this object again.
> 
> So from that POV I think (in my simple way) that this is a good patch.  But
> IIRC, Manfred has reasons why it might not be?

I assume the design comes from Bonwick's paper which states that the 
purpose of object constructor is to support one-time initialization of 
objects which we're _not_ doing in this case.

				Pekka 

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Manfred Spraul]

Pekka J Enberg wrote:

>On Tue, 21 Mar 2006, Andrew Morton wrote:
>  
>
>>I've always felt that this was an odd design.  Because
>>
>>a) All that cache-warmth which we get from the constructor's zeroing can
>>   be lost by the time we get around to using an individual object and
>>
>>b) The object may be cache-cold by the time we free it, and we'll take
>>   cache misses just putting it back into a constructed state for
>>   kmem_cache_free().  And we'll lose that cache warmth by the time we use
>>   this object again.
>>
>>So from that POV I think (in my simple way) that this is a good patch.  But
>>IIRC, Manfred has reasons why it might not be?
>>    
>>
>
>I assume the design comes from Bonwick's paper which states that the 
>purpose of object constructor is to support one-time initialization of 
>objects which we're _not_ doing in this case.
>
>  
>
I agree - memset just before use is the Right Thing (tm).

One minor point: There are two cache_alloc entry points: __cache_alloc, 
which is a forced inline function, and kmem_cache_alloc, which is just a 
wrapper for __cache_alloc. Is it really necessary to call __cache_alloc?
The idea is that __cache_alloc is used just by the two high-performance 
paths: kmem_cache_alloc and kmalloc. Noone else should use __cache_alloc 
directly.

--
    Manfred

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka Enberg]

On Tue, 2006-03-21 at 19:35 +0100, Manfred Spraul wrote:
> One minor point: There are two cache_alloc entry points: __cache_alloc, 
> which is a forced inline function, and kmem_cache_alloc, which is just a 
> wrapper for __cache_alloc. Is it really necessary to call __cache_alloc?

Unfortunately, yes. Using kmem_cache_alloc screws up caller tracing.

				Pekka

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Andrew Morton]

Pekka J Enberg <penberg@cs.Helsinki.FI> wrote:
>
> This patch introduces a memory-zeroing variant of kmem_cache_alloc.
>

Problem is, after I've weathered 10000000000 convert-to-kmem_cache_zalloc
patches, those pestiferous NUMA people are going to come along wanting
kmem_cache_kzalloc_node().

Probably this should be designed for up-front?

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka J Enberg]

On Tue, 21 Mar 2006, Andrew Morton wrote:
> Problem is, after I've weathered 10000000000 convert-to-kmem_cache_zalloc
> patches, those pestiferous NUMA people are going to come along wanting
> kmem_cache_kzalloc_node().
> 
> Probably this should be designed for up-front?

I can send you one unless someone beats me to it ;-).

				Pekka

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka Enberg]

Pekka J Enberg <penberg@cs.Helsinki.FI> wrote:
> >
> > This patch introduces a memory-zeroing variant of kmem_cache_alloc.
> >

On Tue, 2006-03-21 at 03:25 -0800, Andrew Morton wrote:
> Problem is, after I've weathered 10000000000 convert-to-kmem_cache_zalloc
> patches, those pestiferous NUMA people are going to come along wanting
> kmem_cache_kzalloc_node().
> 
> Probably this should be designed for up-front?

Actually, we don't even have kzalloc_node, so I'd say we're better of
waiting for the NUMA folk to ask for them. Hum?

				Pekka

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Bodo Eggert]

Balbir Singh <balbir@in.ibm.com> wrote:

>> No, no, no! I am introducing kmem_cache_zalloc() because there are
>> existing users in the tree. I plan to kill the slab wrappers from XFS
>> completely which is why I need this. We already have object constructors
>> for what you're describing.
> 
> Ok, please keep the interface - build kmem_cache_zalloc() on top of
> what I suggest.

The benefit of using *zalloc is the ability to skip the memset by using
pre-zeroed memory or to use more efficient ways of zeroing a page.
Having to check the value of a parameter wouldn't help.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Balbir Singh]

> > Ok, please keep the interface - build kmem_cache_zalloc() on top of
> > what I suggest.
> 
> The benefit of using *zalloc is the ability to skip the memset by using
> pre-zeroed memory or to use more efficient ways of zeroing a page.
> Having to check the value of a parameter wouldn't help.

Hmm... the current patch directly does a memset(). Are you talking about
possible optimizations to kmem_cache_zalloc()?

Balbir

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Bodo Eggert]

On Tue, 21 Mar 2006, Balbir Singh wrote:

> > > Ok, please keep the interface - build kmem_cache_zalloc() on top of
> > > what I suggest.
> > 
> > The benefit of using *zalloc is the ability to skip the memset by using
> > pre-zeroed memory or to use more efficient ways of zeroing a page.
> > Having to check the value of a parameter wouldn't help.
> 
> Hmm... the current patch directly does a memset(). Are you talking about
> possible optimizations to kmem_cache_zalloc()?

Yes. At least that's what I understand from the whole zalloc process.
-- 
Top 100 things you don't want the sysadmin to say:
4. This won't affect what you're doing.

Re: [PATCH] slab: introduce kmem_cache_zalloc allocator

[Pekka J Enberg]

On Tue, 21 Mar 2006, Bodo Eggert wrote:
> Yes. At least that's what I understand from the whole zalloc process.

Zalloc does two things: (1) reduce human error and (2) shrink kernel text 
size. Pre-zeroing is a possible optimization but needs to be measured. It 
is not necessarily a positive gain due to cache effects.

				Pekka