Re: [PATCH net-next v5 3/7] mm/slab: Add kmem_buckets_destroy()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new comment in kmem_buckets_destroy() (mm/slab_common.c)
  says: "Sizes below arch_slab_minalign() share one cache, which…

--

mm/slab: Add kmem_buckets_destroy()

This adds kmem_buckets_destroy() as a counterpart to kmem_buckets_create(),
so that KUnit tests built as modules can tear down a bucket set. The
create path's bitmap-based error unwinding is replaced with a call to the
new helper. The helper clears every aliased pointer to a cache before
destroying it.

Link: https://lore.kernel.org/all/20240809073309.2134488-1-kees@xxxxxxxxxx/

> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 71926f726a07a..bdc34a5e6d6f5 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c

[ ... ]

> +void kmem_buckets_destroy(kmem_buckets *bucket)
> +{
> +	unsigned int idx, i;
> +
> +	if (!IS_ENABLED(CONFIG_SLAB_BUCKETS) || ZERO_OR_NULL_PTR(bucket))
> +		return;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(kmalloc_caches[KMALLOC_NORMAL]); idx++) {
> +		struct kmem_cache *cache = (*bucket)[idx];
> +
> +		if (!cache)
> +			continue;
> +
> +		/*
> +		 * Sizes below arch_slab_minalign() share one cache, which
> +		 * kmem_buckets_create() then stores at each of their indices.

[Severity: Low]
This isn't a bug, but is this comment accurate about where the aliasing
comes from?

The aliasing in kmalloc_caches[] is set up by new_kmalloc_cache(). It uses
__kmalloc_minalign(), and that is not arch_slab_minalign() on its own:

mm/slab_common.c:__kmalloc_minalign() {
	unsigned int minalign = dma_get_cache_alignment();
	...
	return max(minalign, arch_slab_minalign());
}

new_kmalloc_cache() then rounds every size up to that alignment:

	if (minalign > ARCH_KMALLOC_MINALIGN) {
		aligned_size = ALIGN(aligned_size, minalign);
		aligned_idx = __kmalloc_index(aligned_size, false);
	}

This aliases sizes that are not multiples of minalign, even when they are
not below it. For example, kmalloc-96 maps to the 128 cache when minalign
is 64, and kmalloc-192 maps to the 256 cache when minalign is 128.

kmem_buckets_create() reads object_size from the already-aliased
kmalloc_caches[KMALLOC_NORMAL][idx], so the same aliasing ends up in the
bucket set.

The code itself is fine, because the inner loop clears every matching
pointer whatever caused the alias. Could the comment say that sizes are
rounded up to the kmalloc minimum alignment, rather than "sizes below
arch_slab_minalign()"?

The same wording also survives in the later commit "mm/slab: Let a
bucket set handle __GFP_ACCOUNT". That commit rewords this comment but
keeps "sizes below arch_slab_minalign() share one".

> +		 * Drop every reference to it before destroying it, so that no
> +		 * later pass reads a pointer to a cache that is already gone.
> +		 */
> +		for (i = idx; i < ARRAY_SIZE(kmalloc_caches[KMALLOC_NORMAL]); i++)
> +			if ((*bucket)[i] == cache)
> +				(*bucket)[i] = NULL;
> +
> +		kmem_cache_destroy(cache);
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002231120.late.500-kees%40kernel.org