| Message ID | 20180920200016.11003-1-atomlin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] slub: extend slub debug to handle multiple slabs | expand |
On Thu, 20 Sep 2018 21:00:16 +0100 Aaron Tomlin <atomlin@redhat.com> wrote: > 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 Fair enough, I guess. > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -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. > - 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? > + 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. > + 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? > + > + 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 ;)
On Fri 2018-09-21 16:34 -0700, Andrew Morton wrote: > On Thu, 20 Sep 2018 21:00:16 +0100 Aaron Tomlin <atomlin@redhat.com> wrote: > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -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. > > - 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. OK. > `n' is a rotten identifier. Can't we think of something which > communicates meaning? OK. > > + 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. > > + 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? I think max_t(size_t, ...) should be appropriate? I'll address the above and in the next version. > > + > > + 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 :)
diff --git a/Documentation/vm/slub.rst b/Documentation/vm/slub.rst index 3a775fd64e2d..195928808bac 100644 --- a/Documentation/vm/slub.rst +++ b/Documentation/vm/slub.rst @@ -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:: diff --git a/mm/slub.c b/mm/slub.c index 8da34a8af53d..d20901514075 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -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; }
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 Signed-off-by: Aaron Tomlin <atomlin@redhat.com> --- Changes from v1 [1]: - Add appropriate cast to address compiler warning [1]: https://lore.kernel.org/lkml/20180910111358.10539-1-atomlin@redhat.com/ --- Documentation/vm/slub.rst | 12 +++++++++--- mm/slub.c | 34 +++++++++++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 6 deletions(-)