[PATCH -mm] add a kmem_cache for nsproxy objects

[PATCH -mm] add a kmem_cache for nsproxy objects

[Cedric Le Goater]

It should improve performance in some scenarii where a lot of 
these nsproxy objects are created by unsharing namespaces. This is
a typical use of virtual servers that are being created or entered.

This is also a good tool to find leaks and gather statistics on
namespace usage.

Signed-off-by: Cedric Le Goater <clg@fr.ibm.com>
---
 kernel/nsproxy.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Index: 2.6.22-rc4-mm2/kernel/nsproxy.c
===================================================================
--- 2.6.22-rc4-mm2.orig/kernel/nsproxy.c
+++ 2.6.22-rc4-mm2/kernel/nsproxy.c
@@ -21,6 +21,8 @@
 #include <linux/utsname.h>
 #include <linux/pid_namespace.h>
 
+static struct kmem_cache *nsproxy_cachep;
+
 struct nsproxy init_nsproxy = INIT_NSPROXY(init_nsproxy);
 
 static inline void get_nsproxy(struct nsproxy *ns)
@@ -43,9 +45,11 @@ static inline struct nsproxy *clone_nspr
 {
 	struct nsproxy *ns;
 
-	ns = kmemdup(orig, sizeof(struct nsproxy), GFP_KERNEL);
-	if (ns)
+	ns = kmem_cache_alloc(nsproxy_cachep, GFP_KERNEL);
+	if (ns) {
+		memcpy(ns, orig, sizeof(struct nsproxy));
 		atomic_set(&ns->count, 1);
+	}
 	return ns;
 }
 
@@ -109,7 +113,7 @@ out_uts:
 	if (new_nsp->mnt_ns)
 		put_mnt_ns(new_nsp->mnt_ns);
 out_ns:
-	kfree(new_nsp);
+	kmem_cache_free(nsproxy_cachep, new_nsp);
 	return ERR_PTR(err);
 }
 
@@ -160,7 +164,7 @@ void free_nsproxy(struct nsproxy *ns)
 		put_pid_ns(ns->pid_ns);
 	if (ns->user_ns)
 		put_user_ns(ns->user_ns);
