memcg: add memory.vmscan_stat

[PATCH] add memory.vmscan_stat

commit log of commit 0ae5e89 " memcg: count the soft_limit reclaim in..." says it adds scanning stats to memory.stat file. But it doesn't because we considered we needed to make a concensus for such new APIs.

This patch is a trial to add memory.scan_stat. This shows
- the number of scanned pages(total, anon, file)
- the number of rotated pages(total, anon, file)
- the number of freed pages(total, anon, file)
- the number of elaplsed time (including sleep/pause time)

for both of direct/soft reclaim.

The biggest difference with oringinal Ying's one is that this file can be reset by some write, as

  # echo 0 ...../memory.scan_stat

Example of output is here. This is a result after make -j 6 kernel under 300M limit.

[kamezawa@bluextal ~]$ cat /cgroup/memory/A/memory.scan_stat [kamezawa@bluextal ~]$ cat /cgroup/memory/A/memory.vmscan_stat scanned_pages_by_limit 9471864 scanned_anon_pages_by_limit 6640629 scanned_file_pages_by_limit 2831235 rotated_pages_by_limit 4243974 rotated_anon_pages_by_limit 3971968 rotated_file_pages_by_limit 272006 freed_pages_by_limit 2318492 freed_anon_pages_by_limit 962052 freed_file_pages_by_limit 1356440 elapsed_ns_by_limit 351386416101 scanned_pages_by_system 0 scanned_anon_pages_by_system 0 scanned_file_pages_by_system 0 rotated_pages_by_system 0 rotated_anon_pages_by_system 0 rotated_file_pages_by_system 0
freed_pages_by_system 0
freed_anon_pages_by_system 0 freed_file_pages_by_system 0
elapsed_ns_by_system 0 scanned_pages_by_limit_under_hierarchy 9471864 scanned_anon_pages_by_limit_under_hierarchy 6640629 scanned_file_pages_by_limit_under_hierarchy 2831235 rotated_pages_by_limit_under_hierarchy 4243974 rotated_anon_pages_by_limit_under_hierarchy 3971968 rotated_file_pages_by_limit_under_hierarchy 272006 freed_pages_by_limit_under_hierarchy 2318492 freed_anon_pages_by_limit_under_hierarchy 962052 freed_file_pages_by_limit_under_hierarchy 1356440 elapsed_ns_by_limit_under_hierarchy 351386416101 scanned_pages_by_system_under_hierarchy 0 scanned_anon_pages_by_system_under_hierarchy 0 scanned_file_pages_by_system_under_hierarchy 0 rotated_pages_by_system_under_hierarchy 0 rotated_anon_pages_by_system_under_hierarchy 0 rotated_file_pages_by_system_under_hierarchy 0 freed_pages_by_system_under_hierarchy 0 freed_anon_pages_by_system_under_hierarchy 0 freed_file_pages_by_system_under_hierarchy 0 elapsed_ns_by_system_under_hierarchy 0

total_xxxx is for hierarchy management.

This will be useful for further memcg developments and need to be developped before we do some complicated rework on LRU/softlimit management.

This patch adds a new struct memcg_scanrecord into scan_control struct. sc->nr_scanned at el is not designed for exporting information. For example, nr_scanned is reset frequentrly and incremented +2 at scanning mapped pages.

For avoiding complexity, I added a new param in scan_control which is for exporting scanning score.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

Changelog:
- fixed the trigger for recording nr_freed in shrink_inactive_list()
Changelog:
- renamed as vmscan_stat
- handle file/anon
- added "rotated"
- changed names of param in vmscan_stat.

---
 Documentation/cgroups/memory.txt |   85 +++++++++++++++++++
 include/linux/memcontrol.h       |   19 ++++
 include/linux/swap.h             |    6 -
 mm/memcontrol.c                  |  172 +++++++++++++++++++++++++++++++++++++--
 mm/vmscan.c                      |   39 +++++++-
 5 files changed, 303 insertions(+), 18 deletions(-)

Index: mmotm-0710/Documentation/cgroups/memory.txt
===================================================================

Documentation/cgroups/memory.txt +25 -1

@@ -380,7 +380,7 @@ will be charged as a new owner of it.
 
 5.2 stat file
 
-memory.stat file includes following statistics
+5.2.1 memory.stat file includes following statistics
 
 # per-memory cgroup local status
 cache		- # of bytes of page cache memory.

@@ -438,6 +438,89 @@ Note:
 	 file_mapped is accounted only when the memory cgroup is owner of page
 	 cache.)
 
+5.2.2 memory.vmscan_stat
+
+memory.vmscan_stat includes statistics information for memory scanning and
+freeing, reclaiming. The statistics shows memory scanning information since
+memory cgroup creation and can be reset to 0 by writing 0 as
+
+ #echo 0 > ../memory.vmscan_stat
+
+This file contains following statistics.
+
+[param]_[file_or_anon]_pages_by_[reason]_[under_heararchy]
+[param]_elapsed_ns_by_[reason]_[under_hierarchy]
+
+For example,
+
+  scanned_file_pages_by_limit indicates the number of scanned
+  file pages at vmscan.
+
+Now, 3 parameters are supported
+
+  scanned - the number of pages scanned by vmscan
+  rotated - the number of pages activated at vmscan
+  freed   - the number of pages freed by vmscan
+

Quoted 136 lines · KAMEZAWA Hiroyuki

[PATCH] add memory.vmscan_stat

commit log of commit 0ae5e89 " memcg: count the soft_limit reclaim in..." says it adds scanning stats to memory.stat file. But it doesn't because we considered we needed to make a concensus for such new APIs.

This patch is a trial to add memory.scan_stat. This shows
- the number of scanned pages(total, anon, file)
- the number of rotated pages(total, anon, file)
- the number of freed pages(total, anon, file)
- the number of elaplsed time (including sleep/pause time)

for both of direct/soft reclaim.

The biggest difference with oringinal Ying's one is that this file can be reset by some write, as

  # echo 0 ...../memory.scan_stat

Example of output is here. This is a result after make -j 6 kernel under 300M limit.

