* [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() @ 2012-07-13 23:12 Shuah Khan 2012-07-14 9:18 ` David Rientjes 2012-07-30 10:18 ` Pekka Enberg 0 siblings, 2 replies; 37+ messages in thread From: Shuah Khan @ 2012-07-13 23:12 UTC (permalink / raw) To: cl, penberg, glommer, js1304; +Cc: shuahkhan, linux-mm, LKML 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-13 23:12 [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() Shuah Khan @ 2012-07-14 9:18 ` David Rientjes 2012-07-14 12:01 ` Pekka Enberg 2012-07-30 10:18 ` Pekka Enberg 1 sibling, 1 reply; 37+ messages in thread From: David Rientjes @ 2012-07-14 9:18 UTC (permalink / raw) To: Shuah Khan Cc: cl, penberg, glommer, js1304, shuahkhan, linux-mm, linux-kernel 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-14 9:18 ` David Rientjes @ 2012-07-14 12:01 ` Pekka Enberg 2012-07-16 3:04 ` Shuah Khan 0 siblings, 1 reply; 37+ messages in thread From: Pekka Enberg @ 2012-07-14 12:01 UTC (permalink / raw) To: David Rientjes Cc: Shuah Khan, cl, glommer, js1304, shuahkhan, linux-mm, linux-kernel 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-14 12:01 ` Pekka Enberg @ 2012-07-16 3:04 ` Shuah Khan 2012-07-16 9:58 ` David Rientjes 0 siblings, 1 reply; 37+ messages in thread From: Shuah Khan @ 2012-07-16 3:04 UTC (permalink / raw) To: Pekka Enberg Cc: David Rientjes, cl, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-16 3:04 ` Shuah Khan @ 2012-07-16 9:58 ` David Rientjes 2012-07-16 14:17 ` Christoph Lameter 0 siblings, 1 reply; 37+ messages in thread From: David Rientjes @ 2012-07-16 9:58 UTC (permalink / raw) To: Shuah Khan Cc: Pekka Enberg, cl, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-16 9:58 ` David Rientjes @ 2012-07-16 14:17 ` Christoph Lameter 2012-07-16 15:56 ` Shuah Khan 2012-07-16 19:58 ` David Rientjes 0 siblings, 2 replies; 37+ messages in thread From: Christoph Lameter @ 2012-07-16 14:17 UTC (permalink / raw) To: David Rientjes Cc: Shuah Khan, Pekka Enberg, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-16 14:17 ` Christoph Lameter @ 2012-07-16 15:56 ` Shuah Khan 2012-07-16 19:58 ` David Rientjes 1 sibling, 0 replies; 37+ messages in thread From: Shuah Khan @ 2012-07-16 15:56 UTC (permalink / raw) To: Christoph Lameter Cc: David Rientjes, Pekka Enberg, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-16 14:17 ` Christoph Lameter 2012-07-16 15:56 ` Shuah Khan @ 2012-07-16 19:58 ` David Rientjes 2012-07-16 20:14 ` Christoph Lameter 1 sibling, 1 reply; 37+ messages in thread From: David Rientjes @ 2012-07-16 19:58 UTC (permalink / raw) To: Christoph Lameter Cc: Shuah Khan, Pekka Enberg, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-16 19:58 ` David Rientjes @ 2012-07-16 20:14 ` Christoph Lameter 2012-07-16 23:48 ` David Rientjes 0 siblings, 1 reply; 37+ messages in thread From: Christoph Lameter @ 2012-07-16 20:14 UTC (permalink / raw) To: David Rientjes Cc: Shuah Khan, Pekka Enberg, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-16 20:14 ` Christoph Lameter @ 2012-07-16 23:48 ` David Rientjes 2012-07-17 14:36 ` Christoph Lameter 0 siblings, 1 reply; 37+ messages in thread From: David Rientjes @ 2012-07-16 23:48 UTC (permalink / raw) To: Christoph Lameter Cc: Shuah Khan, Pekka Enberg, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-16 23:48 ` David Rientjes @ 2012-07-17 14:36 ` Christoph Lameter 2012-07-17 14:46 ` Pekka Enberg 0 siblings, 1 reply; 37+ messages in thread From: Christoph Lameter @ 2012-07-17 14:36 UTC (permalink / raw) To: David Rientjes Cc: Shuah Khan, Pekka Enberg, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-17 14:36 ` Christoph Lameter @ 2012-07-17 14:46 ` Pekka Enberg 2012-07-17 15:11 ` Christoph Lameter 2012-07-17 16:52 ` Shuah Khan 0 siblings, 2 replies; 37+ messages in thread From: Pekka Enberg @ 2012-07-17 14:46 UTC (permalink / raw) To: Christoph Lameter Cc: David Rientjes, Shuah Khan, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-17 14:46 ` Pekka Enberg @ 2012-07-17 15:11 ` Christoph Lameter 2012-07-23 7:04 ` Glauber Costa 2012-07-17 16:52 ` Shuah Khan 1 sibling, 1 reply; 37+ messages in thread From: Christoph Lameter @ 2012-07-17 15:11 UTC (permalink / raw) To: Pekka Enberg Cc: David Rientjes, Shuah Khan, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-17 15:11 ` Christoph Lameter @ 2012-07-23 7:04 ` Glauber Costa 2012-07-25 15:28 ` Christoph Lameter 0 siblings, 1 reply; 37+ messages in thread From: Glauber Costa @ 2012-07-23 7:04 UTC (permalink / raw) To: Christoph Lameter Cc: Pekka Enberg, David Rientjes, Shuah Khan, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-23 7:04 ` Glauber Costa @ 2012-07-25 15:28 ` Christoph Lameter 0 siblings, 0 replies; 37+ messages in thread From: Christoph Lameter @ 2012-07-25 15:28 UTC (permalink / raw) To: Glauber Costa Cc: Pekka Enberg, David Rientjes, Shuah Khan, js1304, linux-mm, linux-kernel, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-17 14:46 ` Pekka Enberg 2012-07-17 15:11 ` Christoph Lameter @ 2012-07-17 16:52 ` Shuah Khan 1 sibling, 0 replies; 37+ messages in thread From: Shuah Khan @ 2012-07-17 16:52 UTC (permalink / raw) To: Pekka Enberg Cc: Christoph Lameter, David Rientjes, glommer, js1304, linux-mm, linux-kernel, shuahkhan 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-13 23:12 [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() Shuah Khan 2012-07-14 9:18 ` David Rientjes @ 2012-07-30 10:18 ` Pekka Enberg 2012-07-30 19:56 ` David Rientjes 2012-08-06 3:41 ` Shuah Khan 1 sibling, 2 replies; 37+ messages in thread From: Pekka Enberg @ 2012-07-30 10:18 UTC (permalink / raw) To: shuah.khan Cc: cl, glommer, js1304, shuahkhan, linux-mm, LKML, Andrew Morton, Linus Torvalds, David Rientjes 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> ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-30 10:18 ` Pekka Enberg @ 2012-07-30 19:56 ` David Rientjes 2012-07-30 20:41 ` Pekka Enberg 2012-08-06 3:41 ` Shuah Khan 1 sibling, 1 reply; 37+ messages in thread From: David Rientjes @ 2012-07-30 19:56 UTC (permalink / raw) To: Pekka Enberg Cc: shuah.khan, cl, glommer, js1304, shuahkhan, linux-mm, LKML, Andrew Morton, Linus Torvalds [-- 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? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-30 19:56 ` David Rientjes @ 2012-07-30 20:41 ` Pekka Enberg 2012-07-31 2:07 ` David Rientjes 0 siblings, 1 reply; 37+ messages in thread From: Pekka Enberg @ 2012-07-30 20:41 UTC (permalink / raw) To: David Rientjes Cc: shuah.khan, cl, glommer, js1304, shuahkhan, linux-mm, LKML, Andrew Morton, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-30 20:41 ` Pekka Enberg @ 2012-07-31 2:07 ` David Rientjes 2012-07-31 6:05 ` Pekka Enberg 0 siblings, 1 reply; 37+ messages in thread From: David Rientjes @ 2012-07-31 2:07 UTC (permalink / raw) To: Pekka Enberg Cc: shuah.khan, cl, glommer, js1304, shuahkhan, linux-mm, LKML, Andrew Morton, Linus Torvalds 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-31 2:07 ` David Rientjes @ 2012-07-31 6:05 ` Pekka Enberg 0 siblings, 0 replies; 37+ messages in thread From: Pekka Enberg @ 2012-07-31 6:05 UTC (permalink / raw) To: David Rientjes Cc: shuah.khan, cl, glommer, js1304, shuahkhan, linux-mm, LKML, Andrew Morton, Linus Torvalds 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() 2012-07-30 10:18 ` Pekka Enberg 2012-07-30 19:56 ` David Rientjes @ 2012-08-06 3:41 ` Shuah Khan 2012-08-06 15:14 ` [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function Shuah Khan 1 sibling, 1 reply; 37+ messages in thread From: Shuah Khan @ 2012-08-06 3:41 UTC (permalink / raw) To: Pekka Enberg Cc: cl, glommer, js1304, linux-mm, LKML, Andrew Morton, Linus Torvalds, David Rientjes, shuahkhan 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-06 3:41 ` Shuah Khan @ 2012-08-06 15:14 ` Shuah Khan 2012-08-06 16:49 ` JoonSoo Kim 0 siblings, 1 reply; 37+ messages in thread From: Shuah Khan @ 2012-08-06 15:14 UTC (permalink / raw) To: Pekka Enberg Cc: cl, glommer, js1304, linux-mm, LKML, Andrew Morton, Linus Torvalds, David Rientjes, shuahkhan 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-06 15:14 ` [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function Shuah Khan @ 2012-08-06 16:49 ` JoonSoo Kim 2012-08-06 17:03 ` Shuah Khan 0 siblings, 1 reply; 37+ messages in thread From: JoonSoo Kim @ 2012-08-06 16:49 UTC (permalink / raw) To: shuah.khan Cc: Pekka Enberg, cl, glommer, linux-mm, LKML, Andrew Morton, Linus Torvalds, David Rientjes, shuahkhan > 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-06 16:49 ` JoonSoo Kim @ 2012-08-06 17:03 ` Shuah Khan 2012-08-06 21:13 ` [PATCH v2] " Shuah Khan 2012-08-08 14:14 ` [PATCH RESEND] " Christoph Lameter (Open Source) 0 siblings, 2 replies; 37+ messages in thread From: Shuah Khan @ 2012-08-06 17:03 UTC (permalink / raw) To: JoonSoo Kim Cc: Pekka Enberg, cl, glommer, linux-mm, LKML, Andrew Morton, Linus Torvalds, David Rientjes, shuahkhan 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-06 17:03 ` Shuah Khan @ 2012-08-06 21:13 ` Shuah Khan 2012-08-09 14:06 ` Shuah Khan 2012-08-09 14:13 ` Christoph Lameter (Open Source) 2012-08-08 14:14 ` [PATCH RESEND] " Christoph Lameter (Open Source) 1 sibling, 2 replies; 37+ messages in thread From: Shuah Khan @ 2012-08-06 21:13 UTC (permalink / raw) To: cl, penberg, glommer, js1304, David Rientjes Cc: linux-mm, LKML, Andrew Morton, Linus Torvalds, shuahkhan 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-06 21:13 ` [PATCH v2] " Shuah Khan @ 2012-08-09 14:06 ` Shuah Khan 2012-08-09 14:13 ` Christoph Lameter (Open Source) 1 sibling, 0 replies; 37+ messages in thread From: Shuah Khan @ 2012-08-09 14:06 UTC (permalink / raw) To: cl Cc: penberg, glommer, js1304, David Rientjes, linux-mm, LKML, Andrew Morton, Linus Torvalds, 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); > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-06 21:13 ` [PATCH v2] " Shuah Khan 2012-08-09 14:06 ` Shuah Khan @ 2012-08-09 14:13 ` Christoph Lameter (Open Source) 2012-08-09 17:01 ` Shuah Khan 1 sibling, 1 reply; 37+ messages in thread From: Christoph Lameter (Open Source) @ 2012-08-09 14:13 UTC (permalink / raw) To: Shuah Khan Cc: penberg, glommer, js1304, David Rientjes, linux-mm, LKML, Andrew Morton, Linus Torvalds, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-09 14:13 ` Christoph Lameter (Open Source) @ 2012-08-09 17:01 ` Shuah Khan 2012-08-09 19:08 ` Christoph Lameter (Open Source) 0 siblings, 1 reply; 37+ messages in thread From: Shuah Khan @ 2012-08-09 17:01 UTC (permalink / raw) To: Christoph Lameter (Open Source) Cc: penberg, glommer, js1304, David Rientjes, linux-mm, LKML, Andrew Morton, Linus Torvalds, shuahkhan 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-09 17:01 ` Shuah Khan @ 2012-08-09 19:08 ` Christoph Lameter (Open Source) 2012-08-09 19:33 ` Shuah Khan 0 siblings, 1 reply; 37+ messages in thread From: Christoph Lameter (Open Source) @ 2012-08-09 19:08 UTC (permalink / raw) To: Shuah Khan Cc: penberg, glommer, js1304, David Rientjes, linux-mm, LKML, Andrew Morton, Linus Torvalds, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-09 19:08 ` Christoph Lameter (Open Source) @ 2012-08-09 19:33 ` Shuah Khan 2012-08-12 16:40 ` [PATCH v3] " Shuah Khan 0 siblings, 1 reply; 37+ messages in thread From: Shuah Khan @ 2012-08-09 19:33 UTC (permalink / raw) To: Christoph Lameter (Open Source) Cc: penberg, glommer, js1304, David Rientjes, linux-mm, LKML, Andrew Morton, Linus Torvalds, 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-09 19:33 ` Shuah Khan @ 2012-08-12 16:40 ` Shuah Khan 2012-08-12 17:36 ` Christoph 2012-08-15 23:53 ` Andrew Morton 0 siblings, 2 replies; 37+ messages in thread From: Shuah Khan @ 2012-08-12 16:40 UTC (permalink / raw) To: Christoph Lameter (Open Source) Cc: penberg, glommer, js1304, David Rientjes, linux-mm, LKML, Andrew Morton, Linus Torvalds, shuahkhan 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 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-12 16:40 ` [PATCH v3] " Shuah Khan @ 2012-08-12 17:36 ` Christoph 2012-08-15 23:53 ` Andrew Morton 1 sibling, 0 replies; 37+ messages in thread From: Christoph @ 2012-08-12 17:36 UTC (permalink / raw) To: shuah.khan Cc: penberg, glommer, js1304, DavidRientjes, linux-mm, LKML, Andrew Morton, Linus Torvalds, shuahkhan 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 > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-12 16:40 ` [PATCH v3] " Shuah Khan 2012-08-12 17:36 ` Christoph @ 2012-08-15 23:53 ` Andrew Morton 2012-08-16 6:40 ` Pekka Enberg 1 sibling, 1 reply; 37+ messages in thread From: Andrew Morton @ 2012-08-15 23:53 UTC (permalink / raw) To: shuah.khan Cc: Christoph Lameter (Open Source), penberg, glommer, js1304, David Rientjes, linux-mm, LKML, Linus Torvalds, shuahkhan 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(); _ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-15 23:53 ` Andrew Morton @ 2012-08-16 6:40 ` Pekka Enberg 0 siblings, 0 replies; 37+ messages in thread From: Pekka Enberg @ 2012-08-16 6:40 UTC (permalink / raw) To: Andrew Morton Cc: shuah.khan, Christoph Lameter (Open Source), glommer, js1304, David Rientjes, linux-mm, LKML, Linus Torvalds, shuahkhan 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? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-06 17:03 ` Shuah Khan 2012-08-06 21:13 ` [PATCH v2] " Shuah Khan @ 2012-08-08 14:14 ` Christoph Lameter (Open Source) 2012-08-08 15:13 ` Shuah Khan 1 sibling, 1 reply; 37+ messages in thread From: Christoph Lameter (Open Source) @ 2012-08-08 14:14 UTC (permalink / raw) To: Shuah Khan Cc: JoonSoo Kim, Pekka Enberg, glommer, linux-mm, LKML, Andrew Morton, Linus Torvalds, David Rientjes, shuahkhan 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function 2012-08-08 14:14 ` [PATCH RESEND] " Christoph Lameter (Open Source) @ 2012-08-08 15:13 ` Shuah Khan 0 siblings, 0 replies; 37+ messages in thread From: Shuah Khan @ 2012-08-08 15:13 UTC (permalink / raw) To: Christoph Lameter (Open Source) Cc: JoonSoo Kim, Pekka Enberg, glommer, linux-mm, LKML, Andrew Morton, Linus Torvalds, David Rientjes, 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2012-08-16 6:40 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-13 23:12 [PATCH TRIVIAL] mm: Fix build warning in kmem_cache_create() Shuah Khan 2012-07-14 9:18 ` David Rientjes 2012-07-14 12:01 ` Pekka Enberg 2012-07-16 3:04 ` Shuah Khan 2012-07-16 9:58 ` David Rientjes 2012-07-16 14:17 ` Christoph Lameter 2012-07-16 15:56 ` Shuah Khan 2012-07-16 19:58 ` David Rientjes 2012-07-16 20:14 ` Christoph Lameter 2012-07-16 23:48 ` David Rientjes 2012-07-17 14:36 ` Christoph Lameter 2012-07-17 14:46 ` Pekka Enberg 2012-07-17 15:11 ` Christoph Lameter 2012-07-23 7:04 ` Glauber Costa 2012-07-25 15:28 ` Christoph Lameter 2012-07-17 16:52 ` Shuah Khan 2012-07-30 10:18 ` Pekka Enberg 2012-07-30 19:56 ` David Rientjes 2012-07-30 20:41 ` Pekka Enberg 2012-07-31 2:07 ` David Rientjes 2012-07-31 6:05 ` Pekka Enberg 2012-08-06 3:41 ` Shuah Khan 2012-08-06 15:14 ` [PATCH RESEND] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function Shuah Khan 2012-08-06 16:49 ` JoonSoo Kim 2012-08-06 17:03 ` Shuah Khan 2012-08-06 21:13 ` [PATCH v2] " Shuah Khan 2012-08-09 14:06 ` Shuah Khan 2012-08-09 14:13 ` Christoph Lameter (Open Source) 2012-08-09 17:01 ` Shuah Khan 2012-08-09 19:08 ` Christoph Lameter (Open Source) 2012-08-09 19:33 ` Shuah Khan 2012-08-12 16:40 ` [PATCH v3] " Shuah Khan 2012-08-12 17:36 ` Christoph 2012-08-15 23:53 ` Andrew Morton 2012-08-16 6:40 ` Pekka Enberg 2012-08-08 14:14 ` [PATCH RESEND] " Christoph Lameter (Open Source) 2012-08-08 15:13 ` Shuah Khan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).