-	kfree(ns);
+	kmem_cache_free(nsproxy_cachep, ns);
 }
 
 /*
@@ -191,3 +195,12 @@ int unshare_nsproxy_namespaces(unsigned 
 	}
 	return err;
 }
+
+static int __init nsproxy_cache_init(void)
+{
+	nsproxy_cachep = kmem_cache_create("nsproxy", sizeof(struct nsproxy),
+					   0, SLAB_PANIC, NULL, NULL);
+	return 0;
+}
+
+module_init(nsproxy_cache_init);

Re: [PATCH -mm] add a kmem_cache for nsproxy objects

[Andrew Morton]

On Mon, 18 Jun 2007 22:53:13 +0200
Cedric Le Goater <clg@fr.ibm.com> wrote:

> +static int __init nsproxy_cache_init(void)
> +{
> +	nsproxy_cachep = kmem_cache_create("nsproxy", sizeof(struct nsproxy),
> +					   0, SLAB_PANIC, NULL, NULL);
> +	return 0;
> +}
> +

Christoph added this cheesy KMEM_CACHE macro.  But I don't immediately recall
the rationale so I'm a bit reluctant to ask people to use-the-cheesy-macro.

Perhaps he can remind us why it is there?

Re: [PATCH -mm] add a kmem_cache for nsproxy objects

[Christoph Hellwig]

On Tue, Jun 19, 2007 at 11:35:01AM -0700, Andrew Morton wrote:
> On Mon, 18 Jun 2007 22:53:13 +0200
> Cedric Le Goater <clg@fr.ibm.com> wrote:
> 
> > +static int __init nsproxy_cache_init(void)
> > +{
> > +	nsproxy_cachep = kmem_cache_create("nsproxy", sizeof(struct nsproxy),
> > +					   0, SLAB_PANIC, NULL, NULL);
> > +	return 0;
> > +}
> > +
> 
> Christoph added this cheesy KMEM_CACHE macro.  But I don't immediately recall
> the rationale so I'm a bit reluctant to ask people to use-the-cheesy-macro.
> 
> Perhaps he can remind us why it is there?

Hmm, I must have missed the macro going in.  Frankly speaking I plain hate
it.  It's a rather useless obsfucation.

Re: [PATCH -mm] add a kmem_cache for nsproxy objects

[Christoph Lameter]

On Tue, 19 Jun 2007, Christoph Hellwig wrote:

> Hmm, I must have missed the macro going in.  Frankly speaking I plain hate
> it.  It's a rather useless obsfucation.

It makes the code easier to review and reduces errors by establishing a 
standard way of defining a slab with minimal effort. You can 
still do the  old style and create the kmem_cache_create parameter 
monsters that span lots of lines.

KMEM_CACHE can do it just by specifying two parameters.

Re: [PATCH -mm] add a kmem_cache for nsproxy objects

[Pekka Enberg]

Hi Christoph x 2,

On Tue, 19 Jun 2007, Christoph Hellwig wrote:
> > Hmm, I must have missed the macro going in.  Frankly speaking I plain hate
> > it.  It's a rather useless obsfucation.

I hate the name, but the macro is far from useless.

On 6/19/07, Christoph Lameter <clameter@sgi.com> wrote:
> It makes the code easier to review and reduces errors by establishing a
> standard way of defining a slab with minimal effort. You can
> still do the  old style and create the kmem_cache_create parameter
> monsters that span lots of lines.
>
> KMEM_CACHE can do it just by specifying two parameters.

Yes and if you look at existing callers of kmem_cache_create(), you'll
notice that most of them work _exactly_ the way the macro does. Most
of the time you want to use default alignment and not define a
constructor.

Re: [PATCH -mm] add a kmem_cache for nsproxy objects

[Christoph Lameter]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 803 bytes --]

On Tue, 19 Jun 2007, Andrew Morton wrote:

> On Mon, 18 Jun 2007 22:53:13 +0200
> Cedric Le Goater <clg@fr.ibm.com> wrote:
> 
> > +static int __init nsproxy_cache_init(void)
> > +{
> > +	nsproxy_cachep = kmem_cache_create("nsproxy", sizeof(struct nsproxy),
> > +					   0, SLAB_PANIC, NULL, NULL);
> > +	return 0;
> > +}
> > +
> 
> Christoph added this cheesy KMEM_CACHE macro.  But I don't immediately recall
> the rationale so I'm a bit reluctant to ask people to use-the-cheesy-macro.
> 
> Perhaps he can remind us why it is there?

Because it simplifies the handling of slabs.

The above will could become:

nsproxy_cachep = KMEM_CACHE(nsproxy, SLAB_PANIC);

meaning create a cache for the nsproxy struct, the nsproxy name and the 
nsproxy size. See include/linux/slab.h.
˜

Re: [PATCH -mm] add a kmem_cache for nsproxy objects

[Cedric Le Goater]

Christoph Lameter wrote:
> On Tue, 19 Jun 2007, Andrew Morton wrote:
> 
>> On Mon, 18 Jun 2007 22:53:13 +0200
>> Cedric Le Goater <clg@fr.ibm.com> wrote:
>>
>>> +static int __init nsproxy_cache_init(void)
>>> +{
>>> +	nsproxy_cachep = kmem_cache_create("nsproxy", sizeof(struct nsproxy),
>>> +					   0, SLAB_PANIC, NULL, NULL);
>>> +	return 0;
>>> +}
>>> +
>> Christoph added this cheesy KMEM_CACHE macro.  But I don't immediately recall
>> the rationale so I'm a bit reluctant to ask people to use-the-cheesy-macro.
>>
>> Perhaps he can remind us why it is there?
> 
> Because it simplifies the handling of slabs.
> 
> The above will could become:
> 
> nsproxy_cachep = KMEM_CACHE(nsproxy, SLAB_PANIC);
> 
> meaning create a cache for the nsproxy struct, the nsproxy name and the 
> nsproxy size. See include/linux/slab.h.

yes, I should probably use that for the nsproxy struct.

my 2cts :
  
the macro sets the align parameter to "__alignof__(struct)" by default. 
is that something we want to do all the time ? if so, why not change 
kmem_cache_create() directly ?

Most of the complexity is in flags. I did a grep and picked what i thought 
was the most aggressive. The macro would probably be more useful if we could 
identify by it's name in which context it can be used. 


C.

Re: [PATCH -mm] add a kmem_cache for nsproxy objects

[Christoph Lameter]

On Fri, 22 Jun 2007, Cedric Le Goater wrote:

> the macro sets the align parameter to "__alignof__(struct)" by default. 
> is that something we want to do all the time ? if so, why not change 
> kmem_cache_create() directly ?

Its a safety net. If there is some reason that the structure needs a 
larger alignment than ARCH_KMALLOC_MINALIGN then that alignment will be 
applied.
 
> Most of the complexity is in flags. I did a grep and picked what i thought 
> was the most aggressive. The macro would probably be more useful if we could 
> identify by it's name in which context it can be used. 

What context are you thinking about and how would it influence 
the macro?