linux-kernel - Re: [PATCH] vmstat: don't auto expand the sysfs files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+CK2bBxbvO-osm5XKk4VkaXYgfZXkDAtfayaYJ-vXo=QFqGPA@mail.gmail.com>
Date: Tue, 26 Dec 2023 12:26:27 -0500
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: David Rientjes <rientjes@...gle.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, akpm@...ux-foundation.org, 
	surenb@...gle.com, linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	souravpanda@...gle.com
Subject: Re: [PATCH] vmstat: don't auto expand the sysfs files

On Sun, Dec 24, 2023 at 4:26 PM David Rientjes <rientjes@...gle.com> wrote:
>
> On Thu, 14 Dec 2023, Pasha Tatashin wrote:
>
> > > > Whenever a new fields are added one of the following: node_stat_item
> > > > numa_stat_item zone_stat_item, the /sys/devices/system/node/nodeX/vmstat
> > > > 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 convenient
> 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