[PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Shuah Khan]

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 <shuah.khan@hp.com>
---
 mm/slab_common.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 12637ce..aa3ca5b 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
 
 	s = __kmem_cache_create(name, size, align, flags, ctor);
 
+#ifdef CONFIG_DEBUG_VM
 oops:
+#endif
 	mutex_unlock(&slab_mutex);
 	put_online_cpus();
 
-- 
1.7.9.5

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[David Rientjes]

On Fri, 13 Jul 2012, Shuah Khan wrote:

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 12637ce..aa3ca5b 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
>  
>  	s = __kmem_cache_create(name, size, align, flags, ctor);
>  
> +#ifdef CONFIG_DEBUG_VM
>  oops:
> +#endif
>  	mutex_unlock(&slab_mutex);
>  	put_online_cpus();
>  

Tip: gcc allows label attributes so you could actually do

oops: __maybe_unused

to silence the warning and do the same for the "out" label later in the 
function.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Pekka Enberg]

On Sat, Jul 14, 2012 at 12:18 PM, David Rientjes <rientjes@google.com> wrote:
> On Fri, 13 Jul 2012, Shuah Khan wrote:
>
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index 12637ce..aa3ca5b 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
>>
>>       s = __kmem_cache_create(name, size, align, flags, ctor);
>>
>> +#ifdef CONFIG_DEBUG_VM
>>  oops:
>> +#endif
>>       mutex_unlock(&slab_mutex);
>>       put_online_cpus();
>>
>
> Tip: gcc allows label attributes so you could actually do
>
> oops: __maybe_unused
>
> to silence the warning and do the same for the "out" label later in the
> function.

I'm not exactly loving that either.

It'd probably be better to reshuffle the code so that the debug checks
end up in separate functions that are no-op for !CONFIG_DEBUG_VM. That
way the _labels_ are used unconditionally although there's no actual
code generated.

                        Pekka

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Shuah Khan]

On Sat, 2012-07-14 at 15:01 +0300, Pekka Enberg wrote:

> I'm not exactly loving that either.
> 
> It'd probably be better to reshuffle the code so that the debug checks
> end up in separate functions that are no-op for !CONFIG_DEBUG_VM. That
> way the _labels_ are used unconditionally although there's no actual
> code generated.

I can work on reshuffling the code. Do have a question though. This
following sanity check is currently done only when CONFIG_DEBUG_VM is
defined. However, it does appear to be something that is that should be
checked even in regular path.

struct kmem_cache *kmem_cache_create(const char *name, size_t size,
size_t align,
                unsigned long flags, void (*ctor)(void *))
{
        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



---

}

Am I reading this right?

Thanks,
-- Shuah

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[David Rientjes]

On Sun, 15 Jul 2012, Shuah Khan wrote:

