slub: extend slub debug to handle multiple slabs
Subsystems (3)
PATCH v2 1/1 slub: extend slub debug to handle multiple slabs
Extend the slub_debug syntax to "slub_debug=<flags>[,<slub>]*", where <slub> may contain an asterisk at the end. For example, the following would poison all kmalloc slabs:
slub_debug=P,kmalloc*
and the following would apply the default flags to all kmalloc and all block IO slabs:
slub_debug=,bio*,kmalloc*
Please note that a similar patch was posted by Iliyan Malchev some time ago but was never merged:
https://marc.info/?l=linux-mm&m=131283905330474&w=2
---
Changes from v1 [1]:
- Add appropriate cast to address compiler warning
[1]: https://lore.kernel.org/lkml/[email protected]/
--- Documentation/vm/slub.rst | 12 +++++++++--- mm/slub.c | 34 +++++++++++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 6 deletions(-)
Documentation/vm/slub.rst +9 -3
@@ -36,9 +36,10 @@ debugging is enabled. Format: slub_debug=<Debug-Options> Enable options for all slabs -slub_debug=<Debug-Options>,<slab name> - Enable options only for select slabs +slub_debug=<Debug-Options>,<slab name1>,<slab name2>,... + Enable options only for select slabs (no spaces + after a comma) Possible debug options are::
@@ -62,7 +63,12 @@ Trying to find an issue in the dentry cache? Try:: slub_debug=,dentry -to only enable debugging on the dentry cache. +to only enable debugging on the dentry cache. You may use an asterisk at the +end of the slab name, in order to cover all slabs with the same prefix. For +example, here's how you can poison the dentry cache as well as all kmalloc +slabs: + + slub_debug=P,kmalloc-*,dentry Red zoning and tracking may realign the slab. We can just apply sanity checks to the dentry cache with::
mm/slub.c +31 -3
@@ -1283,9 +1283,37 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,/* * Enable debugging if selected on the kernel commandline. */ -if (slub_debug && (!slub_debug_slabs || (name && - !strncmp (slub_debug_slabs, name,strlen (slub_debug_slabs))))) - flags |= slub_debug; + +char *end, *n, *glob; +int len =strlen (name); + +/* If slub_debug = 0, it folds into the if conditional. */ +if (!slub_debug_slabs) +return flags | slub_debug; + + n = slub_debug_slabs; +while (*n) { +int cmplen; + + end =strchr (n,',' ); +if (!end) + end = n +strlen (n); + + glob =strnchr (n, end - n,'*' ); +if (glob) + cmplen = glob - n; +else + cmplen =max (len, (int )(end - n)); + +if (!strncmp (name, n, cmplen)) { + flags |= slub_debug; +break ; + } + +if (!*end) +break ; + n = end +1 ; + }return flags; }
Signature · 1 line
2.14.4
Andrew Morton
maintainerQuoted 9 lines · On Thu, 20 Sep 2018 21:00:16 +0100 Aaron Tomlin <[email protected]> wrote:
Extend the
slub_debugsyntax to "slub_debug=<flags>[,<slub>]*", where <slub> may contain an asterisk at the end. For example, the following would poison all kmalloc slabs:slub_debug=P,kmalloc*
and the following would apply the default flags to all kmalloc and all block IO slabs:
Quoted 6 lines
slub_debug=,bio*,kmalloc*
Please note that a similar patch was posted by Iliyan Malchev some time ago but was never merged:
https://marc.info/?l=linux-mm&m=131283905330474&w=2
Fair enough, I guess.
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1285
Quoted 6 lines
mm/slub.c +0
-0@@ -1283,9 +1283,37 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,/* * Enable debugging if selected on the kernel commandline. */
The above comment is in a strange place. Can we please move it to above the function definition in the usual fashion? And make it better, if anything seems to be missing.
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1287
Quoted 5 lines
mm/slub.c +2
-3-if (slub_debug && (!slub_debug_slabs || (name && - !strncmp (slub_debug_slabs, name,strlen (slub_debug_slabs))))) - flags |= slub_debug; + +char *end, *n, *glob;
end' and glob' could be local to the loop which uses them, which I find a bit nicer.
`n' is a rotten identifier. Can't we think of something which communicates meaning?
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1292
Quoted 5 lines
mm/slub.c +5
-0+int len =strlen (name); + +/* If slub_debug = 0, it folds into the if conditional. */ +if (!slub_debug_slabs) +return flags | slub_debug;
If we take the above return, the call to strlen() was wasted cycles. Presumably gcc is smart enough to prevent that, but why risk it.
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1306
Quoted 13 lines
mm/slub.c +13
-0+ n = slub_debug_slabs; +while (*n) { +int cmplen; + + end =strchr (n,',' ); +if (!end) + end = n +strlen (n); + + glob =strnchr (n, end - n,'*' ); +if (glob) + cmplen = glob - n; +else + cmplen =max (len, (int )(end - n));
max_t() exists for this. Or maybe make `len' size_t, but I expect that will still warn - that subtraction returns a ptrdiff_t, yes?
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1316
Quoted 10 lines
mm/slub.c +10
-0+ +if (!strncmp (name, n, cmplen)) { + flags |= slub_debug; +break ; + } + +if (!*end) +break ; + n = end +1 ; + }
The code in this loop hurts my brain a bit. I hope it's correct ;)
Quoted 11 lines · Andrew Morton
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1285
Quoted 6 lines · On Thu, 20 Sep 2018 21:00:16 +0100 Aaron Tomlin <[email protected]> wrote:
mm/slub.c +0
-0@@ -1283,9 +1283,37 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,/* * Enable debugging if selected on the kernel commandline. */ The above comment is in a strange place. Can we please move it to above the function definition in the usual fashion? And make it better, if anything seems to be missing.
OK.
Quoted 8 lines
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1287
Quoted 5 lines
mm/slub.c +2
-3-if (slub_debug && (!slub_debug_slabs || (name && - !strncmp (slub_debug_slabs, name,strlen (slub_debug_slabs))))) - flags |= slub_debug; + +char *end, *n, *glob;
end' andglob' could be local to the loop which uses them, which I find a bit nicer.
OK.
Quoted 2 lines
`n' is a rotten identifier. Can't we think of something which communicates meaning?
OK.
Quoted 8 lines
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1292
Quoted 5 lines
mm/slub.c +5
-0+int len =strlen (name); + +/* If slub_debug = 0, it folds into the if conditional. */ +if (!slub_debug_slabs) +return flags | slub_debug;If we take the above return, the call to
strlen()was wasted cycles. Presumably gcc is smart enough to prevent that, but why risk it.
OK.
Quoted 8 lines
Quoted 8 lines
mm/slub.c +8
-0+ n = slub_debug_slabs; +while (*n) { +int cmplen; + + end =strchr (n,',' ); +if (!end) + end = n +strlen (n); +
Quoted 8 lines
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1306
Quoted 5 lines
mm/slub.c +5
-0+ glob =strnchr (n, end - n,'*' ); +if (glob) + cmplen = glob - n; +else + cmplen =max (len, (int )(end - n));
max_t()exists for this. Or maybe make `len'size_t, but I expect that will still warn - that subtraction returns aptrdiff_t, yes?
I think max_t(size_t, ...) should be appropriate?
I'll address the above and in the next version.
Quoted 11 lines
mm/slub.c slab_flags_t kmem_cache_flags(unsigned int object_size, · line 1316
Quoted 10 lines
mm/slub.c +10
-0+ +if (!strncmp (name, n, cmplen)) { + flags |= slub_debug; +break ; + } + +if (!*end) +break ; + n = end +1 ; + }The code in this loop hurts my brain a bit. I hope it's correct ;)
It works :)
Signature · 1 line
Aaron Tomlin