On Tue, 26 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.
>
Thanks, let's get clarification on this once and for all from Andrew and
Greg.
I'd argue that the ship on the "sysfs one-value-per-file rule" has sailed
for long-standing use cases where either (1) switching is just not
possible or (2) switching would be an undue burden to the user.
An example of (1) would be THP enablement and defrag options:
$ grep . /sys/kernel/mm/transparent_hugepage/{defrag,enabled,shmem_enabled}
/sys/kernel/mm/transparent_hugepage/defrag:always defer defer+madvise [madvise] never
/sys/kernel/mm/transparent_hugepage/enabled:[always] madvise never
/sys/kernel/mm/transparent_hugepage/shmem_enabled:always within_size advise [never] deny force
This convention isn't going to change. We're not going to suddenly add a
new enablement or defrag option that can only be set in a newly added
file that is one-value-per-file.
THP was obviously introduced before any sysfs "one-value-per-file rule"
and, in retrospect, I think most people would agree that these files would
be much better off implemented returning a single word. But, choices
where made in the "before times", and it was implemented in a way that
shows all the possible choices and which one is in effect. Owell. We
deal with it, and we move on.
Imagine if I add a new choice of "lightweight" to the "defrag" file. The
only rational way to do that would be to extend the current interface, not
create a new /sys/kernel/mm/transparent_hugepage/defrag/lightweight file
that is one-value-per-file that unsets all the other options in "defrag."
Please.
I use this an an extreme example, but a very real one: we can't break
userspace and that will always supercede the desire to maintain
one-value-per-file.
Don't get me wrong, I am all *for* one-value-per-file, it has many
benefits. But I disagree we should use it a a northstar for *existing*
use cases if it actually means we'll be breaking userspace or making it
much harder on userspace.
Under discussion in this thread would be an example of (2). /proc/vmstat
is a strict superset of its per-node
/sys/devices/system/node/node*/vmstat counterparts.
For ZVCs, we *want* to ensure new stats are added to both. For the same
reason that we want to ensure old stats are removed from both. Starting
to maintain completely different implementations in the kernel comes with
complexity, but also with risk of bugs. Especially for something that
works exceedigly well for the user today.
Saying that we can extend /proc/vmstat for more entries but then *not*
extend /sys/devices/system/node/node*/vmstat for the exact same entries,
and forcing those a new files with a single value, is doing a disservice
to the user who wants to collect the data comprehensively with the same
implementation for the entire system and each node.
The *only* time I think that makes sense is if all new vmstats must be
added in their own files in both procfs (system-wide) and sysfs
(per-node). That very well may be the path forward, but get ready for an
explosion in new top-level /proc files. (It'll also be a shame for
anybody who just dumps /proc/meminfo and /proc/vmstat in testing for
capturing the state of the system.)
The discussion about needing to make vmstat be a binary file because it
doesn't fit into a single page is not very realistic. It will result in
userspace breakage for everything parsing it as it is today, so let's just
set that aside. If a sysfs file needs to support returning more than
PAGE_SIZE of contents, which is just an implementation detail, so be it.
For brand new interfaces, I'm all for it: add one-value-per-file and be
done with it.
But for existing files and conventions, I think we should settle it as
"keep doing what you've been doing for 13+ years" and don't force this
argument every time a kernel developer wants to just add one more stat.
Over to Andrew and Greg.