> I can work on reshuffling the code. Do have a question though. This
> following sanity check is currently done only when CONFIG_DEBUG_VM is
> defined. However, it does appear to be something that is that should be
> checked even in regular path.
> 
> struct kmem_cache *kmem_cache_create(const char *name, size_t size,
> size_t align,
>                 unsigned long flags, void (*ctor)(void *))
> {
>         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
> 

Agreed, this shouldn't depend on CONFIG_DEBUG_VM.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Christoph Lameter]

On Mon, 16 Jul 2012, David Rientjes wrote:

> On Sun, 15 Jul 2012, Shuah Khan wrote:
>
> > I can work on reshuffling the code. Do have a question though. This
> > following sanity check is currently done only when CONFIG_DEBUG_VM is
> > defined. However, it does appear to be something that is that should be
> > checked even in regular path.
> >
> > struct kmem_cache *kmem_cache_create(const char *name, size_t size,
> > size_t align,
> >                 unsigned long flags, void (*ctor)(void *))
> > {
> >         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
> >
>
> Agreed, this shouldn't depend on CONFIG_DEBUG_VM.

These checks are useless for regular kernel operations. They are
only useful when developing code and should only be enabled during
development. There is no point in testing the size and the name which are
typically constant when a slab is created with a stable kernel.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Shuah Khan]

On Mon, 2012-07-16 at 09:17 -0500, Christoph Lameter wrote:
> On Mon, 16 Jul 2012, David Rientjes wrote:
> 
> > On Sun, 15 Jul 2012, Shuah Khan wrote:
> >
> > > I can work on reshuffling the code. Do have a question though. This
> > > following sanity check is currently done only when CONFIG_DEBUG_VM is
> > > defined. However, it does appear to be something that is that should be
> > > checked even in regular path.
> > >
> > > struct kmem_cache *kmem_cache_create(const char *name, size_t size,
> > > size_t align,
> > >                 unsigned long flags, void (*ctor)(void *))
> > > {
> > >         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
> > >
> >
> > Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
> 
> These checks are useless for regular kernel operations. They are
> only useful when developing code and should only be enabled during
> development. There is no point in testing the size and the name which are
> typically constant when a slab is created with a stable kernel.
> 

ok. The first debug section is done prior to holding the slab mutex and
the second debug section is after holding mutex. I will have to think
about the best way to restructure the code. I will send the re-worked
patch soon, so we start refining it if need be.

-- Shuah

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[David Rientjes]

On Mon, 16 Jul 2012, Christoph Lameter wrote:

> > > struct kmem_cache *kmem_cache_create(const char *name, size_t size,
> > > size_t align,
> > >                 unsigned long flags, void (*ctor)(void *))
> > > {
> > >         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
> > >
> >
> > Agreed, this shouldn't depend on CONFIG_DEBUG_VM.
> 
> These checks are useless for regular kernel operations. They are
> only useful when developing code and should only be enabled during
> development. There is no point in testing the size and the name which are
> typically constant when a slab is created with a stable kernel.
> 

Sounds like a response from someone who is very familiar with slab 
allocators.  The reality, though, is that very few people are going to be 
doing development with CONFIG_DEBUG_VM enabled unless they notice problems 
beforehand.

Are you seriously trying to optimize kmem_cache_create()?  These checks 
certainly aren't going to hurt your perfromance and it seems appropriate 
to do some sanity checking before blowing up in unexpected ways.  It's 
also the way it's been done for years before extracting common allocator 
functions to their own file.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Christoph Lameter]

On Mon, 16 Jul 2012, David Rientjes wrote:

> > These checks are useless for regular kernel operations. They are
> > only useful when developing code and should only be enabled during
> > development. There is no point in testing the size and the name which are
> > typically constant when a slab is created with a stable kernel.
> >
>
> Sounds like a response from someone who is very familiar with slab
> allocators.  The reality, though, is that very few people are going to be
> doing development with CONFIG_DEBUG_VM enabled unless they notice problems
> beforehand.

Kernels are certainly run with CONFIG_DEBUG_VM before merges to mainstream
occur. If the developer does not do it then someone else will.

> Are you seriously trying to optimize kmem_cache_create()?  These checks
> certainly aren't going to hurt your perfromance and it seems appropriate
> to do some sanity checking before blowing up in unexpected ways.  It's
> also the way it's been done for years before extracting common allocator
> functions to their own file.

The kernel cannot check everything and will blow up in unexpected ways if
someone codes something stupid. There are numerous debugging options that
need to be switched on to get better debugging information to investigate
deper. Adding special code to replicate these checks is bad.

Frankly, these checks are there only for legacy reasons in the common code
due to SLAB having them. Checking for NULL pointers is pretty useless
since any dereference will cause a oops that will show where this
occurred.

The other checks are of the same order of uselessness. The interrupt check
f.e. is nonsense since the first attempt to acquire the slab
futex will trigger another exception.

I would suggest to rather drop these checks entirely. SLUB never had these
braindead things in them and has been in use for quite a long time.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[David Rientjes]

On Mon, 16 Jul 2012, Christoph Lameter wrote:

> > Sounds like a response from someone who is very familiar with slab
> > allocators.  The reality, though, is that very few people are going to be
> > doing development with CONFIG_DEBUG_VM enabled unless they notice problems
> > beforehand.
> 
> Kernels are certainly run with CONFIG_DEBUG_VM before merges to mainstream
> occur. If the developer does not do it then someone else will.
> 

So let's say a developer wants to pass a dynamically allocated string to 
kmem_cache_create() for the cache name and it happens to be NULL because 
of a failed allocation but this never happened in testing (or it does 
happen but CONFIG_DEBUG_VM=n) and they are using CONFIG_SLAB.

What would the failure be in linux-next?  It looks like it would just 
result in a corrupted slabinfo.  Bad result, we used to catch this problem 
before the extraction of common functionality and now we've allowed a 
corrupted slabinfo for nothing: optimizing kmem_cache_create() is 
pointless.

> The kernel cannot check everything and will blow up in unexpected ways if
> someone codes something stupid. There are numerous debugging options that
> need to be switched on to get better debugging information to investigate
> deper. Adding special code to replicate these checks is bad.
> 

Disagree, CONFIG_SLAB does not blow up for a NULL name string and just 
corrupts userspace.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Christoph Lameter]

On Mon, 16 Jul 2012, David Rientjes wrote:

> > The kernel cannot check everything and will blow up in unexpected ways if
> > someone codes something stupid. There are numerous debugging options that
> > need to be switched on to get better debugging information to investigate
> > deper. Adding special code to replicate these checks is bad.
> >
>
> Disagree, CONFIG_SLAB does not blow up for a NULL name string and just
> corrupts userspace.

Ohh.. So far we only had science fiction. Now kernel fiction.... If you
could corrupt userspace using sysfs with a NULL string then you'd first
need to fix sysfs support.

And if you really want to be totally safe then I guess you need to audit
the kernel and make sure that every core kernel function that takes a
string argument does check for it to be NULL just in case.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Pekka Enberg]

