Skip to contentSkip to conversation

add a kmem_cache for nsproxy objects

PATCH In review Cedric Le Goater 5 participants 8 messages Jun 18, 2007 – Jun 22, 2007

lore
Share & tools
lore

Cedric Le Goater

author
#0 lore raw

PATCH 1/1 add a kmem_cache for nsproxy objects

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 <[email protected]>
---
 kernel/nsproxy.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
kernel/nsproxy.c +17 -4
Index: 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);
#1 lore raw

kernel/nsproxy.c int unshare_nsproxy_namespaces(unsigned · line 205

Quoted 7 lines · On Mon, 18 Jun 2007 22:53:13 +0200 Cedric Le Goater <[email protected]> wrote:
kernel/nsproxy.c +7 -0
+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?

#2 lore raw
Quoted 14 lines · Andrew Morton

kernel/nsproxy.c int unshare_nsproxy_namespaces(unsigned · line 205

Quoted 7 lines · On Mon, 18 Jun 2007 22:53:13 +0200 Cedric Le Goater <[email protected]> wrote:
kernel/nsproxy.c +7 -0
+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.

Quoted 1 line

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.

#3 lore raw
Quoted 2 lines · Christoph Hellwig

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.

#4 lore raw

Hi Christoph x 2,

Quoted 2 lines · Christoph Hellwig
Quoted 2 lines

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.

Quoted 6 lines · Christoph Lameter

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.

#5 lore raw
In reply to Andrew Morton
Quoted 14 lines · Andrew Morton

kernel/nsproxy.c int unshare_nsproxy_namespaces(unsigned · line 205

Quoted 7 lines · On Mon, 18 Jun 2007 22:53:13 +0200 Cedric Le Goater <[email protected]> wrote:
kernel/nsproxy.c +7 -0
+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.

Quoted 1 line

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

author
#6 lore raw
Quoted 17 lines · Christoph Lameter
Quoted 13 lines · Andrew Morton

kernel/nsproxy.c int unshare_nsproxy_namespaces(unsigned · line 205

Quoted 7 lines · On Mon, 18 Jun 2007 22:53:13 +0200 Cedric Le Goater <[email protected]> wrote:
kernel/nsproxy.c +7 -0
+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.

Quoted 1 line

Perhaps he can remind us why it is there?

Quoted 8 lines

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.

#7 lore raw
Quoted 3 lines · Cedric Le Goater

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.

Quoted 3 lines

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?