Re: [PATCH 1/2] tools/mm/slabinfo: refactor slab attribute reading

[Harry Yoo <harry@kernel.org>]

Hi Seongjun, thanks for working on tools/mm/slabinfo improvements!
My comments inlined below.

On Sun, Sep 27, 2026 at 05:57:27AM +0000, Seongjun Hong wrote:
> The fields of struct slabinfo were mixed with deprecated sysfs files,
> which looks complicated. Organize every field of struct slabinfo by
> config option and the attribute order in mm/slub.c's slab_attrs[]. Because
> slabinfo should be able to run on previous kernel releases, leave the
> deprecated members for this time.
>
> read_slab_dir() had ~50 lines of attribute reads inlined. Move them to a
> new fill_slabinfo() helper so that directory walk and per-cache
> attribute reading are separated.
> 
> Fold get_obj_and_str() and decode_numa_list() into get_obj_and_decode()
> to avoid unnecessary strdup() and leave only one member assignment in
> fill_slabinfo() for each sysfs file.
> 
> Make broken statements into a single line for better readibility.
> 
> Remove alias field from struct slabinfo because it is initialized to 0
> and never updated. The only usage of this field "if (slab->alias)" is dead.

Would you please separate this patch into multiple patches?
It's hard to review when multiple refactorings are done in a single
patch.

> No functional change intended.
> 
> Signed-off-by: Seongjun Hong <hsj0512@snu.ac.kr>
> ---
>
>  tools/mm/slabinfo.c | 245 ++++++++++++++++++++++++++++------------------------
>  1 file changed, 131 insertions(+), 114 deletions(-)
> 
> diff --git a/tools/mm/slabinfo.c b/tools/mm/slabinfo.c
> index 48d1ee8b0e81..84359d628f2e 100644
> --- a/tools/mm/slabinfo.c
> +++ b/tools/mm/slabinfo.c
> @@ -27,25 +27,49 @@
>  
>  struct slabinfo {
>  	char *name;
> -	int alias;
>  	int refs;
> -	int aliases, align, cache_dma, cpu_slabs, destroy_by_rcu;
> -	unsigned int hwcache_align, object_size, objs_per_slab;
> -	unsigned int sanity_checks, slab_size, store_user, trace;
> -	int order, poison, reclaim_account, red_zone;
> -	unsigned long partial, objects, slabs, objects_partial, total_objects;
> +	int numa_slabs[MAX_NODES];
> +	int numa_partial[MAX_NODES];
> +
> +	unsigned int slab_size, object_size;
> +	unsigned int objs_per_slab, order;
> +	unsigned long objects_partial, partial;
> +	int aliases;
> +	unsigned int align;
> +	int hwcache_align;
> +	int reclaim_account;
> +	int destroy_by_rcu;
> +
> +	/* CONFIG_SLUB_DEBUG */
> +	unsigned long total_objects, objects, slabs;
> +	int sanity_checks, trace, red_zone, poison, store_user;
> +
> +	/* CONFIG_ZONE_DMA */
> +	int cache_dma;
> +
> +	/* CONFIG_SLUB_STATS */
>  	unsigned long alloc_fastpath, alloc_slowpath;
>  	unsigned long free_fastpath, free_slowpath;
> -	unsigned long free_frozen, free_add_partial, free_remove_partial;
> -	unsigned long alloc_from_partial, alloc_slab, free_slab, alloc_refill;
> -	unsigned long cpuslab_flush, deactivate_full, deactivate_empty;
> +	unsigned long free_add_partial, free_remove_partial;
> +	unsigned long alloc_slab, alloc_node_mismatch, free_slab;
> +	unsigned long order_fallback;
> +	unsigned long cmpxchg_double_fail;
> +
> +	/*
> +	 * Deprecated files:
> +	 * No STAT_ATTR()/SLAB_ATTR() for these exists in mm/slub.c's
> +	 * slab_attrs[] anymore.

This part of the comment looks fine, but

> Although cpu_slabs remains as a file,
> +	 * it is also outdated and always prints 0. Keep these for backward
> +	 * compatibility, but they should be removed later.

This doesn't seem useful information to put in the comment.

We were able to remove some files when
Documentation/ABI/testing/sysfs-kernel-slab says
"Available when CONFIG_SLUB_STATS is enabled", because that implies that
those files may not exist. 

But it's not the case for files like cpu_slabs,
and I don't think we're going to remove them in the future.

Probably simply say something like

"Deprecated files: the kernel does not create those files anymore or
 always prints hardecoded "0" since they are deprecated" ?

> +	 */
> +	int cpu_slabs;
> +	unsigned long free_frozen;
>  	unsigned long deactivate_to_head, deactivate_to_tail;
> -	unsigned long deactivate_remote_frees, order_fallback;
> -	unsigned long cmpxchg_double_cpu_fail, cmpxchg_double_fail;
> -	unsigned long alloc_node_mismatch, deactivate_bypass;
> +	unsigned long alloc_from_partial, alloc_refill;
> +	unsigned long cpuslab_flush, deactivate_full, deactivate_empty;
> +	unsigned long deactivate_remote_frees, deactivate_bypass;
> +	unsigned long cmpxchg_double_cpu_fail;
>  	unsigned long cpu_partial_alloc, cpu_partial_free;
> -	int numa[MAX_NODES];
> -	int numa_partial[MAX_NODES];
>  } slabinfo[MAX_SLABS];

-- 
Cheers,
Harry / Hyeonggon