The test_memcontrol selftest consistently fails its test_memcg_low
sub-test due to the fact that its 3rd test child cgroup which
have a memmory.low of 0 have low event count. This happens when
memory_recursiveprot mount option is enabled which is the default
setting used by systemd to mount cgroup2 filesystem.

Modify the test_memcontrol.c to allow non-zero low event count in this
particular case with memory_recursiveprot on.

With this patch applied, the test_memcg_low sub-test finishes
successfully without failure in most cases. Though both test_memcg_low
and test_memcg_min sub-tests may still fail occasionally if the
memory.current values fall outside of the expected ranges.

The 4th test child cgroup has no memory usage and so has an effective
low of 0. It has no low event count because the mem_cgroup_below_low()
check in shrink_node_memcgs() is skipped as mem_cgroup_below_min()
returns true. If we ever change mem_cgroup_below_min() in such a way
that it no longer skips the no usage case, we will have to add code to
explicitly skip it.

Suggested-by: Michal Koutný <mkoutny@suse.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 tools/testing/selftests/cgroup/test_memcontrol.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index 16f5d74ae762..5a5dcbe57b56 100644
--- 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]
+ * A/B/F  memory.current  = 0   [memory.events:low == 0]
  * (for origin of the numbers, see model in memcg_protection.m.)
  *
  * After that it tries to allocate more than there is
@@ -525,8 +525,14 @@ static int test_memcg_protection(const char *root, bool min)
 		goto cleanup;
 	}
 
+	/*
+	 * Child 2 has memory.low=0, but some low protection is still being
+	 * distributed down from its parent with memory.low=50M if cgroup2
+	 * memory_recursiveprot mount option is enabled. So the low event
+	 * count will be non-zero in this case.
+	 */
 	for (i = 0; i < ARRAY_SIZE(children); i++) {
-		int no_low_events_index = 1;
+		int no_low_events_index = has_recursiveprot ? 2 : 1;
 		long low, oom;
 
 		oom = cg_read_key_long(children[i], "memory.events", "oom ");
-- 
2.49.0

On Tue, Apr 15, 2025 at 05:04:14PM -0400, Waiman Long <longman@redhat.com> wrote:
> +	/*
> +	 * Child 2 has memory.low=0, but some low protection is still being
> +	 * distributed down from its parent with memory.low=50M if cgroup2
> +	 * memory_recursiveprot mount option is enabled. So the low event
> +	 * count will be non-zero in this case.

I say: Child 2 should have zero effective low value in this test case.
Johannes says (IIUC): One cannot argue whether there is or isn't
effective low for Child 2, it depends on siblings.
(I also say that low events should only be counted for nominal low
breaches but that's not so important here.)

But together this means no value of memory.events:low is valid or
invalid in this testcase. Hence I suggested ignoring Child 2's value in
checks.

> +	 */
>  	for (i = 0; i < ARRAY_SIZE(children); i++) {
> -		int no_low_events_index = 1;
> +		int no_low_events_index = has_recursiveprot ? 2 : 1;
>  		long low, oom;
>  
>  		oom = cg_read_key_long(children[i], "memory.events", "oom ");

But this is not what I Suggested-by: [1]

Michal

[1] https://lore.kernel.org/r/awgbdn6gwnj4kfaezsorvopgsdyoty3yahdeanqvoxstz2w2ke@xc3sv43elkz5

On 4/16/25 5:25 AM, Michal Koutný wrote:
> On Tue, Apr 15, 2025 at 05:04:14PM -0400, Waiman Long <longman@redhat.com> wrote:
>> +	/*
>> +	 * Child 2 has memory.low=0, but some low protection is still being
>> +	 * distributed down from its parent with memory.low=50M if cgroup2
>> +	 * memory_recursiveprot mount option is enabled. So the low event
>> +	 * count will be non-zero in this case.
> I say: Child 2 should have zero effective low value in this test case.
> Johannes says (IIUC): One cannot argue whether there is or isn't
> effective low for Child 2, it depends on siblings.
> (I also say that low events should only be counted for nominal low
> breaches but that's not so important here.)
>
> But together this means no value of memory.events:low is valid or
> invalid in this testcase. Hence I suggested ignoring Child 2's value in
> checks.
I understand your point of view. What I want to do is to document the 
expected behavior and I don't see any example of ignoring a metric for a 
particular child in the test. In this particular test, I did see an elow 
of 17 for child 2.
>
>> +	 */
>>   	for (i = 0; i < ARRAY_SIZE(children); i++) {
>> -		int no_low_events_index = 1;
>> +		int no_low_events_index = has_recursiveprot ? 2 : 1;
>>   		long low, oom;
>>   
>>   		oom = cg_read_key_long(children[i], "memory.events", "oom ");
> But this is not what I Suggested-by: [1]

I was referring to the suggestion that the setting of 
memory_recursiveprot mount option has a material impact of the child 2 
test result. Roman probably didn't have memory_recursiveprot set when 
developing this selftest.

I can take out the Suggested-by tag.

Cheers,
Longman


>
> Michal
>
> [1] https://lore.kernel.org/r/awgbdn6gwnj4kfaezsorvopgsdyoty3yahdeanqvoxstz2w2ke@xc3sv43elkz5

On Sun, Apr 20, 2025 at 05:48:15PM -0400, Waiman Long <llong@redhat.com> wrote:
> I was referring to the suggestion that the setting of memory_recursiveprot
> mount option has a material impact of the child 2 test result. Roman
> probably didn't have memory_recursiveprot set when developing this selftest.

The patch in its v7 form is effectively a revert of
	1d09069f5313f ("selftests: memcg: expect no low events in unprotected sibling")

i.e. this would be going in circles (that commit is also a revert) hence
I suggested to exempt looking at memory.events:low entirely with
memory_recursiveprot (and check for 0 when !memory_recursiveprot) --
which is something new (and hopefully universally better :-)

Michal