Re: [PATCH V2 2/2] Documentation: filesystems: introduce proc/slabinfo to users

[zhang fangzheng <fangzheng.zhang1003@gmail.com>]

On Mon, Feb 19, 2024 at 4:09 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 2/19/24 07:23, zhang fangzheng wrote:
> > On Mon, Feb 19, 2024 at 12:24 PM Matthew Wilcox <willy@infradead.org> wrote:
> >>
> >> On Mon, Feb 19, 2024 at 11:19:11AM +0800, Fangzheng Zhang wrote:
> >> > +Note, <slabreclaim> comes from the collected results in the file
> >> > +/sys/kernel/slab/$cache/reclaim_account. Next, we will mark /proc/slabinfo
> >> > +as deprecated and recommend the use of either sysfs directly or
> >> > +use of the "slabinfo" tool that we have been providing in linux/tools/mm.
> >>
> >> Wait, so you're going to all of the trouble of changing the format of
> >> slabinfo (with the associated costs of updating every tool that currently
> >> parses it), only to recommend that we stop using it and start using
> >> tools/mm/slabinfo instead?
> >>
>
> Hi,
>
> > The initial purpose was to obtain the type of each slab through
> > a simple command 'cat proc/slabinfo'. So here, my intention is not to
> > update all slabinfo-related tools for the time being, but to modify
> > the version number of proc/slabinfo and further display the results
> > of using the command.
>
> I'm not sure you understand the concern. There are existing consumers of
> /proc/slabinfo, that might become broken by patch 1/2. We don't even know
> them all, they might not be all opensource etc. So we can't even make sure
> all of them are updated. What can happen after patch 1/2:
> - they keep working and ignore the new column (good)
> - they include a version check and notice a new unsupported version and
> refuse to work
> - confused by the new column they start throwing error, or report wrong
> stats (that's worse)
>
I generally understand your concerns about modifying patch 1/2.

But judging from my modifications, this worry does not seem to be valid.
Because the “/proc/slabinfo” is not used in related slabinfo debugging tools
(such as tools/mm/slabinfo), but "/sys/kernel/slab/<slab_name>/" (in
Documentation/mm/slub.rst) or "/ sys/kernel/debug/slab" (in
tools/mm/slabinfo.c).

Furthermore, the current modification only involves optimizing the output
of proc/slabinfo, and does not modify the  struct slabinfo or struct kmem_cache.
So there is no need to adapt other modifications.

> >> How about we simply do nothing?
>
> Agreed wrt modifying /proc/slabinfo
>
> > The note here means what changes will occur after
> > we modify the version number of proc/slabinfo to 2.2.
> > As for the replacement of tools/mm/slabinfo (that inspired
> > by Christoph’s suggestions), it will be implemented in the next version
> > or even the later version.
>
> So what is your motivation for all this in the first place? You have some
> monitoring tool that relies on /proc/slabinfo and want to distinguish
> reclaimable caches? So you can change it to parse the /sys directories. Is
> it more work? Yes, but you only have to do that once per boot, because
> unlike the object/memory stats in /proc/slabinfo, the reclaimable flag will
> not change for a cache.
>
The situation as you mentioned is very suitable for my current needs.
My original intention is just to get an intuitive slab screen through a
simple ‘cat proc/slabinfo’ command. As for the description "<slabreclaim>
comes from the collected results in the file
/sys/kernel/slab/$cache/reclaim_account"
may not be appropriate. Here I want to express that the column <slabreclaim> has
the same effect as traversing "/sys/kernel/slab/$ cache/reclaim_account".

> Would tools/mm/slabinfo almost work for you, but you're missing something?
> Then send patches for that in the first place. Changing /proc/slabinfo (and
> breaking other consumers) for a quick and easy fix with a different solution
> planned for the future is simply not feasible.
>
Using the slabinfo tool to parse /sys/kernel/slab/$cache/reclaim_account
is what I think about optimizing future tools during the discussion.
It will not affect the current patch 1/2, and patch 2/2 is mainly to
supplement the output examples of proc/slabinfo.

If the community is willing to accept it, I will only modify
patch 1/2 to implement it.

Thanks very much!

> HTH,
> Vlastimil
>
> > Thanks!
>