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:   Tue, 24 Mar 2020 12:25:03 -0700 (PDT)
From:   David Rientjes <rientjes@google.com>
To:     Baoquan He <bhe@redhat.com>
cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, iamjoonsoo.kim@lge.com,
        hannes@cmpxchg.org, mhocko@kernel.org, vbabka@suse.cz
Subject: Re: [PATCH 4/5] mm/vmstat.c: move the per-node stats to the front
 of /proc/zoneinfo
In-Reply-To: <20200324142229.12028-5-bhe@redhat.com>
Message-ID: <alpine.DEB.2.21.2003241220360.34058@chino.kir.corp.google.com>
References: <20200324142229.12028-1-bhe@redhat.com> <20200324142229.12028-5-bhe@redhat.com>
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 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.

> Node 2, per-node stats
>       nr_inactive_anon 48
>       nr_active_anon 15454
> ...
>       nr_foll_pin_acquired 0
>       nr_foll_pin_released 0
> Node 2, zone      DMA
>   pages free     0
>         min      0
>         low      0
>         high     0
>         spanned  0
>         present  0
>         managed  0
> Node 2, zone    DMA32
>   pages free     0
>         min      0
>         low      0
>         high     0
>         spanned  0
>         present  0
>         managed  0
> Node 2, zone   Normal
>   pages free     0
>         min      0
>         low      0
>         high     0
>         spanned  0
>         present  0
>         managed  0
> Node 2, zone  Movable
>   pages free     196346
>         min      3540
> ...
>         managed  262144
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/vmstat.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 6fd1407f4632..4bbf9be786da 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1567,13 +1567,6 @@ static void zoneinfo_show_print(struct seq_file *m, pg_data_t *pgdat,
>  {
>  	int i;
>  	seq_printf(m, "Node %d, zone %8s", pgdat->node_id, zone->name);
> -	if (is_zone_first_populated(pgdat, zone)) {
> -		seq_printf(m, "\n  per-node stats");
> -		for (i = 0; i < NR_VM_NODE_STAT_ITEMS; i++) {
> -			seq_printf(m, "\n      %-12s %lu", node_stat_name(i),
> -				   node_page_state(pgdat, i));
> -		}
> -	}
>  	seq_printf(m,
>  		   "\n  pages free     %lu"
>  		   "\n        min      %lu"
> @@ -1648,7 +1641,18 @@ static void zoneinfo_show_print(struct seq_file *m, pg_data_t *pgdat,
>   */
>  static int zoneinfo_show(struct seq_file *m, void *arg)
>  {
> +	int i;
>  	pg_data_t *pgdat = (pg_data_t *)arg;
> +
> +	if (node_state(pgdat->node_id, N_MEMORY)) {
> +		seq_printf(m, "Node %d, per-node stats", pgdat->node_id);
> +		for (i = 0; i < NR_VM_NODE_STAT_ITEMS; i++) {
> +			seq_printf(m, "\n      %-12s %lu", node_stat_name(i),
> +				   node_page_state(pgdat, i));
> +		}
> +		seq_putc(m, '\n');
> +	}
> +
>  	walk_zones_in_node(m, pgdat, false, false, zoneinfo_show_print);
>  	return 0;
>  }
> -- 
> 2.17.2
> 
> 
>