Re: [PATCH v3] mm/slab_common: Support the slub_debug boot option on specific object size

[Andrew Morton]

On Thu, 23 Apr 2015 21:26:00 +0800 Gavin Guo <gavin.guo@xxxxxxxxxxxxx> wrote:

> The slub_debug=PU,kmalloc-xx cannot work because in the
> create_kmalloc_caches() the s->name is created after the
> create_kmalloc_cache() is called. The name is NULL in the
> create_kmalloc_cache() so the kmem_cache_flags() would not set the
> slub_debug flags to the s->flags. The fix here set up a kmalloc_names
> string array for the initialization purpose and delete the dynamic
> name creation of kmalloc_caches.

This code is still pretty horrid :(

What's all that stuff fiddling around with size_index[], magic
constants everywhere.  Surely there's some way of making this nice and
clear: table-driven, robust to changes.

> +/*
> + * The KMALLOC_LOOP_LOW is the definition for the for loop index start number
> + * to create the kmalloc_caches object in create_kmalloc_caches(). The first
> + * and the second are 96 and 192. You can see that in the kmalloc_index(), if
> + * the KMALLOC_MIN_SIZE <= 32, then return 1 (96). If KMALLOC_MIN_SIZE <= 64,
> + * then return 2 (192). If the KMALLOC_MIN_SIZE is bigger than 64, we don't
> + * need to initialize 96 and 192. Go directly to start the KMALLOC_SHIFT_LOW.
> + */
> +#if KMALLOC_MIN_SIZE <= 32
> +#define KMALLOC_LOOP_LOW 1
> +#elif KMALLOC_MIN_SIZE <= 64
> +#define KMALLOC_LOOP_LOW 2
> +#else
> +#define KMALLOC_LOOP_LOW KMALLOC_SHIFT_LOW
> +#endif
> +
>  #else
>  #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
> +/*
> + * The KMALLOC_MIN_SIZE of slub/slab/slob is 2^3/2^5/2^3. So, even slab is used.
> + * The KMALLOC_MIN_SIZE <= 32. The kmalloc-96 and kmalloc-192 should also be
> + * initialized.
> + */
> +#define KMALLOC_LOOP_LOW 1

Hopefully we can remove the above.

>  /*
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 999bb34..05c6439 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -784,6 +784,31 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
>  }
>  
>  /*
> + * The kmalloc_names is to make slub_debug=,kmalloc-xx option work in the boot
> + * time. The kmalloc_index() support to 2^26=64MB. So, the final entry of the
> + * table is kmalloc-67108864.
> + */
> +static struct {
> +	const char *name;
> +	unsigned long size;
> +} const kmalloc_names[] __initconst = {

OK.  This table is __initconst, so the kstrtoul() trick isn't needed.

> +	{NULL,                      0},		{"kmalloc-96",             96},
> +	{"kmalloc-192",           192},		{"kmalloc-8",               8},
> +	{"kmalloc-16",             16},		{"kmalloc-32",             32},
> +	{"kmalloc-64",             64},		{"kmalloc-128",           128},
> +	{"kmalloc-256",           256},		{"kmalloc-512",           512},
> +	{"kmalloc-1024",         1024},		{"kmalloc-2048",         2048},
> +	{"kmalloc-4096",         4096},		{"kmalloc-8192",         8192},
> +	{"kmalloc-16384",       16384},		{"kmalloc-32768",       32768},
> +	{"kmalloc-65536",       65536},		{"kmalloc-131072",     131072},
> +	{"kmalloc-262144",     262144},		{"kmalloc-524288",     524288},
> +	{"kmalloc-1048576",   1048576},		{"kmalloc-2097152",   2097152},
> +	{"kmalloc-4194304",   4194304},		{"kmalloc-8388608",   8388608},
> +	{"kmalloc-16777216", 16777216},		{"kmalloc-33554432", 33554432},
> +	{"kmalloc-67108864", 67108864}
> +};
> +
>
> ...
>
> +		if (i == 2)
> +			i = (KMALLOC_SHIFT_LOW - 1);

Can we get rid of this by using something like

static struct {
	const char *name;
	unsigned long size;
} const kmalloc_names[] __initconst = {
//	{NULL,                      0},
	{"kmalloc-96",             96},
	{"kmalloc-192",           192},
#if KMALLOC_MIN_SIZE <= 8
	{"kmalloc-8",               8},
#endif
#if KMALLOC_MIN_SIZE <= 16
	{"kmalloc-16",             16},
#endif
#if KMALLOC_MIN_SIZE <= 32
	{"kmalloc-32",             32},
#endif
	{"kmalloc-64",             64},
	{"kmalloc-128",           128},
	{"kmalloc-256",           256},
	{"kmalloc-512",           512},
	{"kmalloc-1024",         1024},
	{"kmalloc-2048",         2048},
	{"kmalloc-4096",         4096},
	{"kmalloc-8192",         8192},
	...
};

(remove the zeroeth entry from kmalloc_names)

(rename kmalloc_names to kmalloc_info or something: it now holds more
than names)

and make the initialization loop do

	for (i = 0; i < ARRAY_SIZE(kmalloc_names); i++)
		kmalloc_caches[i] = ...


Why does the initialization code do the

	if (!kmalloc_caches[i]) {

test?  Can any of these really be initialized?  If so, why is it
legitimate for create_kmalloc_caches() to go altering size_index[]
after some caches have already been set up?


If this is all done right, KMALLOC_LOOP_LOW, KMALLOC_SHIFT_LOW and
KMALLOC_SHIFT_HIGH should just go away - we should be able to implement
all the logic using only KMALLOC_MIN_SIZE and MAX_ORDER.


Perhaps the manipulation of size_index[] should be done while we're
initalizing the caches, perhaps driven by additional fields in
kmalloc_info.


Finally, why does create_kmalloc_caches() use GFP_NOWAIT?  We're in
__init code!  Makes no sense.  Or if it *does* make sense, the reason
should be clearly commented.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/