Received-SPF: pass (google.com: domain of linux-kernel+bounces-11594-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: <20231211154644.4103495-1-pasha.tatashin@soleen.com>
 <3d415ab4-e8c7-7e72-0379-952370612bdd@google.com> <CA+CK2bA2vZp3e+HHfB-sdLsPUYghMxvKcWURktDtNjwPL79Csw@mail.gmail.com>
 <b1049bfa-68c4-e237-30a9-1514a378c7f1@google.com>
In-Reply-To: <b1049bfa-68c4-e237-30a9-1514a378c7f1@google.com>
From: Pasha Tatashin <pasha.tatashin@soleen.com>
Date: Tue, 26 Dec 2023 12:26:27 -0500
Message-ID: <CA+CK2bBxbvO-osm5XKk4VkaXYgfZXkDAtfayaYJ-vXo=QFqGPA@mail.gmail.com>
Subject: Re: [PATCH] vmstat: don't auto expand the sysfs files
To: David Rientjes <rientjes@google.com>
Cc: gregkh@linuxfoundation.org, rafael@kernel.org, akpm@linux-foundation.org, 
	surenb@google.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, 
	souravpanda@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Dec 24, 2023 at 4:26=E2=80=AFPM David Rientjes <rientjes@google.com=
> wrote:
>
> On Thu, 14 Dec 2023, Pasha Tatashin wrote:
>
> > > > Whenever a new fields are added one of the following: node_stat_ite=
m
> > > > numa_stat_item zone_stat_item, the /sys/devices/system/node/nodeX/v=
mstat
> > > > files are auto expanded.
> > > >
> > > > This is a problem, as sysfs files should be only one value per file=
.
> > >
> > > Does this patch address the one-value-per-file issue?  (I think that =
ship
> > > has sailed for vmstat.)
> >
> > That ship has sailed for vmstat, this patch addresses what was asked
> > by GregKH: not to add new values to vmstat, as not to make the
> > existing problem even worse. The sysfs file system has a one page
> > limit per file. The developers will decide how to export the new items
> > added to node_stat, numa_stat, zone_stat individually. Each new item
> > can be exported in its own files, and must have its own documentation
> > about interface stability, value meaning, and expectations when the
> > stat file is absent.
> >
>
> As of at least 6.5, /proc/vmstat is a strict superset of the per-node
> vmstat.  Why is that a problem?

The intent of this series is to stop auto expanding
/sys/devices/system/node/nodeX/vmstat as sysfs should only be one
value per file, and the task is not to make things worse. /proc/vmstat
is mostly ok, however we might not need to auto expand it as well, to
avoid situations where removing a field becomes a problem, and we have
to keep it in the file forever, like what we do with nr_unstable.

> There's great benefit to being able to use the sample implementations to
> parse either /proc/vmstat *or* the per-node vmstat and without needing to
> read the per-node vmstat plus some new set of sysfs files that are
> one-value-per-file.  The per-node vmstat will always be multiple values,
> in fact it's a key value pair.

Yes, but that file is already large, and soon can overflow a page
size, instead of converting it to a binary format, let's add new items
as one item per-file.

> I have to think that doing anything else for vmstat is just adding
> complexity (like this patch) and actually making it *harder* on userspace
> to read the data it needs.
>
> Yes, the per-node vmstat likely shouldn't be in sysfs at all but it
> appears to have been added there 13+ years ago because it was a convenien=
t
> place to add a per-node variant.  That's not ideal, but owell.

It is up-to GregKH who requested this change. Greg, specifically
requested not to add new fields into per-node vmstat, and we are
adding new fields with per-page metadata series, and IOMMU accounting
series as these files are auto-expanded without this series.

Pasha