[kamezawa@bluextal ~]$ cat /cgroup/memory/A/memory.scan_stat [kamezawa@bluextal ~]$ cat /cgroup/memory/A/memory.vmscan_stat scanned_pages_by_limit 9471864 scanned_anon_pages_by_limit 6640629 scanned_file_pages_by_limit 2831235 rotated_pages_by_limit 4243974 rotated_anon_pages_by_limit 3971968 rotated_file_pages_by_limit 272006 freed_pages_by_limit 2318492 freed_anon_pages_by_limit 962052 freed_file_pages_by_limit 1356440 elapsed_ns_by_limit 351386416101 scanned_pages_by_system 0 scanned_anon_pages_by_system 0 scanned_file_pages_by_system 0 rotated_pages_by_system 0 rotated_anon_pages_by_system 0 rotated_file_pages_by_system 0
freed_pages_by_system 0
freed_anon_pages_by_system 0 freed_file_pages_by_system 0
elapsed_ns_by_system 0 scanned_pages_by_limit_under_hierarchy 9471864 scanned_anon_pages_by_limit_under_hierarchy 6640629 scanned_file_pages_by_limit_under_hierarchy 2831235 rotated_pages_by_limit_under_hierarchy 4243974 rotated_anon_pages_by_limit_under_hierarchy 3971968 rotated_file_pages_by_limit_under_hierarchy 272006 freed_pages_by_limit_under_hierarchy 2318492 freed_anon_pages_by_limit_under_hierarchy 962052 freed_file_pages_by_limit_under_hierarchy 1356440 elapsed_ns_by_limit_under_hierarchy 351386416101 scanned_pages_by_system_under_hierarchy 0 scanned_anon_pages_by_system_under_hierarchy 0 scanned_file_pages_by_system_under_hierarchy 0 rotated_pages_by_system_under_hierarchy 0 rotated_anon_pages_by_system_under_hierarchy 0 rotated_file_pages_by_system_under_hierarchy 0 freed_pages_by_system_under_hierarchy 0 freed_anon_pages_by_system_under_hierarchy 0 freed_file_pages_by_system_under_hierarchy 0 elapsed_ns_by_system_under_hierarchy 0

total_xxxx is for hierarchy management.

This will be useful for further memcg developments and need to be developped before we do some complicated rework on LRU/softlimit management.

This patch adds a new struct memcg_scanrecord into scan_control struct. sc->nr_scanned at el is not designed for exporting information. For example, nr_scanned is reset frequentrly and incremented +2 at scanning mapped pages.

For avoiding complexity, I added a new param in scan_control which is for exporting scanning score.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

Changelog:
- fixed the trigger for recording nr_freed in shrink_inactive_list()
Changelog:
- renamed as vmscan_stat
- handle file/anon
- added "rotated"
- changed names of param in vmscan_stat.

---
 Documentation/cgroups/memory.txt |   85 +++++++++++++++++++
 include/linux/memcontrol.h       |   19 ++++
 include/linux/swap.h             |    6 -
 mm/memcontrol.c                  |  172 +++++++++++++++++++++++++++++++++++++--
 mm/vmscan.c                      |   39 +++++++-
 5 files changed, 303 insertions(+), 18 deletions(-)

Index: mmotm-0710/Documentation/cgroups/memory.txt
===================================================================

Documentation/cgroups/memory.txt +25 -1

@@ -380,7 +380,7 @@ will be charged as a new owner of it.
 
 5.2 stat file
 
-memory.stat file includes following statistics
+5.2.1 memory.stat file includes following statistics
 
 # per-memory cgroup local status
 cache		- # of bytes of page cache memory.

@@ -438,6 +438,89 @@ Note:
 	 file_mapped is accounted only when the memory cgroup is owner of page
 	 cache.)
 
+5.2.2 memory.vmscan_stat
+
+memory.vmscan_stat includes statistics information for memory scanning and
+freeing, reclaiming. The statistics shows memory scanning information since
+memory cgroup creation and can be reset to 0 by writing 0 as
+
+ #echo 0 > ../memory.vmscan_stat
+
+This file contains following statistics.
+
+[param]_[file_or_anon]_pages_by_[reason]_[under_heararchy]
+[param]_elapsed_ns_by_[reason]_[under_hierarchy]
+
+For example,
+
+  scanned_file_pages_by_limit indicates the number of scanned
+  file pages at vmscan.
+
+Now, 3 parameters are supported
+
+  scanned - the number of pages scanned by vmscan
+  rotated - the number of pages activated at vmscan
+  freed   - the number of pages freed by vmscan
+

Documentation/cgroups/memory.txt Note: · line 474

Quoted 10 lines

Documentation/cgroups/memory.txt +10 -0

+If "rotated" is high against scanned/freed, the memcg seems busy.
+
+Now, 2 reason are supported
+
+  limit - the memory cgroup's limit
+  system - global memory pressure + softlimit
+           (global memory pressure not under softlimit is not handled now)
+
+When under_hierarchy is added in the tail, the number indicates the
+total memcg scan of its children and itself.

In your implementation, statistics are only accounted to the memcg triggering the limit and the respectively scanned memcgs.

Consider the following setup:

	A
       / \
      B   C
     /
    D

If D tries to charge but hits the limit of A, then B's hierarchy counters do not reflect the reclaim activity resulting in D.

That's not consistent with how hierarchy counters usually operate, and neither with how you documented it.

On a non-technical note: as Ying Han and I were the other two people working on reclaim and statistics, it really irks me that neither of us were CCd on this. Especially on such a controversial change.

Quoted 3 lines · On Mon, 8 Aug 2011 14:43:33 +0200 Johannes Weiner <[email protected]> wrote:

On a non-technical note: as Ying Han and I were the other two people working on reclaim and statistics, it really irks me that neither of us were CCd on this. Especially on such a controversial change.

I always drop CC if no reply/review comes.

Quoted 6 lines · KAMEZAWA Hiroyuki

Quoted 3 lines · On Mon, 8 Aug 2011 14:43:33 +0200 Johannes Weiner <[email protected]> wrote:

On a non-technical note: as Ying Han and I were the other two people working on reclaim and statistics, it really irks me that neither of us were CCd on this. Especially on such a controversial change.

Quoted 1 line

I always drop CC if no reply/review comes.

There is always the possibility that a single mail in an otherwise unrelated patch series is overlooked (especially while on vacation ;). Getting CCd on revisions and -mm inclusion is a really nice reminder.

