improvements about lowmem_reserve and /proc/zoneinfo

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.

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 <[email protected]>

---
 mm/vmstat.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

mm/vmstat.c +11 -7

index 6fd1407f4632..4bbf9be786da 100644
@@ -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;
 }

Signature · 1 line

2.17.2

Quoted 5 lines · Baoquan He

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.

Quoted 18 lines · David Rientjes

Quoted 5 lines · Baoquan He

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.

Quoted 3 lines

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.

Quoted 19 lines · Baoquan He

Quoted 18 lines · David Rientjes

Quoted 5 lines · Baoquan He

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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 <[email protected]>
Date:   Thu Jul 28 15:47:02 2016 -0700

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

Quoted 13 lines · Baoquan He

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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 <[email protected]>
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.

Quoted 31 lines · David Rientjes

Quoted 13 lines · Baoquan He

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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 <[email protected]>
Date:   Wed May 3 14:52:59 2017 -0700

Quoted 7 lines

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.

OK, I see why it is in such a situation, the empty zones were not printed.

I could still not get how vm.lowmem_reserve_ratio is set with /proc/zoneinfo in the old initscripts, do you see any risk if not filling and showing the ->lowmem_reserve[] of empty zone in patch 2 and 3? Thanks in advance.

Quoted 43 lines · Baoquan He

Quoted 31 lines · David Rientjes

Quoted 13 lines · Baoquan He

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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 <[email protected]>
Date:   Wed May 3 14:52:59 2017 -0700

Quoted 7 lines

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.

OK, I see why it is in such a situation, the empty zones were not printed.

Quoted 4 lines

I could still not get how vm.lowmem_reserve_ratio is set with /proc/zoneinfo in the old initscripts, do you see any risk if not filling and showing the ->lowmem_reserve[] of empty zone in patch 2 and 3? Thanks in advance.

The point is why should we even care. Displaying that information shouldn't hurt anything, right?