Received-SPF: pass (google.com: domain of linux-kernel+bounces-72733-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240219031911.10372-1-fangzheng.zhang@unisoc.com>
 <20240219031911.10372-3-fangzheng.zhang@unisoc.com> <ZdLX51r1mOEZKUje@casper.infradead.org>
 <CA+kNDJ+C2b520afauSWbfNK=S1XiNHR_zF32_K-3Rf7R6m3n5Q@mail.gmail.com>
 <4591b2b3-398f-402e-b21d-55b244f05a2e@suse.cz> <CA+kNDJLCEdeQsaaLggxbUzF4mAqk_ZLKe=o3cnRkZO8_EKhoSQ@mail.gmail.com>
 <60edefec-0a78-4c23-bfb6-17ebf326c61a@suse.cz>
In-Reply-To: <60edefec-0a78-4c23-bfb6-17ebf326c61a@suse.cz>
From: zhang fangzheng <fangzheng.zhang1003@gmail.com>
Date: Tue, 20 Feb 2024 17:45:36 +0800
Message-ID: <CA+kNDJKVz18zm4F8fBVjDveLAdP49ZZUrBrAosq5ng1wYHaFgQ@mail.gmail.com>
Subject: Re: [PATCH V2 2/2] Documentation: filesystems: introduce
 proc/slabinfo to users
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Matthew Wilcox <willy@infradead.org>, Fangzheng Zhang <fangzheng.zhang@unisoc.com>, 
	Christoph Lameter <cl@linux.com>, Pekka Enberg <penberg@kernel.org>, David Rientjes <rientjes@google.com>, 
	Joonsoo Kim <iamjoonsoo.kim@lge.com>, Andrew Morton <akpm@linux-foundation.org>, 
	Roman Gushchin <roman.gushchin@linux.dev>, Hyeonggon Yoo <42.hyeyoo@gmail.com>, 
	Greg KH <gregkh@linuxfoundation.org>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org, tkjos@google.com, 
	Yuming Han <yuming.han@unisoc.com>, Chunyan Zhang <zhang.lyra@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 20, 2024 at 5:21=E2=80=AFPM Vlastimil Babka <vbabka@suse.cz> wr=
ote:
>
> On 2/20/24 09:49, zhang fangzheng wrote:
> > On Mon, Feb 19, 2024 at 4:09=E2=80=AFPM Vlastimil Babka <vbabka@suse.cz=
> wrote:
> >>
> >> On 2/19/24 07:23, zhang fangzheng wrote:
> >> > On Mon, Feb 19, 2024 at 12:24=E2=80=AFPM Matthew Wilcox <willy@infra=
dead.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 /pro=
c/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 cur=
rently
> >> >> 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 t=
o
> >> > 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 k=
now
> >> 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 an=
d
> >> refuse to work
> >> - confused by the new column they start throwing error, or report wron=
g
> >> 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 =E2=80=9C/proc/slabinfo=E2=80=9D is not used in related sla=
binfo debugging tools
> > (such as tools/mm/slabinfo),
>
> Hi,
>
> we are not concerned about slabinfo debugging tools that are part of kern=
el
> source tree, but about those outside, including those created privately a=
nd
> we don't even know they exist.
>
For your concerns, I think the supplementary introduction that new
output results
of slabinfo v2.2  in patch 2/2 will be necessary. This can help them
optimize their tools
more quickly to adapt to proc/slabinfo. Is this more friendly?

> > 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 outp=
ut
> > 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=E2=80=99s suggestions), it will be implemented in the n=
ext version
> >> > or even the later version.
> >>
> >> So what is your motivation for all this in the first place? You have s=
ome
> >> 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 =E2=80=98cat proc/slabinfo=E2=80=99 command. As for the descript=
ion "<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 <slabrec=
laim> has
> > the same effect as traversing "/sys/kernel/slab/$ cache/reclaim_account=
".
> >
> >> Would tools/mm/slabinfo almost work for you, but you're missing someth=
ing?
> >> 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 so=
lution
> >> planned for the future is simply not feasible.
> >>
> > Using the slabinfo tool to parse /sys/kernel/slab/$cache/reclaim_accoun=
t
> > 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!
> >>
>