On Mon, 16 Jul 2012, David Rientjes wrote:
>> > The kernel cannot check everything and will blow up in unexpected ways if
>> > someone codes something stupid. There are numerous debugging options that
>> > need to be switched on to get better debugging information to investigate
>> > deper. Adding special code to replicate these checks is bad.
>>
>> Disagree, CONFIG_SLAB does not blow up for a NULL name string and just
>> corrupts userspace.

On Tue, Jul 17, 2012 at 5:36 PM, Christoph Lameter <cl@linux.com> wrote:
> Ohh.. So far we only had science fiction. Now kernel fiction.... If you
> could corrupt userspace using sysfs with a NULL string then you'd first
> need to fix sysfs support.
>
> And if you really want to be totally safe then I guess you need to audit
> the kernel and make sure that every core kernel function that takes a
> string argument does check for it to be NULL just in case.

Well, even SLUB checks for !name in mainline so that's definitely
worth including unconditionally. Furthermore, the size related checks
certainly make sense and I don't see any harm in having them as well.

As for "in_interrupt()", I really don't see the point in keeping that
around. We could push it down to mm/slab.c in "__kmem_cache_create()"
if we wanted to.

                                Pekka

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Christoph Lameter]

On Tue, 17 Jul 2012, Pekka Enberg wrote:

> Well, even SLUB checks for !name in mainline so that's definitely
> worth including unconditionally. Furthermore, the size related checks
> certainly make sense and I don't see any harm in having them as well.

There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a
NULL pointer dereference later in the caller?

> As for "in_interrupt()", I really don't see the point in keeping that
> around. We could push it down to mm/slab.c in "__kmem_cache_create()"
> if we wanted to.

Ok we could do that but I guess we are in the discussion of how much
checking should be done for a production kernel.

I think these checks are way out of hand. We cannot afford to
consistently check parameters to all kernel functions in production. We
will only do these checks in a select manner if these values could
result in serious difficult to debug problems. The checks in slab look
like debugging code that someone needed for a specific debugging scenario.

I can understand that we would keep that in for development but not for
production. Maybe I am a bit biased but my prod kernels need to have
minimal memory footprint due to excessive code size causing regressions.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Shuah Khan]

On Tue, 2012-07-17 at 17:46 +0300, Pekka Enberg wrote:
> On Mon, 16 Jul 2012, David Rientjes wrote:
> >> > The kernel cannot check everything and will blow up in unexpected ways if
> >> > someone codes something stupid. There are numerous debugging options that
> >> > need to be switched on to get better debugging information to investigate
> >> > deper. Adding special code to replicate these checks is bad.
> >>
> >> Disagree, CONFIG_SLAB does not blow up for a NULL name string and just
> >> corrupts userspace.
> 
> On Tue, Jul 17, 2012 at 5:36 PM, Christoph Lameter <cl@linux.com> wrote:
> > Ohh.. So far we only had science fiction. Now kernel fiction.... If you
> > could corrupt userspace using sysfs with a NULL string then you'd first
> > need to fix sysfs support.
> >
> > And if you really want to be totally safe then I guess you need to audit
> > the kernel and make sure that every core kernel function that takes a
> > string argument does check for it to be NULL just in case.
> 
> Well, even SLUB checks for !name in mainline so that's definitely
> worth including unconditionally. Furthermore, the size related checks
> certainly make sense and I don't see any harm in having them as well.
> 
> As for "in_interrupt()", I really don't see the point in keeping that
> around. We could push it down to mm/slab.c in "__kmem_cache_create()"
> if we wanted to.

