Date:   Wed, 25 Mar 2020 22:23:15 +0800
From:   Baoquan He <bhe@...hat.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     David Rientjes <rientjes@...gle.com>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, akpm@...ux-foundation.org,
        iamjoonsoo.kim@....com, hannes@...xchg.org, vbabka@...e.cz,
        mgorman@...hsingularity.net
Subject: Re: [PATCH 4/5] mm/vmstat.c: move the per-node stats to the front of
 /proc/zoneinfo

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@...hsingularity.net>
Date:   Thu Jul 28 15:47:02 2016 -0700

    mm, vmstat: print node-based stats in zoneinfo file

Powered by blists - more mailing lists