On Mon, 2012-07-30 at 13:18 +0300, Pekka Enberg wrote:
> On Sat, Jul 14, 2012 at 2:12 AM, Shuah Khan <[email protected]> wrote:
> > The label oops is used in CONFIG_DEBUG_VM ifdef block and is defined
> > outside ifdef CONFIG_DEBUG_VM block. This results in the following
> > build warning when built with CONFIG_DEBUG_VM disabled. Fix to move
> > label oops definition to inside a CONFIG_DEBUG_VM block.
> >
> > mm/slab_common.c: In function ‘kmem_cache_create’:
> > mm/slab_common.c:101:1: warning: label ‘oops’ defined but not used
> > [-Wunused-label]
> >
> > Signed-off-by: Shuah Khan <[email protected]>
>
> I merged this as an obvious and safe fix for current merge window. We
> need to clean this up properly for v3.7.
Thanks for merging the obvious fix. I was on vacation for the last two
weeks, and just got back. I sent another patch that restructures the
debug and non-debug code right before I went on vacation. Didn't get a
chance to look at the responses (if any). Will get working on following
up and re-working and re-sending the patch as needed this week.
-- Shuah
kmem_cache_create() does cache integrity checks when CONFIG_DEBUG_VM
is defined. These checks interspersed with the regular code path has
lead to compile time warnings when compiled without CONFIG_DEBUG_VM
defined. Restructuring the code to move the integrity checks in to a new
function would eliminate the current compile warning problem and also
will allow for future changes to the debug only code to evolve without
introducing new warnings in the regular path. This restructuring work
is based on the discussion in the following thread:
https://lkml.org/lkml/2012/7/13/424
Signed-off-by: Shuah Khan <[email protected]>
---
mm/slab_common.c | 74 ++++++++++++++++++++++++++++--------------------------
1 file changed, 38 insertions(+), 36 deletions(-)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 12637ce..08bc2a4 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -23,6 +23,41 @@ enum slab_state slab_state;
LIST_HEAD(slab_caches);
DEFINE_MUTEX(slab_mutex);
+static int kmem_cache_sanity_check(const char *name, size_t size)
+{
+#ifdef CONFIG_DEBUG_VM
+ struct kmem_cache *s = NULL;
+
+ list_for_each_entry(s, &slab_caches, list) {
+ char tmp;
+ int res;
+
+ /*
+ * This happens when the module gets unloaded and doesn't
+ * destroy its slab cache and no-one else reuses the vmalloc
+ * area of the module. Print a warning.
+ */
+ res = probe_kernel_address(s->name, tmp);
+ if (res) {
+ pr_err("Slab cache with size %d has lost its name\n",
+ s->object_size);
+ continue;
+ }
+
+ if (!strcmp(s->name, name)) {
+ pr_err("%s (%s): Cache name already exists.\n",
+ __func__, name);
+ dump_stack();
+ s = NULL;
+ return -EINVAL;
+ }
+ }
+
+ WARN_ON(strchr(name, ' ')); /* It confuses parsers */
+#endif
+ return 0;
+}
+
/*
* kmem_cache_create - Create a cache.
* @name: A string which is used in /proc/slabinfo to identify this cache.
@@ -53,48 +88,17 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
{
struct kmem_cache *s = NULL;
-#ifdef CONFIG_DEBUG_VM
if (!name || in_interrupt() || size < sizeof(void *) ||
size > KMALLOC_MAX_SIZE) {
- printk(KERN_ERR "kmem_cache_create(%s) integrity check"
- " failed\n", name);
+ pr_err("kmem_cache_create(%s) integrity check failed\n", name);
goto out;
}
-#endif
get_online_cpus();
mutex_lock(&slab_mutex);
-#ifdef CONFIG_DEBUG_VM
- list_for_each_entry(s, &slab_caches, list) {
- char tmp;
- int res;
-
- /*
- * This happens when the module gets unloaded and doesn't
- * destroy its slab cache and no-one else reuses the vmalloc
- * area of the module. Print a warning.
- */
- res = probe_kernel_address(s->name, tmp);
- if (res) {
- printk(KERN_ERR
- "Slab cache with size %d has lost its name\n",
- s->object_size);
- continue;
- }
-
- if (!strcmp(s->name, name)) {
- printk(KERN_ERR "kmem_cache_create(%s): Cache name"
- " already exists.\n",
- name);
- dump_stack();
- s = NULL;
- goto oops;
- }
- }
-
- WARN_ON(strchr(name, ' ')); /* It confuses parsers */
-#endif
+ if (kmem_cache_sanity_check(name, size))
+ goto oops;
s = __kmem_cache_create(name, size, align, flags, ctor);
@@ -102,9 +106,7 @@ oops:
mutex_unlock(&slab_mutex);
put_online_cpus();
-#ifdef CONFIG_DEBUG_VM
out:
-#endif
if (!s && (flags & SLAB_PANIC))
panic("kmem_cache_create: Failed to create slab '%s'\n", name);
--
1.7.9.5
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 12637ce..08bc2a4 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -23,6 +23,41 @@ enum slab_state slab_state;
> LIST_HEAD(slab_caches);
> DEFINE_MUTEX(slab_mutex);
>
> +static int kmem_cache_sanity_check(const char *name, size_t size)
> +{
> +#ifdef CONFIG_DEBUG_VM
> + struct kmem_cache *s = NULL;
> +
> + list_for_each_entry(s, &slab_caches, list) {
> + char tmp;
> + int res;
> +
> + /*
> + * This happens when the module gets unloaded and doesn't
> + * destroy its slab cache and no-one else reuses the vmalloc
> + * area of the module. Print a warning.
> + */
> + res = probe_kernel_address(s->name, tmp);
> + if (res) {
> + pr_err("Slab cache with size %d has lost its name\n",
> + s->object_size);
> + continue;
> + }
> +
> + if (!strcmp(s->name, name)) {
> + pr_err("%s (%s): Cache name already exists.\n",
> + __func__, name);
> + dump_stack();
> + s = NULL;
> + return -EINVAL;
> + }
> + }
> +
> + WARN_ON(strchr(name, ' ')); /* It confuses parsers */
> +#endif
> + return 0;
> +}
As I know, following is more preferable than above.
#ifdef CONFIG_DEBUG_VM
static int kmem_cache_sanity_check(const char *name, size_t size);
#else
static inline int kmem_cache_sanity_check(const char *name, size_t size)
{
return 0;
}
#endif
Is there any reason to do like that?
Thanks.
On Tue, 2012-08-07 at 01:49 +0900, JoonSoo Kim wrote:
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index 12637ce..08bc2a4 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -23,6 +23,41 @@ enum slab_state slab_state;
> > LIST_HEAD(slab_caches);
> > DEFINE_MUTEX(slab_mutex);
> >
> > +static int kmem_cache_sanity_check(const char *name, size_t size)
> > +{
> > +#ifdef CONFIG_DEBUG_VM
> > + struct kmem_cache *s = NULL;
> > +
> > + list_for_each_entry(s, &slab_caches, list) {
> > + char tmp;
> > + int res;
> > +
> > + /*
> > + * This happens when the module gets unloaded and doesn't
> > + * destroy its slab cache and no-one else reuses the vmalloc
> > + * area of the module. Print a warning.
> > + */
> > + res = probe_kernel_address(s->name, tmp);
> > + if (res) {
> > + pr_err("Slab cache with size %d has lost its name\n",
> > + s->object_size);
> > + continue;
> > + }
> > +
> > + if (!strcmp(s->name, name)) {
> > + pr_err("%s (%s): Cache name already exists.\n",
> > + __func__, name);
> > + dump_stack();
> > + s = NULL;
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + WARN_ON(strchr(name, ' ')); /* It confuses parsers */
> > +#endif
> > + return 0;
> > +}
>
> As I know, following is more preferable than above.
>
> #ifdef CONFIG_DEBUG_VM
> static int kmem_cache_sanity_check(const char *name, size_t size);
> #else
> static inline int kmem_cache_sanity_check(const char *name, size_t size)
> {
> return 0;
> }
> #endif
>
> Is there any reason to do like that?
> Thanks.
No reason, just something I am used to doing :) inline is a good idea. I
can fix that easily and send v2 patch.
-- Shuah
kmem_cache_create() does cache integrity checks when CONFIG_DEBUG_VM
is defined. These checks interspersed with the regular code path has
lead to compile time warnings when compiled without CONFIG_DEBUG_VM
defined. Restructuring the code to move the integrity checks in to a new
function would eliminate the current compile warning problem and also
will allow for future changes to the debug only code to evolve without
introducing new warnings in the regular path. This restructuring work
is based on the discussion in the following thread:
https://lkml.org/lkml/2012/7/13/424
Signed-off-by: Shuah Khan <[email protected]>
---
mm/slab_common.c | 79 +++++++++++++++++++++++++++++-------------------------
1 file changed, 43 insertions(+), 36 deletions(-)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 12637ce..67409f7 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -23,6 +23,46 @@ enum slab_state slab_state;
LIST_HEAD(slab_caches);
DEFINE_MUTEX(slab_mutex);
+#ifdef CONFIG_DEBUG_VM
+static int kmem_cache_sanity_check(const char *name, size_t size)
+{
+ struct kmem_cache *s = NULL;
+
+ list_for_each_entry(s, &slab_caches, list) {
+ char tmp;
+ int res;
+
+ /*
+ * This happens when the module gets unloaded and doesn't
+ * destroy its slab cache and no-one else reuses the vmalloc
+ * area of the module. Print a warning.
+ */
+ res = probe_kernel_address(s->name, tmp);
+ if (res) {
+ pr_err("Slab cache with size %d has lost its name\n",
+ s->object_size);
+ continue;
+ }
+
+ if (!strcmp(s->name, name)) {
+ pr_err("%s (%s): Cache name already exists.\n",
+ __func__, name);
+ dump_stack();
+ s = NULL;
+ return -EINVAL;
+ }
+ }
+
+ WARN_ON(strchr(name, ' ')); /* It confuses parsers */
+ return 0;
+}
+#else
+static inline int kmem_cache_sanity_check(const char *name, size_t size)
+{
+ return 0;
+}
+#endif
+
/*
* kmem_cache_create - Create a cache.
* @name: A string which is used in /proc/slabinfo to identify this cache.
@@ -53,48 +93,17 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
{
struct kmem_cache *s = NULL;
-#ifdef CONFIG_DEBUG_VM
if (!name || in_interrupt() || size < sizeof(void *) ||
size > KMALLOC_MAX_SIZE) {
- printk(KERN_ERR "kmem_cache_create(%s) integrity check"
- " failed\n", name);
+ pr_err("kmem_cache_create(%s) integrity check failed\n", name);
goto out;
}
-#endif
get_online_cpus();
mutex_lock(&slab_mutex);
-#ifdef CONFIG_DEBUG_VM
- list_for_each_entry(s, &slab_caches, list) {
- char tmp;
- int res;
-
- /*
- * This happens when the module gets unloaded and doesn't
- * destroy its slab cache and no-one else reuses the vmalloc
- * area of the module. Print a warning.
- */
- res = probe_kernel_address(s->name, tmp);
- if (res) {
- printk(KERN_ERR
- "Slab cache with size %d has lost its name\n",
- s->object_size);
- continue;
- }
-
- if (!strcmp(s->name, name)) {
- printk(KERN_ERR "kmem_cache_create(%s): Cache name"
- " already exists.\n",
- name);
- dump_stack();
- s = NULL;
- goto oops;
- }
- }
-
- WARN_ON(strchr(name, ' ')); /* It confuses parsers */
-#endif
+ if (kmem_cache_sanity_check(name, size))
+ goto oops;
s = __kmem_cache_create(name, size, align, flags, ctor);
@@ -102,9 +111,7 @@ oops:
mutex_unlock(&slab_mutex);
put_online_cpus();
-#ifdef CONFIG_DEBUG_VM
out:
-#endif
if (!s && (flags & SLAB_PANIC))
panic("kmem_cache_create: Failed to create slab '%s'\n", name);
--
1.7.9.5
On Mon, 6 Aug 2012, Shuah Khan wrote:
> No reason, just something I am used to doing :) inline is a good idea. I
> can fix that easily and send v2 patch.
Leave that to the compiler. There is no performance reason that would
give a benefit from forcing inline.
On Wed, 2012-08-08 at 09:14 -0500, Christoph Lameter (Open Source)
wrote:
> On Mon, 6 Aug 2012, Shuah Khan wrote:
>
> > No reason, just something I am used to doing :) inline is a good idea. I
> > can fix that easily and send v2 patch.
>
> Leave that to the compiler. There is no performance reason that would
> give a benefit from forcing inline.
>
Already fixed in the v2 patch.
Thanks,
-- Shuah
On Mon, 2012-08-06 at 15:13 -0600, Shuah Khan wrote:
> kmem_cache_create() does cache integrity checks when CONFIG_DEBUG_VM
> is defined. These checks interspersed with the regular code path has
> lead to compile time warnings when compiled without CONFIG_DEBUG_VM
> defined. Restructuring the code to move the integrity checks in to a new
> function would eliminate the current compile warning problem and also
> will allow for future changes to the debug only code to evolve without
> introducing new warnings in the regular path. This restructuring work
> is based on the discussion in the following thread:
>
> https://lkml.org/lkml/2012/7/13/424
Comments, questions. Does this patch look good?
Thanks,
-- Shuah
>
> Signed-off-by: Shuah Khan <[email protected]>
> ---
> mm/slab_common.c | 79 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 43 insertions(+), 36 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 12637ce..67409f7 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -23,6 +23,46 @@ enum slab_state slab_state;
> LIST_HEAD(slab_caches);
> DEFINE_MUTEX(slab_mutex);
>
> +#ifdef CONFIG_DEBUG_VM
> +static int kmem_cache_sanity_check(const char *name, size_t size)
> +{
> + struct kmem_cache *s = NULL;
> +
> + list_for_each_entry(s, &slab_caches, list) {
> + char tmp;
> + int res;
> +
> + /*
> + * This happens when the module gets unloaded and doesn't
> + * destroy its slab cache and no-one else reuses the vmalloc
> + * area of the module. Print a warning.
> + */
> + res = probe_kernel_address(s->name, tmp);
> + if (res) {
> + pr_err("Slab cache with size %d has lost its name\n",
> + s->object_size);
> + continue;
> + }
> +
> + if (!strcmp(s->name, name)) {
> + pr_err("%s (%s): Cache name already exists.\n",
> + __func__, name);
> + dump_stack();
> + s = NULL;
> + return -EINVAL;
> + }
> + }
> +
> + WARN_ON(strchr(name, ' ')); /* It confuses parsers */
> + return 0;
> +}
> +#else
> +static inline int kmem_cache_sanity_check(const char *name, size_t size)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * kmem_cache_create - Create a cache.
> * @name: A string which is used in /proc/slabinfo to identify this cache.
> @@ -53,48 +93,17 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
> {
> struct kmem_cache *s = NULL;
>
> -#ifdef CONFIG_DEBUG_VM
> if (!name || in_interrupt() || size < sizeof(void *) ||
> size > KMALLOC_MAX_SIZE) {
> - printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> - " failed\n", name);
> + pr_err("kmem_cache_create(%s) integrity check failed\n", name);
> goto out;
> }
> -#endif
>
> get_online_cpus();
> mutex_lock(&slab_mutex);
>
> -#ifdef CONFIG_DEBUG_VM
> - list_for_each_entry(s, &slab_caches, list) {
> - char tmp;
> - int res;
> -
> - /*
> - * This happens when the module gets unloaded and doesn't
> - * destroy its slab cache and no-one else reuses the vmalloc
> - * area of the module. Print a warning.
> - */
> - res = probe_kernel_address(s->name, tmp);
> - if (res) {
> - printk(KERN_ERR
> - "Slab cache with size %d has lost its name\n",
> - s->object_size);
> - continue;
> - }
> -
> - if (!strcmp(s->name, name)) {
> - printk(KERN_ERR "kmem_cache_create(%s): Cache name"
> - " already exists.\n",
> - name);
> - dump_stack();
> - s = NULL;
> - goto oops;
> - }
> - }
> -
> - WARN_ON(strchr(name, ' ')); /* It confuses parsers */
> -#endif
> + if (kmem_cache_sanity_check(name, size))
> + goto oops;
>
> s = __kmem_cache_create(name, size, align, flags, ctor);
>
> @@ -102,9 +111,7 @@ oops:
> mutex_unlock(&slab_mutex);
> put_online_cpus();
>
> -#ifdef CONFIG_DEBUG_VM
> out:
> -#endif
> if (!s && (flags & SLAB_PANIC))
> panic("kmem_cache_create: Failed to create slab '%s'\n", name);
>
On Mon, 6 Aug 2012, Shuah Khan wrote:
> +#ifdef CONFIG_DEBUG_VM
> +static int kmem_cache_sanity_check(const char *name, size_t size)
Why do we pass "size" in? AFAICT there is no need to.
> @@ -53,48 +93,17 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
> {
> struct kmem_cache *s = NULL;
>
> -#ifdef CONFIG_DEBUG_VM
> if (!name || in_interrupt() || size < sizeof(void *) ||
> size > KMALLOC_MAX_SIZE) {
> - printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> - " failed\n", name);
> + pr_err("kmem_cache_create(%s) integrity check failed\n", name);
> goto out;
> }
> -#endif
>
If you move the above code into the sanity check function then you will be
using the size as well. These are also sanity checks after all.
On Thu, 2012-08-09 at 09:13 -0500, Christoph Lameter (Open Source)
wrote:
> On Mon, 6 Aug 2012, Shuah Khan wrote:
>
> > +#ifdef CONFIG_DEBUG_VM
> > +static int kmem_cache_sanity_check(const char *name, size_t size)
>
> Why do we pass "size" in? AFAICT there is no need to.
It is an oversight on my part. Will re-work the patch as needed. Please
see more on your second comment below.
>
> > @@ -53,48 +93,17 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
> > {
> > struct kmem_cache *s = NULL;
> >
> > -#ifdef CONFIG_DEBUG_VM
> > if (!name || in_interrupt() || size < sizeof(void *) ||
> > size > KMALLOC_MAX_SIZE) {
> > - printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> > - " failed\n", name);
> > + pr_err("kmem_cache_create(%s) integrity check failed\n", name);
> > goto out;
> > }
> > -#endif
> >
>
> If you move the above code into the sanity check function then you will be
> using the size as well. These are also sanity checks after all.
Yes these are also sanity checks, however these checks are common to
debug and non-debug paths, hence the reasoning to leave them in
kmem_cache_create().
You are right, if these checks get moved into the debug section in
kmem_cache_sanity_check, size will be used.
Moving these checks into kmem_cache_sanity_check() would mean return
path handling will change. The first block of sanity checks for name,
and size etc. are done before holding the slab_mutex and the second
block that checks the slab lists is done after holding the mutex.
Depending on which one fails, return handling is going to be different
in that if second block fails, mutex needs to be unlocked and when the
first block fails, there is no need to do that. Nothing that is too
complex to solve, just something that needs to be handled.
Comments, thoughts on
1. just remove size from kmem_cache_sanity_check() parameters
or
2. move first block sanity checks into kmem_cache_sanity_check()
Personally I prefer the first option to avoid complexity in return path
handling. Would like to hear what others think.
-- Shuah
On Thu, 9 Aug 2012, Shuah Khan wrote:
> Moving these checks into kmem_cache_sanity_check() would mean return
> path handling will change. The first block of sanity checks for name,
> and size etc. are done before holding the slab_mutex and the second
> block that checks the slab lists is done after holding the mutex.
> Depending on which one fails, return handling is going to be different
> in that if second block fails, mutex needs to be unlocked and when the
> first block fails, there is no need to do that. Nothing that is too
> complex to solve, just something that needs to be handled.
Right. The taking of the mutex etc is not depending on the parameters at
all. So its possible. Its rather simple.
> Comments, thoughts on
>
> 1. just remove size from kmem_cache_sanity_check() parameters
> or
> 2. move first block sanity checks into kmem_cache_sanity_check()
>
> Personally I prefer the first option to avoid complexity in return path
> handling. Would like to hear what others think.
We already have to deal with the return path handling for other failure
cases.
On Thu, 2012-08-09 at 14:08 -0500, Christoph Lameter (Open Source)
wrote:
> On Thu, 9 Aug 2012, Shuah Khan wrote:
>
> > Moving these checks into kmem_cache_sanity_check() would mean return
> > path handling will change. The first block of sanity checks for name,
> > and size etc. are done before holding the slab_mutex and the second
> > block that checks the slab lists is done after holding the mutex.
> > Depending on which one fails, return handling is going to be different
> > in that if second block fails, mutex needs to be unlocked and when the
> > first block fails, there is no need to do that. Nothing that is too
> > complex to solve, just something that needs to be handled.
>
> Right. The taking of the mutex etc is not depending on the parameters at
> all. So its possible. Its rather simple.
>
> > Comments, thoughts on
> >
> > 1. just remove size from kmem_cache_sanity_check() parameters
> > or
> > 2. move first block sanity checks into kmem_cache_sanity_check()
> >
> > Personally I prefer the first option to avoid complexity in return path
> > handling. Would like to hear what others think.
>
> We already have to deal with the return path handling for other failure
> cases.
Thanks for the feedback. I will send v3 patch with the changes we
discussed.
-- Shuah
kmem_cache_create() does cache integrity checks when CONFIG_DEBUG_VM
is defined. These checks interspersed with the regular code path has
lead to compile time warnings when compiled without CONFIG_DEBUG_VM
defined. Restructuring the code to move the integrity checks in to a new
function would eliminate the current compile warning problem and also
will allow for future changes to the debug only code to evolve without
introducing new warnings in the regular path. This restructuring work
is based on the discussion in the following thread:
https://lkml.org/lkml/2012/7/13/424
Signed-off-by: Shuah Khan <[email protected]>
---
mm/slab_common.c | 90 +++++++++++++++++++++++++++++-------------------------
1 file changed, 48 insertions(+), 42 deletions(-)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 12637ce..44facdf 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -23,6 +23,52 @@ enum slab_state slab_state;
LIST_HEAD(slab_caches);
DEFINE_MUTEX(slab_mutex);
+#ifdef CONFIG_DEBUG_VM
+static int kmem_cache_sanity_check(const char *name, size_t size)
+{
+ struct kmem_cache *s = NULL;
+
+ if (!name || in_interrupt() || size < sizeof(void *) ||
+ size > KMALLOC_MAX_SIZE) {
+ pr_err("kmem_cache_create(%s) integrity check failed\n", name);
+ return -EINVAL;
+ }
+
+ list_for_each_entry(s, &slab_caches, list) {
+ char tmp;
+ int res;
+
+ /*
+ * This happens when the module gets unloaded and doesn't
+ * destroy its slab cache and no-one else reuses the vmalloc
+ * area of the module. Print a warning.
+ */
+ res = probe_kernel_address(s->name, tmp);
+ if (res) {
+ pr_err("Slab cache with size %d has lost its name\n",
+ s->object_size);
+ continue;
+ }
+
+ if (!strcmp(s->name, name)) {
+ pr_err("%s (%s): Cache name already exists.\n",
+ __func__, name);
+ dump_stack();
+ s = NULL;
+ return -EINVAL;
+ }
+ }
+
+ WARN_ON(strchr(name, ' ')); /* It confuses parsers */
+ return 0;
+}
+#else
+static inline int kmem_cache_sanity_check(const char *name, size_t size)
+{
+ return 0;
+}
+#endif
+
/*
* kmem_cache_create - Create a cache.
* @name: A string which is used in /proc/slabinfo to identify this cache.
@@ -53,48 +99,11 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
{
struct kmem_cache *s = NULL;
-#ifdef CONFIG_DEBUG_VM
- if (!name || in_interrupt() || size < sizeof(void *) ||
- size > KMALLOC_MAX_SIZE) {
- printk(KERN_ERR "kmem_cache_create(%s) integrity check"
- " failed\n", name);
- goto out;
- }
-#endif
-
get_online_cpus();
mutex_lock(&slab_mutex);
-#ifdef CONFIG_DEBUG_VM
- list_for_each_entry(s, &slab_caches, list) {
- char tmp;
- int res;
-
- /*
- * This happens when the module gets unloaded and doesn't
- * destroy its slab cache and no-one else reuses the vmalloc
- * area of the module. Print a warning.
- */
- res = probe_kernel_address(s->name, tmp);
- if (res) {
- printk(KERN_ERR
- "Slab cache with size %d has lost its name\n",
- s->object_size);
- continue;
- }
-
- if (!strcmp(s->name, name)) {
- printk(KERN_ERR "kmem_cache_create(%s): Cache name"
- " already exists.\n",
- name);
- dump_stack();
- s = NULL;
- goto oops;
- }
- }
-
- WARN_ON(strchr(name, ' ')); /* It confuses parsers */
-#endif
+ if (kmem_cache_sanity_check(name, size))
+ goto oops;
s = __kmem_cache_create(name, size, align, flags, ctor);
@@ -102,9 +111,6 @@ oops:
mutex_unlock(&slab_mutex);
put_online_cpus();
-#ifdef CONFIG_DEBUG_VM
-out:
-#endif
if (!s && (flags & SLAB_PANIC))
panic("kmem_cache_create: Failed to create slab '%s'\n", name);
--
1.7.9.5
Acked-by: Christoph Lameter <[email protected]>
On Aug 12, 2012, at 11:40, Shuah Khan <[email protected]> wrote:
> kmem_cache_create() does cache integrity checks when CONFIG_DEBUG_VM
> is defined. These checks interspersed with the regular code path has
> lead to compile time warnings when compiled without CONFIG_DEBUG_VM
> defined. Restructuring the code to move the integrity checks in to a new
> function would eliminate the current compile warning problem and also
> will allow for future changes to the debug only code to evolve without
> introducing new warnings in the regular path. This restructuring work
> is based on the discussion in the following thread:
>
> https://lkml.org/lkml/2012/7/13/424
>
> Signed-off-by: Shuah Khan <[email protected]>
> ---
> mm/slab_common.c | 90 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 12637ce..44facdf 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -23,6 +23,52 @@ enum slab_state slab_state;
> LIST_HEAD(slab_caches);
> DEFINE_MUTEX(slab_mutex);
>
> +#ifdef CONFIG_DEBUG_VM
> +static int kmem_cache_sanity_check(const char *name, size_t size)
> +{
> + struct kmem_cache *s = NULL;
> +
> + if (!name || in_interrupt() || size < sizeof(void *) ||
> + size > KMALLOC_MAX_SIZE) {
> + pr_err("kmem_cache_create(%s) integrity check failed\n", name);
> + return -EINVAL;
> + }
> +
> + list_for_each_entry(s, &slab_caches, list) {
> + char tmp;
> + int res;
> +
> + /*
> + * This happens when the module gets unloaded and doesn't
> + * destroy its slab cache and no-one else reuses the vmalloc
> + * area of the module. Print a warning.
> + */
> + res = probe_kernel_address(s->name, tmp);
> + if (res) {
> + pr_err("Slab cache with size %d has lost its name\n",
> + s->object_size);
> + continue;
> + }
> +
> + if (!strcmp(s->name, name)) {
> + pr_err("%s (%s): Cache name already exists.\n",
> + __func__, name);
> + dump_stack();
> + s = NULL;
> + return -EINVAL;
> + }
> + }
> +
> + WARN_ON(strchr(name, ' ')); /* It confuses parsers */
> + return 0;
> +}
> +#else
> +static inline int kmem_cache_sanity_check(const char *name, size_t size)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * kmem_cache_create - Create a cache.
> * @name: A string which is used in /proc/slabinfo to identify this cache.
> @@ -53,48 +99,11 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
> {
> struct kmem_cache *s = NULL;
>
> -#ifdef CONFIG_DEBUG_VM
> - if (!name || in_interrupt() || size < sizeof(void *) ||
> - size > KMALLOC_MAX_SIZE) {
> - printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> - " failed\n", name);
> - goto out;
> - }
> -#endif
> -
> get_online_cpus();
> mutex_lock(&slab_mutex);
>
> -#ifdef CONFIG_DEBUG_VM
> - list_for_each_entry(s, &slab_caches, list) {
> - char tmp;
> - int res;
> -
> - /*
> - * This happens when the module gets unloaded and doesn't
> - * destroy its slab cache and no-one else reuses the vmalloc
> - * area of the module. Print a warning.
> - */
> - res = probe_kernel_address(s->name, tmp);
> - if (res) {
> - printk(KERN_ERR
> - "Slab cache with size %d has lost its name\n",
> - s->object_size);
> - continue;
> - }
> -
> - if (!strcmp(s->name, name)) {
> - printk(KERN_ERR "kmem_cache_create(%s): Cache name"
> - " already exists.\n",
> - name);
> - dump_stack();
> - s = NULL;
> - goto oops;
> - }
> - }
> -
> - WARN_ON(strchr(name, ' ')); /* It confuses parsers */
> -#endif
> + if (kmem_cache_sanity_check(name, size))
> + goto oops;
>
> s = __kmem_cache_create(name, size, align, flags, ctor);
>
> @@ -102,9 +111,6 @@ oops:
> mutex_unlock(&slab_mutex);
> put_online_cpus();
>
> -#ifdef CONFIG_DEBUG_VM
> -out:
> -#endif
> if (!s && (flags & SLAB_PANIC))
> panic("kmem_cache_create: Failed to create slab '%s'\n", name);
>
> --
> 1.7.9.5
>
>
On Sun, 12 Aug 2012 10:40:18 -0600
Shuah Khan <[email protected]> wrote:
> kmem_cache_create() does cache integrity checks when CONFIG_DEBUG_VM
> is defined. These checks interspersed with the regular code path has
> lead to compile time warnings when compiled without CONFIG_DEBUG_VM
> defined. Restructuring the code to move the integrity checks in to a new
> function would eliminate the current compile warning problem and also
> will allow for future changes to the debug only code to evolve without
> introducing new warnings in the regular path. This restructuring work
> is based on the discussion in the following thread:
Your patch appears to be against some ancient old kernel, such as 3.5.
I did this:
--- a/mm/slab_common.c~mm-slab_commonc-restructure-kmem_cache_create-to-move-debug-cache-integrity-checks-into-a-new-function-fix
+++ a/mm/slab_common.c
@@ -101,15 +101,8 @@ struct kmem_cache *kmem_cache_create(con
get_online_cpus();
mutex_lock(&slab_mutex);
-
- if (kmem_cache_sanity_check(name, size))
- goto oops;
-
- s = __kmem_cache_create(name, size, align, flags, ctor);
-
-#ifdef CONFIG_DEBUG_VM
-oops:
-#endif
+ if (kmem_cache_sanity_check(name, size) == 0)
+ s = __kmem_cache_create(name, size, align, flags, ctor);
mutex_unlock(&slab_mutex);
put_online_cpus();
_
On Thu, Aug 16, 2012 at 2:53 AM, Andrew Morton
<[email protected]> wrote:
> On Sun, 12 Aug 2012 10:40:18 -0600
> Shuah Khan <[email protected]> wrote:
>
>> kmem_cache_create() does cache integrity checks when CONFIG_DEBUG_VM
>> is defined. These checks interspersed with the regular code path has
>> lead to compile time warnings when compiled without CONFIG_DEBUG_VM
>> defined. Restructuring the code to move the integrity checks in to a new
>> function would eliminate the current compile warning problem and also
>> will allow for future changes to the debug only code to evolve without
>> introducing new warnings in the regular path. This restructuring work
>> is based on the discussion in the following thread:
>
> Your patch appears to be against some ancient old kernel, such as 3.5.
> I did this:
>
> --- a/mm/slab_common.c~mm-slab_commonc-restructure-kmem_cache_create-to-move-debug-cache-integrity-checks-into-a-new-function-fix
> +++ a/mm/slab_common.c
> @@ -101,15 +101,8 @@ struct kmem_cache *kmem_cache_create(con
>
> get_online_cpus();
> mutex_lock(&slab_mutex);
> -
> - if (kmem_cache_sanity_check(name, size))
> - goto oops;
> -
> - s = __kmem_cache_create(name, size, align, flags, ctor);
> -
> -#ifdef CONFIG_DEBUG_VM
> -oops:
> -#endif
> + if (kmem_cache_sanity_check(name, size) == 0)
> + s = __kmem_cache_create(name, size, align, flags, ctor);
> mutex_unlock(&slab_mutex);
> put_online_cpus();
Yup. Shuah, care to spin another version against slab/next?