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
Date: Wed, 25 Mar 2020 12:45:18 -0700 (PDT) [thread overview]
Message-ID: <alpine.DEB.2.21.2003251242060.51844@chino.kir.corp.google.com> (raw)
In-Reply-To: <20200325142315.GC9942@MiWiFi-R3L-srv>
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
>
>
>
next prev parent reply other threads:[~2020-03-25 19:45 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-24 14:22 [PATCH 0/5] improvements about lowmem_reserve and /proc/zoneinfo Baoquan He
2020-03-24 14:22 ` [PATCH 1/5] mm/page_alloc.c: only tune sysctl_lowmem_reserve_ratio value once when changing it Baoquan He
2020-03-24 14:22 ` [PATCH 2/5] mm/page_alloc.c: clear out zone->lowmem_reserve[] if the zone is empty Baoquan He
2020-03-24 14:22 ` [PATCH 3/5] mm/vmstat.c: do not show lowmem reserve protection information of empty zone Baoquan He
2020-03-24 14:22 ` [PATCH 4/5] mm/vmstat.c: move the per-node stats to the front of /proc/zoneinfo Baoquan He
2020-03-24 19:25 ` David Rientjes
2020-03-25 5:53 ` Baoquan He
2020-03-25 8:55 ` Michal Hocko
2020-03-25 14:23 ` Baoquan He
2020-03-25 19:45 ` David Rientjes [this message]
2020-03-26 4:24 ` Baoquan He
2020-03-26 6:43 ` Michal Hocko
2020-03-26 11:22 ` Baoquan He
2020-03-24 14:22 ` [PATCH 5/5] mm/vmstat.c: remove the useless code Baoquan He
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.21.2003251242060.51844@chino.kir.corp.google.com \
--to=rientjes@google.com \
--cc=akpm@linux-foundation.org \
--cc=bhe@redhat.com \
--cc=hannes@cmpxchg.org \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@techsingularity.net \
--cc=mhocko@kernel.org \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox