[PATCH 1/3] slab: introduce krealloc

[PATCH 1/3] slab: introduce krealloc

[Pekka J Enberg]

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

Introduce krealloc() for reallocating memory while preserving contents.

Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 include/linux/slab.h |    1 +
 mm/util.c            |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

Index: 2.6/include/linux/slab.h
===================================================================
--- 2.6.orig/include/linux/slab.h	2007-02-21 09:05:08.000000000 +0200
+++ 2.6/include/linux/slab.h	2007-02-21 09:05:47.000000000 +0200
@@ -72,6 +72,7 @@
  */
 void *__kmalloc(size_t, gfp_t);
 void *__kzalloc(size_t, gfp_t);
+void *krealloc(const void *, size_t, gfp_t);
 void kfree(const void *);
 unsigned int ksize(const void *);
 
Index: 2.6/mm/util.c
===================================================================
--- 2.6.orig/mm/util.c	2007-02-21 09:05:49.000000000 +0200
+++ 2.6/mm/util.c	2007-02-21 09:34:32.000000000 +0200
@@ -18,6 +18,40 @@
 }
 EXPORT_SYMBOL(__kzalloc);
 
+/**
+ * krealloc - reallocate memory. The contents will remain unchanged.
+ *
+ * @p: object to reallocate memory for.
+ * @new_size: how many bytes of memory are required.
+ * @flags: the type of memory to allocate.
+ *
+ * The contents of the object pointed to are preserved up to the
+ * lesser of the new and old sizes.  If @p is %NULL, krealloc()
+ * behaves exactly like kmalloc().  If @size is 0 and @p is not a
+ * %NULL pointer, the object pointed to is freed.
+ */
+void *krealloc(const void *p, size_t new_size, gfp_t flags)
+{
+	void *ret;
+
+	if (unlikely(!p))
+		return kmalloc_track_caller(new_size, flags);
+
+	if (unlikely(!new_size)) {
+		kfree(p);
+		return NULL;
+	}
+
+	ret = kmalloc_track_caller(new_size, flags);
+	if (!ret)
+		return NULL;
+
+	memcpy(ret, p, min(new_size, ksize(p)));
+	kfree(p);
+	return ret;
+}
+EXPORT_SYMBOL(krealloc);
+
 /*
  * kstrdup - allocate space for and copy an existing string
  *

Re: [PATCH 1/3] slab: introduce krealloc

[Arjan van de Ven]

On Wed, 2007-02-21 at 10:06 +0200, Pekka J Enberg wrote:
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> Introduce krealloc() for reallocating memory while preserving contents.


please mark this one __must_check.. not storing realloc() return values
is one of the more nasty types of bugs... but gcc can help us greatly
here ;)

Re: [PATCH 1/3] slab: introduce krealloc

[Pekka Enberg]

On 2/21/07, Arjan van de Ven <arjan@infradead.org> wrote:
> please mark this one __must_check.. not storing realloc() return values
> is one of the more nasty types of bugs... but gcc can help us greatly
> here ;)

So I guess we want the same thing for the other allocator functions
(__kmalloc et al) as well?

Re: [PATCH 1/3] slab: introduce krealloc

[Arjan van de Ven]

On Wed, 2007-02-21 at 11:33 +0200, Pekka Enberg wrote:
> On 2/21/07, Arjan van de Ven <arjan@infradead.org> wrote:
> > please mark this one __must_check.. not storing realloc() return values
> > is one of the more nasty types of bugs... but gcc can help us greatly
> > here ;)
> 
> So I guess we want the same thing for the other allocator functions
> (__kmalloc et al) as well?

kmalloc doesn't normally get forgotten. realloc() otoh is a very classic
and far more frequent mistake....

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [PATCH 1/3] slab: introduce krealloc

[Christoph Lameter]

On Wed, 21 Feb 2007, Pekka Enberg wrote:

> On 2/21/07, Arjan van de Ven <arjan@infradead.org> wrote:
> > please mark this one __must_check.. not storing realloc() return values
> > is one of the more nasty types of bugs... but gcc can help us greatly
> > here ;)
> 
> So I guess we want the same thing for the other allocator functions
> (__kmalloc et al) as well?

I think so.

Re: [PATCH 1/3] slab: introduce krealloc

[Christoph Lameter]

On Wed, 21 Feb 2007, Pekka J Enberg wrote:

> +void *krealloc(const void *p, size_t new_size, gfp_t flags)
> +{
> +	void *ret;
> +
> +	if (unlikely(!p))
> +		return kmalloc_track_caller(new_size, flags);
> +
> +	if (unlikely(!new_size)) {
> +		kfree(p);
> +		return NULL;
> +	}
> +
> +	ret = kmalloc_track_caller(new_size, flags);
> +	if (!ret)
> +		return NULL;
> +
> +	memcpy(ret, p, min(new_size, ksize(p)));
> +	kfree(p);
> +	return ret;
> +}
> +EXPORT_SYMBOL(krealloc);

Well could you check ksize for the old object first and if ksize <= new 
size then just skip the copy? I think this may allow you 
to get rid of the ksize callers.

Re: [PATCH 1/3] slab: introduce krealloc

[Pekka Enberg]

Christoph Lameter wrote:
> Well could you check ksize for the old object first and if ksize <= new 
> size then just skip the copy? I think this may allow you 
> to get rid of the ksize callers.

And not reallocate at all, right? I thought about that but then you 
wouldn't be able to use realloc() to make the buffer smaller.

			Pekka

Re: [PATCH 1/3] slab: introduce krealloc

[Christoph Lameter]

On Wed, 21 Feb 2007, Pekka Enberg wrote:

> Christoph Lameter wrote:
> > Well could you check ksize for the old object first and if ksize <= new size
> > then just skip the copy? I think this may allow you to get rid of the ksize
> > callers.
> 
> And not reallocate at all, right? I thought about that but then you wouldn't
> be able to use realloc() to make the buffer smaller.

I think there are two ways to address that:

1. Just do not allow shrinking via realloc. Probably no big loss and best 
performance.

2. Check if the size specified is larger than the next smallest general 
cache and only copy if we would really would allocate from a different 
cache.

Re: [PATCH 1/3] slab: introduce krealloc

[Pekka Enberg]

Hi Christoph,

Christoph Lameter wrote:
> 1. Just do not allow shrinking via realloc. Probably no big loss and best 
> performance.

Not a big loss if you can afford the wasted memory. But, I don't think 
we should do this, there's no way for the caller to know that we will 
hold on to the memory indefinitely.

Christoph Lameter wrote:
> 2. Check if the size specified is larger than the next smallest general 
> cache and only copy if we would really would allocate from a different 
> cache.

Yeah, I was thinking about this too but decided against it (for now) as 
I am mostly concerned with removing the slab abuses from unionfs. Also, 
it is not immediately obvious we want to do this for all cases of 
krealloc so I'd prefer to keep the API for a while and decide to 
optimize or not optimize later. Note that we would only get rid of one 
of the kfree callers, the other one doesn't want to do krealloc(), it 
never reuses the old values.

			Pekka

Re: [PATCH 1/3] slab: introduce krealloc

[Christoph Lameter]

On Wed, 21 Feb 2007, Pekka Enberg wrote:

> Christoph Lameter wrote:
> > 2. Check if the size specified is larger than the next smallest general
> > cache and only copy if we would really would allocate from a different
> > cache.
> 
> Yeah, I was thinking about this too but decided against it (for now) as I am
> mostly concerned with removing the slab abuses from unionfs. Also, it is not
> immediately obvious we want to do this for all cases of krealloc so I'd prefer

Why not? Its a realloc call and these are the classic semantics of 
realloc. Otherwise realloc will always move the memory.

> to keep the API for a while and decide to optimize or not optimize later. Note
> that we would only get rid of one of the kfree callers, the other one doesn't
> want to do krealloc(), it never reuses the old values.

Check that both sizes fall into the same general cache. Do the following 
at the beginning of the function

struct kmem_cache *cachep = page_get_slab(virt_to_page(object));

if (new_size && cachep == kmem_find_general_cachep(new_size, 
cachep->gfpflags))
	/*
	 * Old and new object size us the same general slab so we do not 
	 * have to do anything
	 */
	return object;

