On 03/25/20 at 09:55am, Michal Hocko wrote:
> On Wed 25-03-20 13:53:31, Baoquan He wrote:
> > On 03/24/20 at 12:25pm, David Rientjes wrote:
> > > On Tue, 24 Mar 2020, Baoquan He wrote:
> > >
> > > > This moving makes the layout of /proc/zoneinfo more sensible. And there
> > > > are 4 zones at most currently, it doesn't need to scroll down much to get
> > > > to the 1st populated zone, even though the 1st populated zone is MOVABLE
> > > > zone.
> > > >
> > >
> > > Doesn't this introduce risk that it will break existing parsers of
> > > /proc/zoneinfo in subtle ways?
> > >
> > > In some cases /proc/zoneinfo is a tricky file to correctly parse because
> > > you have to rely on the existing order in which it is printed to determine
> > > which zone is being described. We need to print zones even with unmanaged
> > > pages, for instance, otherwise userspace may be unaware of which zones are
> > > supported and what order they are in. That's important to be able to
> > > construct the proper string to use when writing vm.lowmem_reserve_ratio.
> > >
> > > I'd prefer not changing the order of /proc/zoneinfo if it can be avoided
> > > just because the risk outweighs the reward that we may break some
> > > initscript parsers.
> >
> > Oh, I may not describe the change and result clearly. This patch doesn't
> > change zone order at all. I only move the per-node stats to the front of
> > each node, the zone order is completely kept the same, still DMA, DMA32,
> > NORMAL, MOVABLE.
>
> Even this can break existing parsers. Fixing that up is likely not hard
> and existing parsers would be mostly debugging hacks here and there but
> I do miss any actual justification except for you considering it more
> sensible. I do not remember this would be a common pain point for people
> parsing this file. If anything the overal structure of the file makes it
> hard to parse and your patches do not really address that. We are likely
> too late to make the output much more sensible TBH.
>
> That being said, I haven't looked more closely on your patches because I
> do not have spare cycles for that. Your justification for touching the
> code which seems to be working relatively well is quite weak IMHO, yet
> it adds a non zero risk for breaking existing parsers.
I would take the saying of non zero risk for breaking existing parsers.
When considering this change, I thought about the possible risk. However,
found out the per-node stats was added in 2016 which is not so late, and
assume nobody will rely on the order of per-node stats embeded into a
zone. But I have to admit any concern or worry of risk is worth being
considerred carefully since /proc/zoneinfo is a classic interface.
So, in view of objections from you and David, I would like to drop this
patch and patch 5. It's a small improvement, not worth taking any risk.
But if it goes back to this time of 2017, I would like to spend some
time to defend it :-)
commit e2ecc8a79ed49f7838b4fdf352c4c48cec9424ac
Author: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
Date: Thu Jul 28 15:47:02 2016 -0700
mm, vmstat: print node-based stats in zoneinfo file
