* [PATCH v2] slub: extend slub debug to handle multiple slabs
@ 2018-09-20 20:00 Aaron Tomlin
2018-09-21 23:34 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: Aaron Tomlin @ 2018-09-20 20:00 UTC (permalink / raw)
To: cl, penberg, rientjes, iamjoonsoo.kim, akpm
Cc: linux-mm, linux-kernel, atomlin
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(-)
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;
}
--
2.14.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] slub: extend slub debug to handle multiple slabs
2018-09-20 20:00 [PATCH v2] slub: extend slub debug to handle multiple slabs Aaron Tomlin
@ 2018-09-21 23:34 ` Andrew Morton
2018-09-28 10:05 ` Aaron Tomlin
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2018-09-21 23:34 UTC (permalink / raw)
To: Aaron Tomlin
Cc: cl, penberg, rientjes, iamjoonsoo.kim, linux-mm, linux-kernel
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 ;)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] slub: extend slub debug to handle multiple slabs
2018-09-21 23:34 ` Andrew Morton
@ 2018-09-28 10:05 ` Aaron Tomlin
0 siblings, 0 replies; 3+ messages in thread
From: Aaron Tomlin @ 2018-09-28 10:05 UTC (permalink / raw)
To: Andrew Morton
Cc: cl, penberg, rientjes, iamjoonsoo.kim, linux-mm, linux-kernel
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 :)
--
Aaron Tomlin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-09-28 10:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-20 20:00 [PATCH v2] slub: extend slub debug to handle multiple slabs Aaron Tomlin
2018-09-21 23:34 ` Andrew Morton
2018-09-28 10:05 ` Aaron Tomlin
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).