Is it safe to hold slab_mutex when in interrupt context? Pushing
in_interrupt() check down to "__kmem_cache_create()" would mean, this
check is done while holding slab_mutex. If it is not safe to be in
interrupt context, then it would a bit late to do the check?

Also all of these checks (as I see it) will allow kmem_cache_create() to
fail gracefully. I understand that kernel doesn't do this type checking
consistently, guess that is larger scope issue. Does it make sense to do
these checks in this particular case?

I am working on two different restructuring options:

1. Move all of the debug code and the regular code into
kmem_cache_create_debug() which is called from kmem_cache_create() in
ifdef CONFIG_DEBUG block and push the regular code into #else case.

I don't like this a whole lot because of the duplication of normal code
path. However, this seems to be better than the second alternative,
because of the complexity involved in taking code paths based on where
the sanity checks failed.

2. Move just the debug code block that does sanity checks on slab_caches
list and have it return failure which will result in the 

       mutex_unlock(&slab_mutex);
        put_online_cpus();

        if (!s && (flags & SLAB_PANIC))
                panic("kmem_cache_create: Failed to create slab '%s'\n",
name):

get executed from the regular code path. I like this option because it
there is no duplication of regular code. However, if for any reason
debug code changes and results conditional code paths taken after
return, it will get very complex.

Any thoughts, should I send RFC patches so we can discuss the code.


-- Shuah

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Glauber Costa]

On 07/17/2012 07:11 PM, Christoph Lameter wrote:
> On Tue, 17 Jul 2012, Pekka Enberg wrote:
> 
>> Well, even SLUB checks for !name in mainline so that's definitely
>> worth including unconditionally. Furthermore, the size related checks
>> certainly make sense and I don't see any harm in having them as well.
> 
> There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a
> NULL pointer dereference later in the caller?
> 

It obviously depends on the caller.
Although most of the calls to kmem_cache_create are made from static
data, we can't assume that. Of course whoever is using static data
should do those very same tests from the outside to be safe, but in case
they do not, this seems to fall in the category of things that make
debugging easier - even if we later on get to a NULL pointer dereference.

Your mentioned bias towards minimum code size, however, is totally
valid, IMHO. But I doubt those checks would introduce a huge footprint.
I would imagine you being much more concerned about being able to wipe
out entire subsystems like memcg, which will give you a lot more.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Christoph Lameter]

On Mon, 23 Jul 2012, Glauber Costa wrote:

> >> worth including unconditionally. Furthermore, the size related checks
> >> certainly make sense and I don't see any harm in having them as well.
> >
> > There is a WARN_ON() there and then it returns NULL!!! Crazy. Causes a
> > NULL pointer dereference later in the caller?
> >
>
> It obviously depends on the caller.

This is a violation of the calling convention to say the least. This means
if you have SLAB_PANIC set and accidentally set the name to NULL the
function will return despite the error and not panic!

> Although most of the calls to kmem_cache_create are made from static
> data, we can't assume that. Of course whoever is using static data
> should do those very same tests from the outside to be safe, but in case
> they do not, this seems to fall in the category of things that make
> debugging easier - even if we later on get to a NULL pointer dereference.
>
> Your mentioned bias towards minimum code size, however, is totally
> valid, IMHO. But I doubt those checks would introduce a huge footprint.
> I would imagine you being much more concerned about being able to wipe
> out entire subsystems like memcg, which will give you a lot more.

They are useless checks since any use of the name will also cause a NULL
pointer dereference. Same is true for interrupt checks. Checks like that
indicate a deterioration of the code base. People are afraid that
something goes wrong because they no longer understand the code so they
build a embroidery around it instead of relying on the already existing
checks at vital places. The embroidery can be useful for debugging thats
why I left it in for the CONFIG_DEBUG_VM but certainly should not be
included in production kernels.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Pekka Enberg]

On Sat, Jul 14, 2012 at 2:12 AM, Shuah Khan <shuah.khan@hp.com> 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 <shuah.khan@hp.com>

