2012-10-02 10:02:55

by Glauber Costa

[permalink] [raw]
Subject: [PATCH v3] slab: Ignore internal flags in cache creation

Some flags are used internally by the allocators for management
purposes. One example of that is the CFLGS_OFF_SLAB flag that slab uses
to mark that the metadata for that cache is stored outside of the slab.

No cache should ever pass those as a creation flags. We can just ignore
this bit if it happens to be passed (such as when duplicating a cache in
the kmem memcg patches).

Because such flags can vary from allocator to allocator, we allow them
to make their own decisions on that, defining SLAB_AVAILABLE_FLAGS with
all flags that are valid at creation time. Allocators that doesn't have
any specific flag requirement should define that to mean all flags.

Common code will mask out all flags not belonging to that set.

[ v2: leave the mask out decision up to the allocators ]
[ v3: define flags for all allocators ]

Signed-off-by: Glauber Costa <[email protected]>
Acked-by: Christoph Lameter <[email protected]>
Acked-by: David Rientjes <[email protected]>
CC: Pekka Enberg <[email protected]>
---
include/linux/slab_def.h | 19 +++++++++++++++++++
include/linux/slob_def.h | 2 ++
include/linux/slub_def.h | 2 ++
mm/slab.c | 22 ----------------------
mm/slab_common.c | 7 +++++++
5 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
index 36d7031..cec4f5d 100644
--- a/include/linux/slab_def.h
+++ b/include/linux/slab_def.h
@@ -15,6 +15,25 @@
#include <asm/cache.h> /* kmalloc_sizes.h needs L1_CACHE_BYTES */
#include <linux/compiler.h>

+/* Legal flag mask for kmem_cache_create(). */
+#ifdef CONFIG_DEBUG_SLAB
+#define SLAB_AVAILABLE_FLAGS (SLAB_RED_ZONE | \
+ SLAB_POISON | SLAB_HWCACHE_ALIGN | \
+ SLAB_CACHE_DMA | \
+ SLAB_STORE_USER | \
+ SLAB_RECLAIM_ACCOUNT | SLAB_PANIC | \
+ SLAB_DESTROY_BY_RCU | SLAB_MEM_SPREAD | \
+ SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE | \
+ SLAB_NOTRACK)
+#else
+#define SLAB_AVAILABLE_FLAGS (SLAB_HWCACHE_ALIGN | \
+ SLAB_CACHE_DMA | \
+ SLAB_RECLAIM_ACCOUNT | SLAB_PANIC | \
+ SLAB_DESTROY_BY_RCU | SLAB_MEM_SPREAD | \
+ SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE | \
+ SLAB_NOTRACK)
+#endif
+
/*
* struct kmem_cache
*
diff --git a/include/linux/slob_def.h b/include/linux/slob_def.h
index 0ec00b3..606b97b 100644
--- a/include/linux/slob_def.h
+++ b/include/linux/slob_def.h
@@ -1,6 +1,8 @@
#ifndef __LINUX_SLOB_DEF_H
#define __LINUX_SLOB_DEF_H

+#define SLAB_AVAILABLE_FLAGS 0xFFFFFFFFUL /* No flag restriction */
+
void *kmem_cache_alloc_node(struct kmem_cache *, gfp_t flags, int node);

static __always_inline void *kmem_cache_alloc(struct kmem_cache *cachep,
diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index df448ad..9e9b8c5 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -14,6 +14,8 @@

#include <linux/kmemleak.h>

+#define SLAB_AVAILABLE_FLAGS 0xFFFFFFFFUL /* No flag restriction */
+
enum stat_item {
ALLOC_FASTPATH, /* Allocation from cpu slab */
ALLOC_SLOWPATH, /* Allocation by getting a new cpu slab */
diff --git a/mm/slab.c b/mm/slab.c
index 8524923..8c1d447 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -162,23 +162,6 @@
*/
static bool pfmemalloc_active __read_mostly;

-/* Legal flag mask for kmem_cache_create(). */
-#if DEBUG
-# define CREATE_MASK (SLAB_RED_ZONE | \
- SLAB_POISON | SLAB_HWCACHE_ALIGN | \
- SLAB_CACHE_DMA | \
- SLAB_STORE_USER | \
- SLAB_RECLAIM_ACCOUNT | SLAB_PANIC | \
- SLAB_DESTROY_BY_RCU | SLAB_MEM_SPREAD | \
- SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE | SLAB_NOTRACK)
-#else
-# define CREATE_MASK (SLAB_HWCACHE_ALIGN | \
- SLAB_CACHE_DMA | \
- SLAB_RECLAIM_ACCOUNT | SLAB_PANIC | \
- SLAB_DESTROY_BY_RCU | SLAB_MEM_SPREAD | \
- SLAB_DEBUG_OBJECTS | SLAB_NOLEAKTRACE | SLAB_NOTRACK)
-#endif
-
/*
* kmem_bufctl_t:
*
@@ -2385,11 +2368,6 @@ __kmem_cache_create (struct kmem_cache *cachep, unsigned long flags)
if (flags & SLAB_DESTROY_BY_RCU)
BUG_ON(flags & SLAB_POISON);
#endif
- /*
- * Always checks flags, a caller might be expecting debug support which
- * isn't available.
- */
- BUG_ON(flags & ~CREATE_MASK);

/*
* Check that size is in terms of words. This is needed to avoid
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 9c21725..79be32e 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -107,6 +107,13 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
if (!kmem_cache_sanity_check(name, size) == 0)
goto out_locked;

+ /*
+ * Some allocators will constraint the set of valid flags to a subset
+ * of all flags. We expect them to define CACHE_CREATE_MASK in this
+ * case, and we'll just provide them with a sanitized version of the
+ * passed flags.
+ */
+ flags &= SLAB_AVAILABLE_FLAGS;

s = __kmem_cache_alias(name, size, align, flags, ctor);
if (s)
--
1.7.11.4


Subject: Re: [PATCH v3] slab: Ignore internal flags in cache creation

On Tue, 2 Oct 2012, Glauber Costa wrote:

> #include <linux/kmemleak.h>
>
> +#define SLAB_AVAILABLE_FLAGS 0xFFFFFFFFUL /* No flag restriction */
> +
> enum stat_item {

I thought the SLAB_AVAILABLE_FLAGS would stand for something meaningful
like the flags supported by an allocator given a kernel config. F.e. SLUB
does not support the debug flags if not compiled with debug.

This looks like it could become material that would fit in mm/slab.h.
There are sets of flags that all allocators have to support (RCU, DMA etc)
and others (like the debug flags) that are optional.

Slob also supports some flags but never any of the debug flags.

2012-10-02 19:54:22

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH v3] slab: Ignore internal flags in cache creation

On Tue, 2 Oct 2012, Glauber Costa wrote:

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 9c21725..79be32e 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -107,6 +107,13 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
> if (!kmem_cache_sanity_check(name, size) == 0)
> goto out_locked;
>
> + /*
> + * Some allocators will constraint the set of valid flags to a subset
> + * of all flags. We expect them to define CACHE_CREATE_MASK in this

s/CACHE_CREATE_MASK/SLAB_AVAILABLE_FLAGS/

I don't think SLAB_AVAILABLE_FLAGS is the best name we can come up with, I
think it should be at least something like CACHE_ALLOWED_FLAGS, but I'm
fine with whatever it turns out to be.

> + * case, and we'll just provide them with a sanitized version of the
> + * passed flags.
> + */
> + flags &= SLAB_AVAILABLE_FLAGS;
>
> s = __kmem_cache_alias(name, size, align, flags, ctor);
> if (s)