Unless there is a really good reason not to (is there ever?), could you please keep CCs?

Quoted 7 lines · On Tue, 9 Aug 2011 10:01:59 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 6 lines · KAMEZAWA Hiroyuki

Quoted 3 lines · On Mon, 8 Aug 2011 14:43:33 +0200 Johannes Weiner <[email protected]> wrote:

On a non-technical note: as Ying Han and I were the other two people working on reclaim and statistics, it really irks me that neither of us were CCd on this.  Especially on such a controversial change.

Quoted 9 lines

Quoted 1 line

I always drop CC if no reply/review comes.

There is always the possibility that a single mail in an otherwise unrelated patch series is overlooked (especially while on vacation ;). Getting CCd on revisions and -mm inclusion is a really nice reminder.

Unless there is a really good reason not to (is there ever?), could you please keep CCs?

Ok, if you want, I'll CC always.
I myself just don't like to get 3 copies of mails when I don't have much interests ;)

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 15 lines · On Mon, 8 Aug 2011 14:43:33 +0200 Johannes Weiner <[email protected]> wrote:

Review

Quoted 2 lines · KAMEZAWA Hiroyuki

(quoted diff) +2 -0

+When under_hierarchy is added in the tail, the number indicates the
+total memcg scan of its children and itself.

In your implementation, statistics are only accounted to the memcg triggering the limit and the respectively scanned memcgs.

Consider the following setup:

        A
       / \
      B   C
     /
    D

Quoted 3 lines

If D tries to charge but hits the limit of A, then B's hierarchy counters do not reflect the reclaim activity resulting in D.

yes, as I expected.

Quoted 21 lines · KAMEZAWA Hiroyuki

Quoted 18 lines · On Mon, 8 Aug 2011 14:43:33 +0200 Johannes Weiner <[email protected]> wrote:

Review

Quoted 2 lines · KAMEZAWA Hiroyuki

(quoted diff) +2 -0

+When under_hierarchy is added in the tail, the number indicates the
+total memcg scan of its children and itself.

In your implementation, statistics are only accounted to the memcg triggering the limit and the respectively scanned memcgs.

Consider the following setup:

        A
       / \
      B   C
     /
    D

If D tries to charge but hits the limit of A, then B's hierarchy counters do not reflect the reclaim activity resulting in D.

Quoted 1 line

yes, as I expected.

Andrew,

with a flawed design, the author unwilling to fix it, and two NAKs, can we please revert this before the release?

Quoted 22 lines · On Mon, 29 Aug 2011 17:51:13 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 21 lines · KAMEZAWA Hiroyuki

Quoted 18 lines · On Mon, 8 Aug 2011 14:43:33 +0200 Johannes Weiner <[email protected]> wrote:

Review

Quoted 2 lines · KAMEZAWA Hiroyuki

(quoted diff) +2 -0

+When under_hierarchy is added in the tail, the number indicates the
+total memcg scan of its children and itself.

In your implementation, statistics are only accounted to the memcg triggering the limit and the respectively scanned memcgs.

Consider the following setup:

        A
       / \
      B   C
     /
    D

If D tries to charge but hits the limit of A, then B's hierarchy counters do not reflect the reclaim activity resulting in D.

Quoted 6 lines

Quoted 1 line

yes, as I expected.

Andrew,

with a flawed design, the author unwilling to fix it, and two NAKs, can we please revert this before the release?

How about this ?

mm/memcontrol.c +1 -5