Re: [PATCH 1/3] slab: introduce krealloc

[Pekka Enberg]

On 2/21/07, Christoph Lameter <clameter@sgi.com> wrote:
> Why not? Its a realloc call and these are the classic semantics of
> realloc. Otherwise realloc will always move the memory.

Well, as a reference, the user-space equivalent is defined in SUSv3 as:

"The realloc() function shall change the size of the memory object
pointed to by ptr to the size specified by size."

I think it is reasonable to expect krealloc() to not waste too much
space if I, say, reallocate a 128 byte buffer to 32 bytes.

On 2/21/07, Christoph Lameter <clameter@sgi.com> wrote:
> Check that both sizes fall into the same general cache. Do the following
> at the beginning of the function

Not available in the slob allocator AFAIK but yeah, I'll add this
optimization to the slab version. Thanks Christoph.

                             Pekka

Re: [PATCH 1/3] slab: introduce krealloc

[Christoph Lameter]

On Wed, 21 Feb 2007, Pekka Enberg wrote:

> Well, as a reference, the user-space equivalent is defined in SUSv3 as:
> 
> "The realloc() function shall change the size of the memory object
> pointed to by ptr to the size specified by size."

The realloc functions intent is to leave the object in place if possible.
Otherwise one can simply alloc a new object and free the old one.