[PATCH v2] slub: extend slub debug to handle multiple slabs

[PATCH v2] slub: extend slub debug to handle multiple slabs

[Aaron Tomlin]

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

Re: [PATCH v2] slub: extend slub debug to handle multiple slabs

[Andrew Morton]

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 ;)

Re: [PATCH v2] slub: extend slub debug to handle multiple slabs

[Aaron Tomlin]

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