Message-ID: <60edefec-0a78-4c23-bfb6-17ebf326c61a@suse.cz>
Date: Tue, 20 Feb 2024 10:21:24 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: zhang fangzheng <fangzheng.zhang1003@...il.com>
Cc: Matthew Wilcox <willy@...radead.org>,
 Fangzheng Zhang <fangzheng.zhang@...soc.com>,
 Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
 David Rientjes <rientjes@...gle.com>, Joonsoo Kim <iamjoonsoo.kim@....com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 Hyeonggon Yoo <42.hyeyoo@...il.com>, Greg KH <gregkh@...uxfoundation.org>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org, tkjos@...gle.com,
 Yuming Han <yuming.han@...soc.com>, Chunyan Zhang <zhang.lyra@...il.com>
Subject: Re: [PATCH V2 2/2] Documentation: filesystems: introduce
 proc/slabinfo to users

On 2/20/24 09:49, zhang fangzheng wrote:
> On Mon, Feb 19, 2024 at 4:09 PM Vlastimil Babka <vbabka@...e.cz> wrote:
>>
>> On 2/19/24 07:23, zhang fangzheng wrote:
>> > On Mon, Feb 19, 2024 at 12:24 PM Matthew Wilcox <willy@...radead.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),

Hi,

we are not concerned about slabinfo debugging tools that are part of kernel
source tree, but about those outside, including those created privately and
we don't even know they exist.

> 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,

It's not "only", the output of /proc/slabinfo is what those tools consume,
so that's what concerns us the most.

> and does not modify the  struct slabinfo or struct kmem_cache.
> So there is no need to adapt other modifications.

These on the other hand are internal details of the kernel which we can
modify as much we want

>> >> 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>

That would be nice, but again we must be careful about existing consumers of
/proc/slabinfo so we can't always have nice things.

> 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!
>>

Powered by blists - more mailing lists