Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 25 Mar 2020 12:45:18 -0700 (PDT)
From:   David Rientjes <rientjes@google.com>
To:     Baoquan He <bhe@redhat.com>
cc:     Michal Hocko <mhocko@kernel.org>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, akpm@linux-foundation.org,
        iamjoonsoo.kim@lge.com, hannes@cmpxchg.org, vbabka@suse.cz,
        mgorman@techsingularity.net
Subject: Re: [PATCH 4/5] mm/vmstat.c: move the per-node stats to the front
 of /proc/zoneinfo
In-Reply-To: <20200325142315.GC9942@MiWiFi-R3L-srv>
Message-ID: <alpine.DEB.2.21.2003251242060.51844@chino.kir.corp.google.com>
References: <20200324142229.12028-1-bhe@redhat.com> <20200324142229.12028-5-bhe@redhat.com> <alpine.DEB.2.21.2003241220360.34058@chino.kir.corp.google.com> <20200325055331.GB9942@MiWiFi-R3L-srv> <20200325085537.GZ19542@dhcp22.suse.cz>
 <20200325142315.GC9942@MiWiFi-R3L-srv>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 25 Mar 2020, Baoquan He wrote:

> > 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.
> 

For context, we started parsing /proc/zoneinfo in initscripts to be able 
to determine the order in which vm.lowmem_reserve_ratio needs to be set 
and this required my kernel change from 2017:

commit b2bd8598195f1b2a72130592125ac6b4218988a2
Author: David Rientjes <rientjes@google.com>
Date:   Wed May 3 14:52:59 2017 -0700

    mm, vmstat: print non-populated zones in zoneinfo

Otherwise, we found, it's much more difficult to determine how this array 
should be structured.  So at least we parse this file very carefully, I'm 
not sure how much others do, but it seems like an unnecessary risk for 
little reward.  I'm happy to see it has been decided to drop this patch 
and patch 5.

> 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@techsingularity.net>
> Date:   Thu Jul 28 15:47:02 2016 -0700
> 
>     mm, vmstat: print node-based stats in zoneinfo file
> 
> 
>