On 4/14/25 8:42 AM, Michal Koutný wrote:
On Sun, Apr 13, 2025 at 10:12:48PM -0400, Waiman Long longman@redhat.com wrote:
- memory.low is set to a non-zero value but the cgroup has no task in it so that it has an effective low value of 0. Again it may have a non-zero low event count if memory reclaim happens. This is probably not a result expected by the users and it is really doubtful that users will check an empty cgroup with no task in it and expecting some non-zero event counts.
I think you want to distinguish "no tasks" vs "no usage" in this paragraph.
Good point. Will update it if I need to send a new version.
--- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -5963,6 +5963,10 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) mem_cgroup_calculate_protection(target_memcg, memcg);
/* Skip memcg with no usage */if (!mem_cgroup_usage(memcg, false))continue;- if (mem_cgroup_below_min(target_memcg, memcg)) {
As I think more about this -- the idea expressed by the diff makes sense. But is it really a change? For non-root memcgs, they'll be skipped because 0 >= 0 (in mem_cgroup_below_min()) and root memcg would hardly be skipped.
I did see some low event in the no usage case because of the ">=" comparison used in mem_cgroup_below_min(). I originally planning to guard against the elow == 0 case but Johannes advised against it.
--- a/tools/testing/selftests/cgroup/test_memcontrol.c +++ b/tools/testing/selftests/cgroup/test_memcontrol.c @@ -380,10 +380,10 @@ static bool reclaim_until(const char *memcg, long goal);
- Then it checks actual memory usages and expects that:
- A/B memory.current ~= 50M
- A/B/C memory.current ~= 29M
- A/B/D memory.current ~= 21M
- A/B/E memory.current ~= 0
- A/B/F memory.current = 0
- A/B/C memory.current ~= 29M [memory.events:low > 0]
- A/B/D memory.current ~= 21M [memory.events:low > 0]
- A/B/E memory.current ~= 0 [memory.events:low == 0 if !memory_recursiveprot, > 0 otherwise]
Please note the subtlety in my suggestion -- I want the test with memory_recursiveprot _not_ to check events count at all. Because: a) it forces single interpretation of low events wrt effective low limit b) effective low limit should still be 0 in E in this testcase (there should be no unclaimed protection of C and D).
Yes, low event count for E is 0 in the !memory_recursiveprot case, but C/D still have low events and setting no_low_events_index to -1 will fail the test and it is not the same as not checking low event counts at all.
Cheers, Longman