@@ -1710,11 +1711,18 @@ static void mem_cgroup_record_scanstat(s
 	spin_lock(&memcg->scanstat.lock);
 	__mem_cgroup_record_scanstat(memcg->scanstat.stats[context], rec);
 	spin_unlock(&memcg->scanstat.lock);
-
-	memcg = rec->root;
-	spin_lock(&memcg->scanstat.lock);
-	__mem_cgroup_record_scanstat(memcg->scanstat.rootstats[context], rec);
-	spin_unlock(&memcg->scanstat.lock);
+	cgroup = memcg->css.cgroup;

Quoted 50 lines · KAMEZAWA Hiroyuki

Quoted 22 lines · On Mon, 29 Aug 2011 17:51:13 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 21 lines · KAMEZAWA Hiroyuki

Quoted 18 lines · On Mon, 8 Aug 2011 14:43:33 +0200 Johannes Weiner <[email protected]> wrote:

Review

Quoted 2 lines · KAMEZAWA Hiroyuki

(quoted diff) +2 -0

+When under_hierarchy is added in the tail, the number indicates the
+total memcg scan of its children and itself.

In your implementation, statistics are only accounted to the memcg triggering the limit and the respectively scanned memcgs.

Consider the following setup:

        A
       / \
      B   C
     /
    D

If D tries to charge but hits the limit of A, then B's hierarchy counters do not reflect the reclaim activity resulting in D.

Quoted 6 lines

Quoted 1 line

yes, as I expected.

Andrew,

with a flawed design, the author unwilling to fix it, and two NAKs, can we please revert this before the release?

How about this ?

mm/memcontrol.c +7 -5

@@ -1710,11 +1711,18 @@ static void mem_cgroup_record_scanstat(s
 	spin_lock(&memcg->scanstat.lock);
 	__mem_cgroup_record_scanstat(memcg->scanstat.stats[context], rec);
 	spin_unlock(&memcg->scanstat.lock);
-
-	memcg = rec->root;
-	spin_lock(&memcg->scanstat.lock);
-	__mem_cgroup_record_scanstat(memcg->scanstat.rootstats[context], rec);
-	spin_unlock(&memcg->scanstat.lock);
+	cgroup = memcg->css.cgroup;
+	do {
+		spin_lock(&memcg->scanstat.lock);
+		__mem_cgroup_record_scanstat(
+			memcg->scanstat.hierarchy_stats[context], rec);
+		spin_unlock(&memcg->scanstat.lock);
+		if (!cgroup->parent)

mm/memcontrol.c static void mem_cgroup_record_scanstat(s · line 1724

Quoted 4 lines

mm/memcontrol.c +4 -0

+			break;
+		cgroup = cgroup->parent;
+		memcg = mem_cgroup_from_cont(cgroup);
+	} while (memcg->use_hierarchy && memcg != rec->root);

Okay, so this looks correct, but it sums up all parents after each memcg scanned, which could have a performance impact. Usually, hierarchy statistics are only summed up when a user reads them.

I don't get why this has to be done completely different from the way we usually do things, without any justification, whatsoever.

Why do you want to pass a recording structure down the reclaim stack?

Why not make it per-cpu counters that are only summed up, together with the hierarchy values, when someone is actually interested in them? With an interface like mem_cgroup_count_vm_event(), or maybe even an extension of that function?

Hmm...removing hierarchy part completely seems fine to me.

Quoted 17 lines · On Tue, 30 Aug 2011 09:04:24 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 16 lines · KAMEZAWA Hiroyuki

mm/memcontrol.c +7 -5

@@ -1710,11 +1711,18 @@ static void mem_cgroup_record_scanstat(s
 	spin_lock(&memcg->scanstat.lock);
 	__mem_cgroup_record_scanstat(memcg->scanstat.stats[context], rec);
 	spin_unlock(&memcg->scanstat.lock);
-
-	memcg = rec->root;
-	spin_lock(&memcg->scanstat.lock);
-	__mem_cgroup_record_scanstat(memcg->scanstat.rootstats[context], rec);
-	spin_unlock(&memcg->scanstat.lock);
+	cgroup = memcg->css.cgroup;
+	do {
+		spin_lock(&memcg->scanstat.lock);
+		__mem_cgroup_record_scanstat(
+			memcg->scanstat.hierarchy_stats[context], rec);
+		spin_unlock(&memcg->scanstat.lock);
+		if (!cgroup->parent)

Quoted 9 lines

mm/memcontrol.c static void mem_cgroup_record_scanstat(s · line 1724

Quoted 4 lines

mm/memcontrol.c +4 -0

+			break;
+		cgroup = cgroup->parent;
+		memcg = mem_cgroup_from_cont(cgroup);
+	} while (memcg->use_hierarchy && memcg != rec->root);

Okay, so this looks correct, but it sums up all parents after each memcg scanned, which could have a performance impact. Usually, hierarchy statistics are only summed up when a user reads them.

Hmm. But sum-at-read doesn't work.

Assume 3 cgroups in a hierarchy.

	A
       /
      B
     /
    C

C's scan contains 3 causes.
C's scan caused by limit of A.
C's scan caused by limit of B.
C's scan caused by limit of C.

If we make hierarchy sum at read, we think
B's scan_stat = B's scan_stat + C's scan_stat
But in precice, this is

B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

In orignal version.
B's scan_stat = B's scan_stat caused by B +
C's scan_stat caused by B +

After this patch,
B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

Hmm...removing hierarchy part completely seems fine to me.

Quoted 4 lines

I don't get why this has to be done completely different from the way we usually do things, without any justification, whatsoever.

Why do you want to pass a recording structure down the reclaim stack?

Just for reducing number of passed variables.

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 17 lines · On Tue, 30 Aug 2011 09:04:24 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 16 lines · KAMEZAWA Hiroyuki

mm/memcontrol.c +7 -5

@@ -1710,11 +1711,18 @@ static void mem_cgroup_record_scanstat(s
 	spin_lock(&memcg->scanstat.lock);
 	__mem_cgroup_record_scanstat(memcg->scanstat.stats[context], rec);
 	spin_unlock(&memcg->scanstat.lock);
-
-	memcg = rec->root;
-	spin_lock(&memcg->scanstat.lock);
-	__mem_cgroup_record_scanstat(memcg->scanstat.rootstats[context], rec);
-	spin_unlock(&memcg->scanstat.lock);
+	cgroup = memcg->css.cgroup;
+	do {
+		spin_lock(&memcg->scanstat.lock);
+		__mem_cgroup_record_scanstat(
+			memcg->scanstat.hierarchy_stats[context], rec);
+		spin_unlock(&memcg->scanstat.lock);
+		if (!cgroup->parent)

Quoted 9 lines

mm/memcontrol.c static void mem_cgroup_record_scanstat(s · line 1724

Quoted 4 lines

mm/memcontrol.c +4 -0

+			break;
+		cgroup = cgroup->parent;
+		memcg = mem_cgroup_from_cont(cgroup);
+	} while (memcg->use_hierarchy && memcg != rec->root);

Okay, so this looks correct, but it sums up all parents after each memcg scanned, which could have a performance impact. Usually, hierarchy statistics are only summed up when a user reads them.

Hmm. But sum-at-read doesn't work.

Assume 3 cgroups in a hierarchy.

	A
       /
      B
     /
    C

C's scan contains 3 causes.
C's scan caused by limit of A.
C's scan caused by limit of B.
C's scan caused by limit of C.

If we make hierarchy sum at read, we think
B's scan_stat = B's scan_stat + C's scan_stat
But in precice, this is

B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

In orignal version.
B's scan_stat = B's scan_stat caused by B +
C's scan_stat caused by B +

Quoted 8 lines

After this patch,
B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

Hmm...removing hierarchy part completely seems fine to me.

I see.

You want to look at A and see whether its limit was responsible for reclaim scans in any children. IMO, that is asking the question backwards. Instead, there is a cgroup under reclaim and one wants to find out the cause for that. Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics. If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

	root_mem == victim -> account to victim as _limit
	root_mem != victim -> account to victim as _hierarchical

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

Quoted 6 lines

Quoted 4 lines

I don't get why this has to be done completely different from the way we usually do things, without any justification, whatsoever.

Why do you want to pass a recording structure down the reclaim stack?

Just for reducing number of passed variables.

It's still sitting on bottom of the reclaim stack the whole time.

With my proposal, you would only need to pass the extra root_mem pointer.

Quoted 89 lines · On Tue, 30 Aug 2011 10:42:45 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 17 lines · On Tue, 30 Aug 2011 09:04:24 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 16 lines · KAMEZAWA Hiroyuki

mm/memcontrol.c +7 -5

@@ -1710,11 +1711,18 @@ static void mem_cgroup_record_scanstat(s
 	spin_lock(&memcg->scanstat.lock);
 	__mem_cgroup_record_scanstat(memcg->scanstat.stats[context], rec);
 	spin_unlock(&memcg->scanstat.lock);
-
-	memcg = rec->root;
-	spin_lock(&memcg->scanstat.lock);
-	__mem_cgroup_record_scanstat(memcg->scanstat.rootstats[context], rec);
-	spin_unlock(&memcg->scanstat.lock);
+	cgroup = memcg->css.cgroup;
+	do {
+		spin_lock(&memcg->scanstat.lock);
+		__mem_cgroup_record_scanstat(
+			memcg->scanstat.hierarchy_stats[context], rec);
+		spin_unlock(&memcg->scanstat.lock);
+		if (!cgroup->parent)

Quoted 9 lines

mm/memcontrol.c static void mem_cgroup_record_scanstat(s · line 1724

Quoted 4 lines

mm/memcontrol.c +4 -0

+			break;
+		cgroup = cgroup->parent;
+		memcg = mem_cgroup_from_cont(cgroup);
+	} while (memcg->use_hierarchy && memcg != rec->root);

Okay, so this looks correct, but it sums up all parents after each memcg scanned, which could have a performance impact. Usually, hierarchy statistics are only summed up when a user reads them.

Hmm. But sum-at-read doesn't work.

Assume 3 cgroups in a hierarchy.

	A
       /
      B
     /
    C

C's scan contains 3 causes.
C's scan caused by limit of A.
C's scan caused by limit of B.
C's scan caused by limit of C.

If we make hierarchy sum at read, we think
B's scan_stat = B's scan_stat + C's scan_stat
But in precice, this is

B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

In orignal version.
B's scan_stat = B's scan_stat caused by B +
C's scan_stat caused by B +

Quoted 8 lines

After this patch,
B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

Hmm...removing hierarchy part completely seems fine to me.

I see.

You want to look at A and see whether its limit was responsible for reclaim scans in any children. IMO, that is asking the question backwards. Instead, there is a cgroup under reclaim and one wants to find out the cause for that. Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics. If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

Quoted 5 lines

	root_mem == victim -> account to victim as _limit
	root_mem != victim -> account to victim as _hierarchical

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

hmm. I have no strong opinion.

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

I don't get why this has to be done completely different from the way we usually do things, without any justification, whatsoever.

Why do you want to pass a recording structure down the reclaim stack?

Just for reducing number of passed variables.

It's still sitting on bottom of the reclaim stack the whole time.

With my proposal, you would only need to pass the extra root_mem pointer.

I'm sorry I miss something. Do you say to add a function like

mem_cgroup_record_reclaim_stat(memcg, root_mem, anon_scan, anon_free, anon_rotate,
file_scan, file_free, elapsed_ns)

?

Quoted 12 lines · KAMEZAWA Hiroyuki

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

I don't get why this has to be done completely different from the way we usually do things, without any justification, whatsoever.

Why do you want to pass a recording structure down the reclaim stack?

Just for reducing number of passed variables.

It's still sitting on bottom of the reclaim stack the whole time.

With my proposal, you would only need to pass the extra root_mem pointer.

Quoted 6 lines

I'm sorry I miss something. Do you say to add a function like

mem_cgroup_record_reclaim_stat(memcg, root_mem, anon_scan, anon_free, anon_rotate,
file_scan, file_free, elapsed_ns)

?

Exactly, though passing it a stat item index and a delta would probably be closer to our other statistics accounting, i.e.:

	mem_cgroup_record_reclaim_stat(sc->mem_cgroup, sc->root_mem_cgroup,
				       MEM_CGROUP_SCAN_ANON, *nr_anon);

where sc->mem_cgroup is victim' and sc->root_mem_cgroup is root_mem' from hierarchical_reclaim. ->root_mem_cgroup might be confusing, though. I named it ->target_mem_cgroup in my patch set but I don't feel too strongly about that.

Even better would be to reuse enum vm_event_item and at one point merge all the accounting stuff into a single function and have one single set of events that makes sense on a global level as well as on a per-memcg level.

There is deviation and implementing similar things twice with slight variations and I don't see any justification for all that extra code that needs maintaining. Or counters that have similar names globally and on a per-memcg level but with different meanings, like the rotated counter. Globally, a rotated page (PGROTATED) is one that is moved back to the inactive list after writeback finishes. Per-memcg, the rotated counter is our internal heuristics value to balance pressure between LRUs and means either rotated on the inactive list, activated, not activated but countes as activated because of VM_EXEC etc.

I am still for reverting this patch before the release until we have this all sorted out. I feel rather strongly that these statistics are in no way ready to make them part of the ABI and export them to userspace as they are now.

Quoted 47 lines · On Tue, 30 Aug 2011 12:17:26 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 12 lines · KAMEZAWA Hiroyuki

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

I don't get why this has to be done completely different from the way we usually do things, without any justification, whatsoever.

Why do you want to pass a recording structure down the reclaim stack?

Just for reducing number of passed variables.

It's still sitting on bottom of the reclaim stack the whole time.

With my proposal, you would only need to pass the extra root_mem pointer.

Quoted 6 lines

I'm sorry I miss something. Do you say to add a function like

mem_cgroup_record_reclaim_stat(memcg, root_mem, anon_scan, anon_free, anon_rotate,
file_scan, file_free, elapsed_ns)

?

Exactly, though passing it a stat item index and a delta would probably be closer to our other statistics accounting, i.e.:

	mem_cgroup_record_reclaim_stat(sc->mem_cgroup, sc->root_mem_cgroup,
				       MEM_CGROUP_SCAN_ANON, *nr_anon);

where sc->mem_cgroup is victim' and sc->root_mem_cgroup is root_mem' from hierarchical_reclaim. ->root_mem_cgroup might be confusing, though. I named it ->target_mem_cgroup in my patch set but I don't feel too strongly about that.

Even better would be to reuse enum vm_event_item and at one point merge all the accounting stuff into a single function and have one single set of events that makes sense on a global level as well as on a per-memcg level.

There is deviation and implementing similar things twice with slight variations and I don't see any justification for all that extra code that needs maintaining. Or counters that have similar names globally and on a per-memcg level but with different meanings, like the rotated counter. Globally, a rotated page (PGROTATED) is one that is moved back to the inactive list after writeback finishes. Per-memcg, the rotated counter is our internal heuristics value to balance pressure between LRUs and means either rotated on the inactive list, activated, not activated but countes as activated because of VM_EXEC etc.

Quoted 4 lines

I am still for reverting this patch before the release until we have this all sorted out. I feel rather strongly that these statistics are in no way ready to make them part of the ABI and export them to userspace as they are now.

How about fixing interface first ? 1st version of this patch was in April and no big change since then.
I don't want to be starved more.

Quoted 6 lines · KAMEZAWA Hiroyuki

On Tue, 30 Aug 2011 12:17:26 +0200
Johannes Weiner <[email protected]> wrote:

How about fixing interface first ? 1st version of this patch was in April and no big change since then.
I don't want to be starved more.

Back then I mentioned all my concerns and alternate suggestions. Different from you, I explained and provided a reason for every single counter I wanted to add, suggested a basic pattern for how to interpret them to gain insight into memcg configurations and their behaviour. No reaction. If you want to make progress, than don't ignore concerns and arguments. If my arguments are crap, then tell me why and we can move on.

What we have now is not ready. It wasn't discussed properly, which certainly wasn't for the lack of interest in this change. I just got tired of raising the same points over and over again without answer.

The amount of time a change has been around is not an argument for it to get merged. On the other hand, the fact that it hasn't changed since April *even though* the implementation was opposed back then doesn't really speak for your way of getting this upstream, does it?

Quoted 63 lines · KAMEZAWA Hiroyuki

Quoted 60 lines · On Tue, 30 Aug 2011 10:42:45 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 27 lines

Assume 3 cgroups in a hierarchy.

	A
       /
      B
     /
    C

C's scan contains 3 causes.
C's scan caused by limit of A.
C's scan caused by limit of B.
C's scan caused by limit of C.

If we make hierarchy sum at read, we think
B's scan_stat = B's scan_stat + C's scan_stat
But in precice, this is

B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

In orignal version.
B's scan_stat = B's scan_stat caused by B +
C's scan_stat caused by B +

Quoted 8 lines

After this patch,
B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

Hmm...removing hierarchy part completely seems fine to me.

I see.

You want to look at A and see whether its limit was responsible for reclaim scans in any children. IMO, that is asking the question backwards. Instead, there is a cgroup under reclaim and one wants to find out the cause for that. Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics. If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

	root_mem == victim -> account to victim as _limit
	root_mem != victim -> account to victim as _hierarchical

Quoted 4 lines

Quoted 2 lines

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

hmm. I have no strong opinion.

I do :-)

Quoted 64 lines · On Tue, 30 Aug 2011 12:17:26 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 63 lines · KAMEZAWA Hiroyuki

Quoted 60 lines · On Tue, 30 Aug 2011 10:42:45 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 27 lines

Assume 3 cgroups in a hierarchy.

	A
       /
      B
     /
    C

C's scan contains 3 causes.
C's scan caused by limit of A.
C's scan caused by limit of B.
C's scan caused by limit of C.

If we make hierarchy sum at read, we think
B's scan_stat = B's scan_stat + C's scan_stat
But in precice, this is

B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

In orignal version.
B's scan_stat = B's scan_stat caused by B +
C's scan_stat caused by B +

Quoted 8 lines

After this patch,
B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

Hmm...removing hierarchy part completely seems fine to me.

I see.

You want to look at A and see whether its limit was responsible for reclaim scans in any children. IMO, that is asking the question backwards. Instead, there is a cgroup under reclaim and one wants to find out the cause for that. Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics. If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

	root_mem == victim -> account to victim as _limit
	root_mem != victim -> account to victim as _hierarchical

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

hmm. I have no strong opinion.

I do :-)

BTW, how to calculate C's lru scan caused by A finally ?

            A
           /
          B
         /
        C

At scanning LRU of C because of A's limit, where stats are recorded ?

If we record it in C, we lose where the memory pressure comes from.

Quoted 82 lines · KAMEZAWA Hiroyuki

Quoted 64 lines · On Tue, 30 Aug 2011 12:17:26 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 63 lines · KAMEZAWA Hiroyuki

Quoted 60 lines · On Tue, 30 Aug 2011 10:42:45 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 27 lines

Assume 3 cgroups in a hierarchy.

	A
       /
      B
     /
    C

C's scan contains 3 causes.
C's scan caused by limit of A.
C's scan caused by limit of B.
C's scan caused by limit of C.

If we make hierarchy sum at read, we think
B's scan_stat = B's scan_stat + C's scan_stat
But in precice, this is

B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

In orignal version.
B's scan_stat = B's scan_stat caused by B +
C's scan_stat caused by B +

Quoted 8 lines

After this patch,
B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

Hmm...removing hierarchy part completely seems fine to me.

I see.

You want to look at A and see whether its limit was responsible for reclaim scans in any children. IMO, that is asking the question backwards. Instead, there is a cgroup under reclaim and one wants to find out the cause for that. Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics. If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

	root_mem == victim -> account to victim as _limit
	root_mem != victim -> account to victim as _hierarchical

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

hmm. I have no strong opinion.

I do :-)

BTW, how to calculate C's lru scan caused by A finally ?

            A
           /
          B
         /
        C

Quoted 3 lines

At scanning LRU of C because of A's limit, where stats are recorded ?

If we record it in C, we lose where the memory pressure comes from.

It's recorded in C as 'scanned due to parent'.

If you want to track down where pressure comes from, you check the outer container, B. If B is scanned due to internal pressure, you know that C's external pressure comes from B. If B is scanned due to external pressure, you know that B's and C's pressure comes from A or the physical memory limit (the outermost container, so to speak).

The containers are nested. If C is scanned because of the limit in A, then this concerns B as well and B must be scanned as well as B, as C's usage is fully contained in B.

There is not really a direct connection between C and A that is irrelevant to B, so I see no need to record in C which parent was the cause of the pressure. Just that it was /a/ parent and not itself. Then you can follow the pressure up the hierarchy tree.

Answer to your original question:

C_scan_due_to_A = C_scan_external - B_scan_internal - A_scan_external

Quoted 106 lines · On Tue, 30 Aug 2011 13:32:21 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 82 lines · KAMEZAWA Hiroyuki

Quoted 64 lines · On Tue, 30 Aug 2011 12:17:26 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 63 lines · KAMEZAWA Hiroyuki

Quoted 60 lines · On Tue, 30 Aug 2011 10:42:45 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 27 lines

Assume 3 cgroups in a hierarchy.

	A
       /
      B
     /
    C

C's scan contains 3 causes.
C's scan caused by limit of A.
C's scan caused by limit of B.
C's scan caused by limit of C.

If we make hierarchy sum at read, we think
B's scan_stat = B's scan_stat + C's scan_stat
But in precice, this is

B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

In orignal version.
B's scan_stat = B's scan_stat caused by B +
C's scan_stat caused by B +

Quoted 8 lines

After this patch,
B's scan_stat = B's scan_stat caused by B +
B's scan_stat caused by A + C's scan_stat caused by C + C's scan_stat caused by B + C's scan_stat caused by A.

Hmm...removing hierarchy part completely seems fine to me.

I see.

You want to look at A and see whether its limit was responsible for reclaim scans in any children. IMO, that is asking the question backwards. Instead, there is a cgroup under reclaim and one wants to find out the cause for that. Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics. If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

	root_mem == victim -> account to victim as _limit
	root_mem != victim -> account to victim as _hierarchical

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

hmm. I have no strong opinion.

I do :-)

BTW, how to calculate C's lru scan caused by A finally ?

            A
           /
          B
         /
        C

Quoted 3 lines

At scanning LRU of C because of A's limit, where stats are recorded ?

If we record it in C, we lose where the memory pressure comes from.

It's recorded in C as 'scanned due to parent'.

If you want to track down where pressure comes from, you check the outer container, B. If B is scanned due to internal pressure, you know that C's external pressure comes from B. If B is scanned due to external pressure, you know that B's and C's pressure comes from A or the physical memory limit (the outermost container, so to speak).

The containers are nested. If C is scanned because of the limit in A, then this concerns B as well and B must be scanned as well as B, as C's usage is fully contained in B.

There is not really a direct connection between C and A that is irrelevant to B, so I see no need to record in C which parent was the cause of the pressure. Just that it was /a/ parent and not itself. Then you can follow the pressure up the hierarchy tree.

Answer to your original question:

Quoted 2 lines

C_scan_due_to_A = C_scan_external - B_scan_internal - A_scan_external

I'm confused.

If vmscan is scanning in C's LRU,

	(memcg == root) : C_scan_internal ++
	(memcg != root) : C_scan_external ++

Why A_scan_external exists ? It's 0 ?

I think we can never get numbers.

Quoted 8 lines · KAMEZAWA Hiroyuki

Quoted 5 lines · On Tue, 30 Aug 2011 13:32:21 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 4 lines · KAMEZAWA Hiroyuki

Quoted 1 line · On Tue, 30 Aug 2011 12:17:26 +0200 Johannes Weiner <[email protected]> wrote:

On Tue, Aug 30, 2011 at 05:56:09PM +0900, KAMEZAWA Hiroyuki wrote:

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Tue, 30 Aug 2011 10:42:45 +0200
Johannes Weiner <[email protected]> wrote:

I'm confused.

If vmscan is scanning in C's LRU,

	(memcg == root) : C_scan_internal ++
	(memcg != root) : C_scan_external ++

Yes.

Quoted 3 lines

Why A_scan_external exists ? It's 0 ?

I think we can never get numbers.

Kswapd/direct reclaim should probably be accounted as A_external, since A has no limit, so reclaim pressure can not be internal.

On the other hand, one could see the amount of physical memory in the machine as A's limit and account global reclaim as A_internal.

I think the former may be more natural.

That aside, all memcgs should have the same statistics, obviously. Scripts can easily deal with counters being zero. If items differ between cgroups, that would suck a lot.

Quoted 18 lines · On Wed, 31 Aug 2011 08:23:54 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 8 lines · KAMEZAWA Hiroyuki

Quoted 5 lines · On Tue, 30 Aug 2011 13:32:21 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 4 lines · KAMEZAWA Hiroyuki

Quoted 1 line · On Tue, 30 Aug 2011 12:17:26 +0200 Johannes Weiner <[email protected]> wrote:

On Tue, Aug 30, 2011 at 05:56:09PM +0900, KAMEZAWA Hiroyuki wrote:

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

On Tue, 30 Aug 2011 10:42:45 +0200
Johannes Weiner <[email protected]> wrote:

I'm confused.

If vmscan is scanning in C's LRU,

	(memcg == root) : C_scan_internal ++
	(memcg != root) : C_scan_external ++

Quoted 9 lines

Yes.

Quoted 3 lines

Why A_scan_external exists ? It's 0 ?

I think we can never get numbers.

Kswapd/direct reclaim should probably be accounted as A_external, since A has no limit, so reclaim pressure can not be internal.

hmm, ok. All memory pressure from memcg/system other than the memcg itsef is all external.

Quoted 3 lines

On the other hand, one could see the amount of physical memory in the machine as A's limit and account global reclaim as A_internal.

Quoted 5 lines

I think the former may be more natural.

That aside, all memcgs should have the same statistics, obviously. Scripts can easily deal with counters being zero. If items differ between cgroups, that would suck a lot.

So, when I improve direct-reclaim path, I need to see score in scan_internal.

How do you think about background-reclaim-per-memcg ? Should be counted into scan_internal ?

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 17 lines · On Tue, 30 Aug 2011 09:04:24 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 16 lines · KAMEZAWA Hiroyuki

mm/memcontrol.c +7 -5

@@ -1710,11 +1711,18 @@ static void mem_cgroup_record_scanstat(s
   spin_lock(&memcg->scanstat.lock);
   __mem_cgroup_record_scanstat(memcg->scanstat.stats[context], rec);
   spin_unlock(&memcg->scanstat.lock);
-
- memcg = rec->root;
- spin_lock(&memcg->scanstat.lock);
- __mem_cgroup_record_scanstat(memcg->scanstat.rootstats[context], rec);
- spin_unlock(&memcg->scanstat.lock);
+ cgroup = memcg->css.cgroup;
+ do {
+         spin_lock(&memcg->scanstat.lock);
+         __mem_cgroup_record_scanstat(
+                 memcg->scanstat.hierarchy_stats[context], rec);
+         spin_unlock(&memcg->scanstat.lock);
+         if (!cgroup->parent)

Quoted 9 lines

mm/memcontrol.c static void mem_cgroup_record_scanstat(s · line 1724

Quoted 4 lines

mm/memcontrol.c +4 -0

+                 break;
+         cgroup = cgroup->parent;
+         memcg = mem_cgroup_from_cont(cgroup);
+ } while (memcg->use_hierarchy && memcg != rec->root);

Okay, so this looks correct, but it sums up all parents after each memcg scanned, which could have a performance impact.  Usually, hierarchy statistics are only summed up when a user reads them.

Hmm. But sum-at-read doesn't work.

Assume 3 cgroups in a hierarchy.

      A
       /
      B
     /
    C

C's scan contains 3 causes.
      C's scan caused by limit of A.
      C's scan caused by limit of B.
      C's scan caused by limit of C.

If we make hierarchy sum at read, we think B's scan_stat = B's scan_stat + C's scan_stat But in precice, this is

      B's scan_stat = B's scan_stat caused by B +
                      B's scan_stat caused by A +
                      C's scan_stat caused by C +
                      C's scan_stat caused by B +
                      C's scan_stat caused by A.

In orignal version.
      B's scan_stat = B's scan_stat caused by B +
                      C's scan_stat caused by B +

Quoted 8 lines

After this patch,
      B's scan_stat = B's scan_stat caused by B +
                      B's scan_stat caused by A +
                      C's scan_stat caused by C +
                      C's scan_stat caused by B +
                      C's scan_stat caused by A.

Hmm...removing hierarchy part completely seems fine to me.

I see.

You want to look at A and see whether its limit was responsible for reclaim scans in any children.  IMO, that is asking the question backwards.  Instead, there is a cgroup under reclaim and one wants to find out the cause for that.  Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics.  If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

       root_mem == victim -> account to victim as _limit root_mem != victim -> account to victim as _hierarchical

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

      A (root)
       /
      B
     /
    C

Quoted 7 lines

Quoted 5 lines

I don't get why this has to be done completely different from the way we usually do things, without any justification, whatsoever.

Why do you want to pass a recording structure down the reclaim stack?

Just for reducing number of passed variables.

It's still sitting on bottom of the reclaim stack the whole time.

With my proposal, you would only need to pass the extra root_mem pointer.

Quoted 19 lines · Johannes Weiner

You want to look at A and see whether its limit was responsible for reclaim scans in any children. IMO, that is asking the question backwards. Instead, there is a cgroup under reclaim and one wants to find out the cause for that. Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics. If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

Quoted 5 lines

       root_mem == victim -> account to victim as _limit
       root_mem != victim -> account to victim as _hierarchical

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

This is pretty much the stats I am currently using for debugging the reclaim patches. For example:

scanned_pages_by_system 0
scanned_pages_by_system_under_hierarchy 50989

scanned_pages_by_limit 0
scanned_pages_by_limit_under_hierarchy 0

"_system" is count under global reclaim, and "_limit" is count under per-memcg reclaim.
"_under_hiearchy" is set if memcg is not the one triggering pressure.

Quoted 35 lines · Ying Han

Quoted 19 lines · Johannes Weiner

You want to look at A and see whether its limit was responsible for reclaim scans in any children.  IMO, that is asking the question backwards.  Instead, there is a cgroup under reclaim and one wants to find out the cause for that.  Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics.  If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

Quoted 5 lines

       root_mem == victim -> account to victim as _limit root_mem != victim -> account to victim as _hierarchical

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

This is pretty much the stats I am currently using for debugging the reclaim patches. For example:

scanned_pages_by_system 0
scanned_pages_by_system_under_hierarchy 50989

scanned_pages_by_limit 0
scanned_pages_by_limit_under_hierarchy 0

Quoted 3 lines

"_system" is count under global reclaim, and "_limit" is count under per-memcg reclaim.
"_under_hiearchy" is set if memcg is not the one triggering pressure.

I don't get this distinction between _system and _limit.  How is it orthogonal to _limit vs. _hierarchy, i.e. internal vs. external?

If the system scans memcgs then no limit is at fault.  It's just external pressure.

For example, what is the distinction between scanned_pages_by_system and scanned_pages_by_system_under_hierarchy?

the limit (_by_system -> global reclaim) nor not due to the limit (!_under_hierarchy -> memcg is the one triggering pressure)

Quoted 36 lines · Johannes Weiner

Quoted 35 lines · Ying Han

Quoted 19 lines · Johannes Weiner

You want to look at A and see whether its limit was responsible for reclaim scans in any children. IMO, that is asking the question backwards. Instead, there is a cgroup under reclaim and one wants to find out the cause for that. Not the other way round.

In my original proposal I suggested differentiating reclaim caused by internal pressure (due to own limit) and reclaim caused by external/hierarchical pressure (due to limits from parents).

If you want to find out why C is under reclaim, look at its reclaim statistics. If the _limit numbers are high, C's limit is the problem. If the _hierarchical numbers are high, the problem is B, A, or physical memory, so you check B for _limit and _hierarchical as well, then move on to A.

Implementing this would be as easy as passing not only the memcg to scan (victim) to the reclaim code, but also the memcg /causing/ the reclaim (root_mem):

Quoted 5 lines

       root_mem == victim -> account to victim as _limit
       root_mem != victim -> account to victim as _hierarchical

This would make things much simpler and more natural, both the code and the way of tracking down a problem, IMO.

This is pretty much the stats I am currently using for debugging the reclaim patches. For example:

scanned_pages_by_system 0
scanned_pages_by_system_under_hierarchy 50989

scanned_pages_by_limit 0
scanned_pages_by_limit_under_hierarchy 0

Quoted 6 lines

Quoted 3 lines

"_system" is count under global reclaim, and "_limit" is count under per-memcg reclaim.
"_under_hiearchy" is set if memcg is not the one triggering pressure.

I don't get this distinction between _system and _limit. How is it orthogonal to _limit vs. _hierarchy, i.e. internal vs. external?

Something like :

(quoted diff) +7 -0

+enum mem_cgroup_scan_context {
+       SCAN_BY_SYSTEM,
+       SCAN_BY_SYSTEM_UNDER_HIERARCHY,
+       SCAN_BY_LIMIT,
+       SCAN_BY_LIMIT_UNDER_HIERARCHY,
+       NR_SCAN_CONTEXT,
+};

if (global_reclaim(sc))
context = scan_by_system
else
context = scan_by_limit

if (target != mem)
context++;