I merged this as an obvious and safe fix for current merge window. We
need to clean this up properly for v3.7.

> ---
>  mm/slab_common.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 12637ce..aa3ca5b 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -98,7 +98,9 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
>
>         s = __kmem_cache_create(name, size, align, flags, ctor);
>
> +#ifdef CONFIG_DEBUG_VM
>  oops:
> +#endif
>         mutex_unlock(&slab_mutex);
>         put_online_cpus();
>
> --
> 1.7.9.5
>
>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[David Rientjes]

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

On Mon, 30 Jul 2012, Pekka Enberg 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 <shuah.khan@hp.com>
> 
> I merged this as an obvious and safe fix for current merge window. We
> need to clean this up properly for v3.7.
> 

-Wunused-label is overridden in gcc for a label that is conditionally 
referenced by using __maybe_unused in the kernel.  I'm not sure what's so 
obscure about

out: __maybe_unused

Are label attributes really that obsecure?

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Pekka Enberg]

On Mon, Jul 30, 2012 at 10:56 PM, David Rientjes <rientjes@google.com> wrote:
> -Wunused-label is overridden in gcc for a label that is conditionally
> referenced by using __maybe_unused in the kernel.  I'm not sure what's so
> obscure about
>
> out: __maybe_unused
>
> Are label attributes really that obsecure?

I think they are.

The real problem, however, is that label attributes would just paper
over the badly thought out control flow in the function and not make the
code any better or easier to read.

                        Pekka

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[David Rientjes]

On Mon, 30 Jul 2012, Pekka Enberg wrote:

> > -Wunused-label is overridden in gcc for a label that is conditionally
> > referenced by using __maybe_unused in the kernel.  I'm not sure what's so
> > obscure about
> >
> > out: __maybe_unused
> >
> > Are label attributes really that obsecure?
> 
> I think they are.
> 
> The real problem, however, is that label attributes would just paper
> over the badly thought out control flow in the function and not make the
> code any better or easier to read.
> 

So much for compromise, I thought we had agreed that at least some of the 
checks for !name, in_interrupt() or bad size values should be moved out 
from under the #ifdef CONFIG_DEBUG_VM, but this wasn't done.  This 
discussion would be irrelevent if we actually did what we talked about.

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Pekka Enberg]

On Mon, 30 Jul 2012, David Rientjes wrote:
> So much for compromise, I thought we had agreed that at least some of the 
> checks for !name, in_interrupt() or bad size values should be moved out 
> from under the #ifdef CONFIG_DEBUG_VM, but this wasn't done.  This 
> discussion would be irrelevent if we actually did what we talked about.

I didn't want to change the checks at the last minute and invalidate 
testing in linux-next but I'm more than happy to merge such a patch when 
the merge window closes.

			Pekka

Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create()

[Shuah Khan]

On Mon, 2012-07-30 at 13:18 +0300, Pekka Enberg wrote:
> On Sat, Jul 14, 2012 at 2:12 AM, Shuah Khan <shuah.khan@hp.com> 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 <shuah.khan@hp.com>
> 
> 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

[PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Shuah Khan]

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 <shuah.khan@hp.com>
---
 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

Re: [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[JoonSoo Kim]

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

Re: [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Shuah Khan]

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

[PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Shuah Khan]

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 <shuah.khan@hp.com>
---
 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

Re: [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Christoph Lameter (Open Source)]

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.

Re: [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Shuah Khan]

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

Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Shuah Khan]

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 <shuah.khan@hp.com>
> ---
>  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);
>  

Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Christoph Lameter (Open Source)]

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.

Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Shuah Khan]

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

Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Christoph Lameter (Open Source)]

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.

Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Shuah Khan]

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

[PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Shuah Khan]

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 <shuah.khan@hp.com>
---
 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

Re: [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Christoph]

Acked-by: Christoph Lameter <cl@linux.com>



On Aug 12, 2012, at 11:40, Shuah Khan <shuah.khan@hp.com> 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 <shuah.khan@hp.com>
> ---
> 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
> 
> 

Re: [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Andrew Morton]

On Sun, 12 Aug 2012 10:40:18 -0600
Shuah Khan <shuah.khan@hp.com> 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();
 
_

Re: [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

[Pekka Enberg]

On Thu, Aug 16, 2012 at 2:53 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sun, 12 Aug 2012 10:40:18 -0600
> Shuah Khan <shuah.khan@hp.com> 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?