Re: [PATCH v2 2/7] mm, slab: unlink slabinfo, sysfs and debugfs immediately

Jann Horn
2024-08-07T19:12:24Z

On Wed, Aug 7, 2024 at 12:31 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> kmem_cache_destroy() includes removing the associated sysfs and debugfs
> directories, and the cache from the list of caches that appears in
> /proc/slabinfo. Currently this might not happen immediately when:
>
> - the cache is SLAB_TYPESAFE_BY_RCU and the cleanup is delayed,
> including the directores removal
> - __kmem_cache_shutdown() fails due to outstanding objects - the
> directories remain indefinitely
>
> When a cache is recreated with the same name, such as due to module
> unload followed by a load, the directories will fail to be recreated for
> the new instance of the cache due to the old directories being present.
> The cache will also appear twice in /proc/slabinfo.
>
> While we want to convert the SLAB_TYPESAFE_BY_RCU cleanup to be
> synchronous again, the second point remains. So let's fix this first and
> have the directories and slabinfo removed immediately in
> kmem_cache_destroy() and regardless of __kmem_cache_shutdown() success.
>
> This should not make debugging harder if __kmem_cache_shutdown() fails,
> because a detailed report of outstanding objects is printed into dmesg
> already due to the failure.

Reading this sentence made be curious what __kmem_cache_shutdown()
actually does - and I think technically, it prints a report of only
the outstanding objects *on the first NUMA node with outstanding
objects*? __kmem_cache_shutdown() bails immediately after seeing one
node with outstanding objects.

That's not really relevant to this series though, just a random observation.

> Also simplify kmem_cache_release() sysfs handling by using
> __is_defined(SLAB_SUPPORTS_SYSFS).
>
> Note the resulting code in kmem_cache_destroy() is a bit ugly but will
> be further simplified - this is in order to make small bisectable steps.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Reviewed-by: Jann Horn <jannh@google.com>