tools/testing/selftests/cgroup/test_memcontrol.c contains a set of
testcases which validate expected behavior of the cgroup memory controller.
Roman Gushchin recently sent out a patchset that fixed a few issues in the
test. This patchset continues that effort by fixing a few more issues that
were causing non-deterministic failures in the suite. With this patchset,
I'm unable to reproduce any more errors after running the tests in a
continuous loop for many iterations. Before, I was able to reproduce at
least one of the errors fixed in this patchset with just one or two runs.

Changelog:
v2:
  - Fixed the comment headers in test_memcg_min() and test_memcg_low() to
    reflect the new ordering of child cgroups in those tests.
  - Fixed the comment I added in test_memcg_oom_group_leaf_events() to use /* */
    for multiline comments, as is the norm according to the kernel style guide.
  - Changed some of the conditional logic in test_memcg_oom_group_leaf_events()
    that checks for OOM event counts based on memory_localevents to be more
    intuitive.

David Vernet (5):
  cgroups: Refactor children cgroups in memcg tests
  cgroup: Account for memory_recursiveprot in test_memcg_low()
  cgroup: Account for memory_localevents in
    test_memcg_oom_group_leaf_events()
  cgroup: Removing racy check in test_memcg_sock()
  cgroup: Fix racy check in alloc_pagecache_max_30M() helper function

 tools/testing/selftests/cgroup/cgroup_util.c  | 12 +++
 tools/testing/selftests/cgroup/cgroup_util.h  |  1 +
 .../selftests/cgroup/test_memcontrol.c        | 77 ++++++++++++-------
 3 files changed, 64 insertions(+), 26 deletions(-)

-- 
2.30.2

Hello.

On Sat, Apr 23, 2022 at 08:56:15AM -0700, David Vernet <void@manifault.com> wrote:
> tools/testing/selftests/cgroup/test_memcontrol.c contains a set of
> testcases which validate expected behavior of the cgroup memory controller.
> Roman Gushchin recently sent out a patchset that fixed a few issues in the
> test.

Link here
https://lore.kernel.org/r/20220415000133.3955987-1-roman.gushchin@linux.dev/.

> This patchset continues that effort by fixing a few more issues that
> were causing non-deterministic failures in the suite.

Are the Roman's patches merged anywhere? (I ran into some issues when I
was rebasing your (David's) series on top of master.) I'd like to put
all sensible patches in one series or stack on existing branch (if
there's any).

For possible v3 of this series, I did:
  - dropped the patch that allows non-zero memory.events:low for a sibling with
    memory.low=0 when mounted with memory_recursiveprot (the case needs more
    discussion),
  - added few more cleanups, convenience for debugging,
  - fixed comments in the first patch.

Pushed here [1] before properly sending the v3 for discussion.

Thanks,
Michal

[1] https://github.com/Werkov/linux/commits/cgroup-ml/memory.low-overreclaim-var2

Hi Michal,

On Thu, May 12, 2022 at 07:04:10PM +0200, Michal Koutný wrote:
> Are the Roman's patches merged anywhere? (I ran into some issues when I
> was rebasing your (David's) series on top of master.) I'd like to put
> all sensible patches in one series or stack on existing branch (if
> there's any).

Roman's patches are present on master on the linux-mm tree. See
b7dbfd6553d..a131b1ed12c6.

> For possible v3 of this series, I did:
>   - dropped the patch that allows non-zero memory.events:low for a sibling with
>     memory.low=0 when mounted with memory_recursiveprot (the case needs more
>     discussion),

Ack, and thanks for keeping us steered in the right direction here. I don't
see this in the patch set you linked, but I agree this commit should be
reverted and the reclaim logic instead fixed.

>   - added few more cleanups, convenience for debugging,

Are you referring to the FAIL() macro you added? I would love to Ack that,
but unfortunately checkpatch.pl will probably yell at you for having a goto
in that macro, per the point about avoiding macros that affect control flow
[0].

I tried to do the same thing when sending out my patch set and had to
revert it before sending it to upstream.

Thanks,
David

[0] https://github.com/Werkov/linux/commit/a076339cc4825af2f22f58c1347a572b104b8221

On Thu, May 12, 2022 at 10:30:18AM -0700, David Vernet wrote:
> Hi Michal,
> 
> On Thu, May 12, 2022 at 07:04:10PM +0200, Michal Koutný wrote:
> > Are the Roman's patches merged anywhere? (I ran into some issues when I
> > was rebasing your (David's) series on top of master.) I'd like to put
> > all sensible patches in one series or stack on existing branch (if
> > there's any).
> 
> Roman's patches are present on master on the linux-mm tree. See
> b7dbfd6553d..a131b1ed12c6.
> 
> > For possible v3 of this series, I did:
> >   - dropped the patch that allows non-zero memory.events:low for a sibling with
> >     memory.low=0 when mounted with memory_recursiveprot (the case needs more
> >     discussion),
> 
> Ack, and thanks for keeping us steered in the right direction here. I don't
> see this in the patch set you linked, but I agree this commit should be
> reverted and the reclaim logic instead fixed.
> 
> >   - added few more cleanups, convenience for debugging,
> 
> Are you referring to the FAIL() macro you added? I would love to Ack that,
> but unfortunately checkpatch.pl will probably yell at you for having a goto
> in that macro, per the point about avoiding macros that affect control flow
> [0].
> 
> I tried to do the same thing when sending out my patch set and had to
> revert it before sending it to upstream.
> 
> Thanks,
> David
> 
> [0] https://github.com/Werkov/linux/commit/a076339cc4825af2f22f58c1347a572b104b8221

Sorry, I meant to link this:

https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl

Hello.

I'm just flushing the simple patches to make memcontrol selftests check the
events behavior we had consensus about (test_memcg_low fails). (I've dropped to
goto macros for now.)

(test_memcg_reclaim, test_memcg_swap_max fail for me now but it's present
even before the refactoring.)

The only bigger change is adjustment of the protected values to make tests
succeed with the given tolerance.

It's based on mm-stable [1] commit e240ac52f7da. AFAIC, the fixup and partial
reverts may be folded into respective commits.
Let me know if it should be (re)based on something else.

Thanks,
Michal

[1] https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/log/tools/testing/selftests/cgroup?h=mm-stable

Michal Koutný (4):
  selftests: memcg: Fix compilation
  selftests: memcg: Expect no low events in unprotected sibling
  selftests: memcg: Adjust expected reclaim values of protected cgroups
  selftests: memcg: Remove protection from top level memcg

 .../selftests/cgroup/test_memcontrol.c        | 59 +++++++++----------
 1 file changed, 29 insertions(+), 30 deletions(-)

-- 
2.35.3

This fixes mis-applied changes from commit 72b1e03aa725 ("cgroup:
account for memory_localevents in test_memcg_oom_group_leaf_events()").

Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
 .../selftests/cgroup/test_memcontrol.c        | 25 +++++++++++--------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index 6ab94317c87b..4958b42201a9 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -1241,7 +1241,16 @@ static int test_memcg_oom_group_leaf_events(const char *root)
 	if (cg_read_key_long(child, "memory.events", "oom_kill ") <= 0)
 		goto cleanup;
 
-	if (cg_read_key_long(parent, "memory.events", "oom_kill ") <= 0)
+	parent_oom_events = cg_read_key_long(
+			parent, "memory.events", "oom_kill ");
+	/*
+	 * If memory_localevents is not enabled (the default), the parent should
+	 * count OOM events in its children groups. Otherwise, it should not
+	 * have observed any events.
+	 */
+	if (has_localevents && parent_oom_events != 0)
+		goto cleanup;
+	else if (!has_localevents && parent_oom_events <= 0)
 		goto cleanup;
 
 	ret = KSFT_PASS;
@@ -1349,20 +1358,14 @@ static int test_memcg_oom_group_score_events(const char *root)
 	if (!cg_run(memcg, alloc_anon, (void *)MB(100)))
 		goto cleanup;
 
-	parent_oom_events = cg_read_key_long(
-			parent, "memory.events", "oom_kill ");
-	/*
-	 * If memory_localevents is not enabled (the default), the parent should
-	 * count OOM events in its children groups. Otherwise, it should not
-	 * have observed any events.
-	 */
-	if ((has_localevents && parent_oom_events == 0) ||
-	     parent_oom_events > 0)
-		ret = KSFT_PASS;
+	if (cg_read_key_long(memcg, "memory.events", "oom_kill ") != 3)
+		FAIL(cleanup);
 
 	if (kill(safe_pid, SIGKILL))
 		goto cleanup;
 
+	ret = KSFT_PASS;
+
 cleanup:
 	if (memcg)
 		cg_destroy(memcg);
-- 
2.35.3

On Fri, May 13, 2022 at 07:18:08PM +0200, Michal Koutny wrote:
> This fixes mis-applied changes from commit 72b1e03aa725 ("cgroup:
> account for memory_localevents in test_memcg_oom_group_leaf_events()").
> 
> Signed-off-by: Michal Koutný <mkoutny@suse.com>
> ---
>  .../selftests/cgroup/test_memcontrol.c        | 25 +++++++++++--------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 6ab94317c87b..4958b42201a9 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -1241,7 +1241,16 @@ static int test_memcg_oom_group_leaf_events(const char *root)
>  	if (cg_read_key_long(child, "memory.events", "oom_kill ") <= 0)
>  		goto cleanup;
>  
> -	if (cg_read_key_long(parent, "memory.events", "oom_kill ") <= 0)
> +	parent_oom_events = cg_read_key_long(
> +			parent, "memory.events", "oom_kill ");
> +	/*
> +	 * If memory_localevents is not enabled (the default), the parent should
> +	 * count OOM events in its children groups. Otherwise, it should not
> +	 * have observed any events.
> +	 */
> +	if (has_localevents && parent_oom_events != 0)
> +		goto cleanup;
> +	else if (!has_localevents && parent_oom_events <= 0)
>  		goto cleanup;
>  
>  	ret = KSFT_PASS;
> @@ -1349,20 +1358,14 @@ static int test_memcg_oom_group_score_events(const char *root)
>  	if (!cg_run(memcg, alloc_anon, (void *)MB(100)))
>  		goto cleanup;
>  
> -	parent_oom_events = cg_read_key_long(
> -			parent, "memory.events", "oom_kill ");
> -	/*
> -	 * If memory_localevents is not enabled (the default), the parent should
> -	 * count OOM events in its children groups. Otherwise, it should not
> -	 * have observed any events.
> -	 */
> -	if ((has_localevents && parent_oom_events == 0) ||
> -	     parent_oom_events > 0)
> -		ret = KSFT_PASS;
> +	if (cg_read_key_long(memcg, "memory.events", "oom_kill ") != 3)
> +		FAIL(cleanup);
>  
>  	if (kill(safe_pid, SIGKILL))
>  		goto cleanup;
>  
> +	ret = KSFT_PASS;
> +
>  cleanup:
>  	if (memcg)
>  		cg_destroy(memcg);
> -- 
> 2.35.3
> 

My bad.

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!

On Fri, May 13, 2022 at 11:53:26AM -0700, Roman Gushchin wrote:
> On Fri, May 13, 2022 at 07:18:08PM +0200, Michal Koutny wrote:
> > This fixes mis-applied changes from commit 72b1e03aa725 ("cgroup:
> > account for memory_localevents in test_memcg_oom_group_leaf_events()").
> > 
> > Signed-off-by: Michal Koutný <mkoutny@suse.com>
> > ---
> >  .../selftests/cgroup/test_memcontrol.c        | 25 +++++++++++--------
> >  1 file changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> > index 6ab94317c87b..4958b42201a9 100644
> > --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> > @@ -1241,7 +1241,16 @@ static int test_memcg_oom_group_leaf_events(const char *root)
> >  	if (cg_read_key_long(child, "memory.events", "oom_kill ") <= 0)
> >  		goto cleanup;
> >  
> > -	if (cg_read_key_long(parent, "memory.events", "oom_kill ") <= 0)
> > +	parent_oom_events = cg_read_key_long(
> > +			parent, "memory.events", "oom_kill ");
> > +	/*
> > +	 * If memory_localevents is not enabled (the default), the parent should
> > +	 * count OOM events in its children groups. Otherwise, it should not
> > +	 * have observed any events.
> > +	 */
> > +	if (has_localevents && parent_oom_events != 0)
> > +		goto cleanup;
> > +	else if (!has_localevents && parent_oom_events <= 0)
> >  		goto cleanup;
> >  
> >  	ret = KSFT_PASS;
> > @@ -1349,20 +1358,14 @@ static int test_memcg_oom_group_score_events(const char *root)
> >  	if (!cg_run(memcg, alloc_anon, (void *)MB(100)))
> >  		goto cleanup;
> >  
> > -	parent_oom_events = cg_read_key_long(
> > -			parent, "memory.events", "oom_kill ");
> > -	/*
> > -	 * If memory_localevents is not enabled (the default), the parent should
> > -	 * count OOM events in its children groups. Otherwise, it should not
> > -	 * have observed any events.
> > -	 */
> > -	if ((has_localevents && parent_oom_events == 0) ||
> > -	     parent_oom_events > 0)
> > -		ret = KSFT_PASS;
> > +	if (cg_read_key_long(memcg, "memory.events", "oom_kill ") != 3)
> > +		FAIL(cleanup);
> >  
> >  	if (kill(safe_pid, SIGKILL))
> >  		goto cleanup;
> >  
> > +	ret = KSFT_PASS;
> > +
> >  cleanup:
> >  	if (memcg)
> >  		cg_destroy(memcg);
> > -- 
> > 2.35.3
> > 
> 
> My bad.

Actually not, as pointed by David.
Seems like it's git fault. The original patch looks correct.

On Fri, May 13, 2022 at 07:18:08PM +0200, Michal Koutný wrote:
> This fixes mis-applied changes from commit 72b1e03aa725 ("cgroup:
> account for memory_localevents in test_memcg_oom_group_leaf_events()").
> 
> Signed-off-by: Michal Koutný <mkoutny@suse.com>
> ---
>  .../selftests/cgroup/test_memcontrol.c        | 25 +++++++++++--------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 6ab94317c87b..4958b42201a9 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -1241,7 +1241,16 @@ static int test_memcg_oom_group_leaf_events(const char *root)
>  	if (cg_read_key_long(child, "memory.events", "oom_kill ") <= 0)
>  		goto cleanup;
>  
> -	if (cg_read_key_long(parent, "memory.events", "oom_kill ") <= 0)
> +	parent_oom_events = cg_read_key_long(
> +			parent, "memory.events", "oom_kill ");
> +	/*
> +	 * If memory_localevents is not enabled (the default), the parent should
> +	 * count OOM events in its children groups. Otherwise, it should not
> +	 * have observed any events.
> +	 */
> +	if (has_localevents && parent_oom_events != 0)
> +		goto cleanup;
> +	else if (!has_localevents && parent_oom_events <= 0)
>  		goto cleanup;
>  
>  	ret = KSFT_PASS;
> @@ -1349,20 +1358,14 @@ static int test_memcg_oom_group_score_events(const char *root)
>  	if (!cg_run(memcg, alloc_anon, (void *)MB(100)))
>  		goto cleanup;
>  
> -	parent_oom_events = cg_read_key_long(
> -			parent, "memory.events", "oom_kill ");
> -	/*
> -	 * If memory_localevents is not enabled (the default), the parent should
> -	 * count OOM events in its children groups. Otherwise, it should not
> -	 * have observed any events.
> -	 */
> -	if ((has_localevents && parent_oom_events == 0) ||
> -	     parent_oom_events > 0)
> -		ret = KSFT_PASS;
> +	if (cg_read_key_long(memcg, "memory.events", "oom_kill ") != 3)
> +		FAIL(cleanup);
>  
>  	if (kill(safe_pid, SIGKILL))
>  		goto cleanup;
>  
> +	ret = KSFT_PASS;
> +
>  cleanup:
>  	if (memcg)
>  		cg_destroy(memcg);
> -- 
> 2.35.3
> 

Thanks for the fix, Michal.

Reviewed-by: David Vernet <void@manifault.com>

This is effectively a revert of commit cdc69458a5f3 ("cgroup: account
for memory_recursiveprot in test_memcg_low()"). The case test_memcg_low
will fail with memory_recursiveprot until resolved in reclaim
code.
However, this patch preserves the existing helpers and variables for
later uses.

Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
 tools/testing/selftests/cgroup/test_memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index 4958b42201a9..eba252fa64ac 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -528,7 +528,7 @@ static int test_memcg_low(const char *root)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(children); i++) {
-		int no_low_events_index = has_recursiveprot ? 2 : 1;
+		int no_low_events_index = 1;
 
 		oom = cg_read_key_long(children[i], "memory.events", "oom ");
 		low = cg_read_key_long(children[i], "memory.events", "low ");
-- 
2.35.3

On Fri, May 13, 2022 at 07:18:09PM +0200, Michal Koutny wrote:
> This is effectively a revert of commit cdc69458a5f3 ("cgroup: account
> for memory_recursiveprot in test_memcg_low()"). The case test_memcg_low
> will fail with memory_recursiveprot until resolved in reclaim
> code.

Hm, what are our plans here? Are we going to fix it soon-ish, or there
is still no agreement on how to proceed?

Hi.

On Fri, May 13, 2022 at 11:54:16AM -0700, Roman Gushchin <roman.gushchin@linux.dev> wrote:
> Hm, what are our plans here? Are we going to fix it soon-ish, or there
> is still no agreement on how to proceed?

Here are some of my ideas in random order so far and comments:

0) mask memory.events:low
-> not a real fix

1) don't do SWAP_CLUSTER_MAX roundup
-> won't solve sudden lift of protection

2) instead of SWAP_CLUSTER_MAX over-reclaim, do same under-reclaim
-> same problem as 1)

3) update children_low_usage transactionally (after reclaim round is done)
- ???

4) don't recursively distribute residual protection in the same reclaim round
- ???

5) iterate siblings from highest to lowest protection 
- not a solution

6) assign only >SWAP_CLUSTER_MAX of residuum
- need more info

I'm discouraged by possible complexity of 3) or 4), while 6) is what I'd
like to look more into.

HTH,
Michal

On Fri, May 13, 2022 at 07:18:09PM +0200, Michal Koutný wrote:
> This is effectively a revert of commit cdc69458a5f3 ("cgroup: account
> for memory_recursiveprot in test_memcg_low()"). The case test_memcg_low
> will fail with memory_recursiveprot until resolved in reclaim
> code.
> However, this patch preserves the existing helpers and variables for
> later uses.
> 
> Signed-off-by: Michal Koutný <mkoutny@suse.com>
> ---
>  tools/testing/selftests/cgroup/test_memcontrol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 4958b42201a9..eba252fa64ac 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -528,7 +528,7 @@ static int test_memcg_low(const char *root)
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(children); i++) {
> -		int no_low_events_index = has_recursiveprot ? 2 : 1;
> +		int no_low_events_index = 1;
>  
>  		oom = cg_read_key_long(children[i], "memory.events", "oom ");
>  		low = cg_read_key_long(children[i], "memory.events", "low ");
> -- 
> 2.35.3
> 

Reviewed-by: David Vernet <void@manifault.com>

The numbers are not easy to derive in a closed form (certainly mere
protections ratios do not apply), therefore use a simulation to obtain
expected numbers.

The new values make the protection tests succeed more precisely.

	% run as: octave-cli script
	%
	% Input configurations
	% -------------------
	% E parent effective protection
	% n nominal protection of siblings set at the givel level
	% c current consumption -,,-

	% example from testcase (values in GB)
	E = 50 / 1024;
	n = [75 25 0 500 ] / 1024;
	c = [50 50 50 0] / 1024;

	% Reclaim parameters
	% ------------------

	% Minimal reclaim amount (GB)
	cluster = 32*4 / 2**20;

	% Reclaim coefficient (think as 0.5^sc->priority)
	alpha = .1

	% Simulation parameters
	% ---------------------
	epsilon = 1e-7;
	timeout = 1000;

	% Simulation loop
	% ---------------------
	% Simulation assumes siblings consumed the initial amount of memory (w/out
	% reclaim) and then the reclaim starts, all memory is reclaimable, i.e. treated
	% same. It simulates only non-low reclaim and assumes all memory.min = 0.

	ch = [];
	eh = [];
	rh = [];

	for t = 1:timeout
		% low_usage
		u = min(c, n);
		siblings = sum(u);

		% effective_protection()
		protected = min(n, c);                % start with nominal
		e = protected * min(1, E / siblings); % normalize overcommit

		% recursive protection
		unclaimed = max(0, E - siblings);
		parent_overuse = sum(c) - siblings;
		if (unclaimed > 0 && parent_overuse > 0)
			overuse = max(0, c - protected);
			e += unclaimed * (overuse / parent_overuse);
		endif

		% get_scan_count()
		r = alpha * c;             % assume all memory is in a single LRU list

		% commit 1bc63fb1272b ("mm, memcg: make scan aggression always exclude protection")
		sz = max(e, c);
		r .*= (1 - (e+epsilon) ./ (sz+epsilon));

		% uncomment to debug prints
		% e, c, r

		% nothing to reclaim, reached equilibrium
		if max(r) < epsilon
			break;
		endif

		% SWAP_CLUSTER_MAX
		r = max(r, (r > epsilon) .* cluster);
		% XXX here I do parallel reclaim of all siblings
		% in reality reclaim is serialized and each sibling recalculates own residual
		c = max(c - r, 0);

		ch = [ch ; c];
		eh = [eh ; e];
		rh = [rh ; r];
	endfor

	t
	c, e

Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
 .../selftests/cgroup/test_memcontrol.c        | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index eba252fa64ac..9ffacf024bbd 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -260,9 +260,9 @@ static int cg_test_proc_killed(const char *cgroup)
  * memory pressure in it.
  *
  * A/B    memory.current ~= 50M
- * A/B/C  memory.current ~= 33M
- * A/B/D  memory.current ~= 17M
- * A/B/F  memory.current ~= 0
+ * A/B/C  memory.current ~= 29M
+ * A/B/D  memory.current ~= 21M
+ * A/B/E  memory.current ~= 0
  *
  * After that it tries to allocate more than there is
  * unprotected memory in A available, and checks
@@ -365,10 +365,10 @@ static int test_memcg_min(const char *root)
 	for (i = 0; i < ARRAY_SIZE(children); i++)
 		c[i] = cg_read_long(children[i], "memory.current");
 
-	if (!values_close(c[0], MB(33), 10))
+	if (!values_close(c[0], MB(29), 10))
 		goto cleanup;
 
-	if (!values_close(c[1], MB(17), 10))
+	if (!values_close(c[1], MB(21), 10))
 		goto cleanup;
 
 	if (c[3] != 0)
@@ -417,9 +417,9 @@ static int test_memcg_min(const char *root)
  *
  * Then it checks actual memory usages and expects that:
  * A/B    memory.current ~= 50M
- * A/B/   memory.current ~= 33M
- * A/B/D  memory.current ~= 17M
- * A/B/F  memory.current ~= 0
+ * A/B/   memory.current ~= 29M
+ * A/B/D  memory.current ~= 21M
+ * A/B/E  memory.current ~= 0
  *
  * After that it tries to allocate more than there is
  * unprotected memory in A available,
@@ -512,10 +512,10 @@ static int test_memcg_low(const char *root)
 	for (i = 0; i < ARRAY_SIZE(children); i++)
 		c[i] = cg_read_long(children[i], "memory.current");
 
-	if (!values_close(c[0], MB(33), 10))
+	if (!values_close(c[0], MB(29), 10))
 		goto cleanup;
 
-	if (!values_close(c[1], MB(17), 10))
+	if (!values_close(c[1], MB(21), 10))
 		goto cleanup;
 
 	if (c[3] != 0)
-- 
2.35.3

On Fri, May 13, 2022 at 07:18:10PM +0200, Michal Koutny wrote:
> The numbers are not easy to derive in a closed form (certainly mere
> protections ratios do not apply), therefore use a simulation to obtain
> expected numbers.
> 
> The new values make the protection tests succeed more precisely.
> 
> 	% run as: octave-cli script
> 	%
> 	% Input configurations
> 	% -------------------
> 	% E parent effective protection
> 	% n nominal protection of siblings set at the givel level
> 	% c current consumption -,,-
> 
> 	% example from testcase (values in GB)
> 	E = 50 / 1024;
> 	n = [75 25 0 500 ] / 1024;
> 	c = [50 50 50 0] / 1024;
> 
> 	% Reclaim parameters
> 	% ------------------
> 
> 	% Minimal reclaim amount (GB)
> 	cluster = 32*4 / 2**20;
> 
> 	% Reclaim coefficient (think as 0.5^sc->priority)
> 	alpha = .1
> 
> 	% Simulation parameters
> 	% ---------------------
> 	epsilon = 1e-7;
> 	timeout = 1000;
> 
> 	% Simulation loop
> 	% ---------------------
> 	% Simulation assumes siblings consumed the initial amount of memory (w/out
> 	% reclaim) and then the reclaim starts, all memory is reclaimable, i.e. treated
> 	% same. It simulates only non-low reclaim and assumes all memory.min = 0.
> 
> 	ch = [];
> 	eh = [];
> 	rh = [];
> 
> 	for t = 1:timeout
> 		% low_usage
> 		u = min(c, n);
> 		siblings = sum(u);
> 
> 		% effective_protection()
> 		protected = min(n, c);                % start with nominal
> 		e = protected * min(1, E / siblings); % normalize overcommit
> 
> 		% recursive protection
> 		unclaimed = max(0, E - siblings);
> 		parent_overuse = sum(c) - siblings;
> 		if (unclaimed > 0 && parent_overuse > 0)
> 			overuse = max(0, c - protected);
> 			e += unclaimed * (overuse / parent_overuse);
> 		endif
> 
> 		% get_scan_count()
> 		r = alpha * c;             % assume all memory is in a single LRU list
> 
> 		% commit 1bc63fb1272b ("mm, memcg: make scan aggression always exclude protection")
> 		sz = max(e, c);
> 		r .*= (1 - (e+epsilon) ./ (sz+epsilon));
> 
> 		% uncomment to debug prints
> 		% e, c, r
> 
> 		% nothing to reclaim, reached equilibrium
> 		if max(r) < epsilon
> 			break;
> 		endif
> 
> 		% SWAP_CLUSTER_MAX
> 		r = max(r, (r > epsilon) .* cluster);
> 		% XXX here I do parallel reclaim of all siblings
> 		% in reality reclaim is serialized and each sibling recalculates own residual
> 		c = max(c - r, 0);
> 
> 		ch = [ch ; c];
> 		eh = [eh ; e];
> 		rh = [rh ; r];
> 	endfor
> 
> 	t
> 	c, e

This is a cool stuff!

How about to place it into a separate file and add a comment into the code
with a reference?

Thanks!

The reclaim is triggered by memory limit in a subtree, therefore the
testcase does not need configured protection against external reclaim.

Also, correct/deduplicate respective comments

Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
 tools/testing/selftests/cgroup/test_memcontrol.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index 9ffacf024bbd..9d370aafd799 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -247,7 +247,7 @@ static int cg_test_proc_killed(const char *cgroup)
 
 /*
  * First, this test creates the following hierarchy:
- * A       memory.min = 50M,  memory.max = 200M
+ * A       memory.min = 0,    memory.max = 200M
  * A/B     memory.min = 50M,  memory.current = 50M
  * A/B/C   memory.min = 75M,  memory.current = 50M
  * A/B/D   memory.min = 25M,  memory.current = 50M
@@ -257,7 +257,7 @@ static int cg_test_proc_killed(const char *cgroup)
  * Usages are pagecache, but the test keeps a running
  * process in every leaf cgroup.
  * Then it creates A/G and creates a significant
- * memory pressure in it.
+ * memory pressure in A.
  *
  * A/B    memory.current ~= 50M
  * A/B/C  memory.current ~= 29M
@@ -335,8 +335,6 @@ static int test_memcg_min(const char *root)
 			      (void *)(long)fd);
 	}
 
-	if (cg_write(parent[0], "memory.min", "50M"))
-		goto cleanup;
 	if (cg_write(parent[1], "memory.min", "50M"))
 		goto cleanup;
 	if (cg_write(children[0], "memory.min", "75M"))
@@ -404,8 +402,8 @@ static int test_memcg_min(const char *root)
 
 /*
  * First, this test creates the following hierarchy:
- * A       memory.low = 50M,  memory.max = 200M
- * A/B     memory.low = 50M,  memory.current = 50M
+ * A       memory.low = 0,    memory.max = 200M
+ * A/B     memory.low = 50M,  memory.current = ...
  * A/B/C   memory.low = 75M,  memory.current = 50M
  * A/B/D   memory.low = 25M,  memory.current = 50M
  * A/B/E   memory.low = 0,    memory.current = 50M
@@ -490,8 +488,6 @@ static int test_memcg_low(const char *root)
 			goto cleanup;
 	}
 
-	if (cg_write(parent[0], "memory.low", "50M"))
-		goto cleanup;
 	if (cg_write(parent[1], "memory.low", "50M"))
 		goto cleanup;
 	if (cg_write(children[0], "memory.low", "75M"))
-- 
2.35.3

On Fri, May 13, 2022 at 07:18:11PM +0200, Michal Koutný wrote:
> The reclaim is triggered by memory limit in a subtree, therefore the
> testcase does not need configured protection against external reclaim.
> 
> Also, correct/deduplicate respective comments
> 
> Signed-off-by: Michal Koutný <mkoutny@suse.com>
> ---
>  tools/testing/selftests/cgroup/test_memcontrol.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 9ffacf024bbd..9d370aafd799 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -247,7 +247,7 @@ static int cg_test_proc_killed(const char *cgroup)
>  
>  /*
>   * First, this test creates the following hierarchy:
> - * A       memory.min = 50M,  memory.max = 200M
> + * A       memory.min = 0,    memory.max = 200M
>   * A/B     memory.min = 50M,  memory.current = 50M
>   * A/B/C   memory.min = 75M,  memory.current = 50M
>   * A/B/D   memory.min = 25M,  memory.current = 50M
> @@ -257,7 +257,7 @@ static int cg_test_proc_killed(const char *cgroup)
>   * Usages are pagecache, but the test keeps a running
>   * process in every leaf cgroup.
>   * Then it creates A/G and creates a significant
> - * memory pressure in it.
> + * memory pressure in A.
>   *
>   * A/B    memory.current ~= 50M
>   * A/B/C  memory.current ~= 29M
> @@ -335,8 +335,6 @@ static int test_memcg_min(const char *root)
>  			      (void *)(long)fd);
>  	}
>  
> -	if (cg_write(parent[0], "memory.min", "50M"))
> -		goto cleanup;
>  	if (cg_write(parent[1], "memory.min", "50M"))
>  		goto cleanup;
>  	if (cg_write(children[0], "memory.min", "75M"))
> @@ -404,8 +402,8 @@ static int test_memcg_min(const char *root)
>  
>  /*
>   * First, this test creates the following hierarchy:
> - * A       memory.low = 50M,  memory.max = 200M
> - * A/B     memory.low = 50M,  memory.current = 50M
> + * A       memory.low = 0,    memory.max = 200M
> + * A/B     memory.low = 50M,  memory.current = ...

Is there a reason that we would adjust this comment but not the A/B comment
from above in from test_memcg_low()? In both cases, I would just remove the
memory.current = ... part altogether, as Roman suggested.

>   * A/B/C   memory.low = 75M,  memory.current = 50M
>   * A/B/D   memory.low = 25M,  memory.current = 50M
>   * A/B/E   memory.low = 0,    memory.current = 50M
> @@ -490,8 +488,6 @@ static int test_memcg_low(const char *root)
>  			goto cleanup;
>  	}
>  
> -	if (cg_write(parent[0], "memory.low", "50M"))
> -		goto cleanup;
>  	if (cg_write(parent[1], "memory.low", "50M"))
>  		goto cleanup;
>  	if (cg_write(children[0], "memory.low", "75M"))
> -- 
> 2.35.3
> 

Looks good otherwise.

Reviewed-by: David Vernet <void@manifault.com>

On Fri, May 13, 2022 at 07:18:11PM +0200, Michal Koutny wrote:
> The reclaim is triggered by memory limit in a subtree, therefore the
> testcase does not need configured protection against external reclaim.
> 
> Also, correct/deduplicate respective comments
> 
> Signed-off-by: Michal Koutný <mkoutny@suse.com>
> ---
>  tools/testing/selftests/cgroup/test_memcontrol.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 9ffacf024bbd..9d370aafd799 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -247,7 +247,7 @@ static int cg_test_proc_killed(const char *cgroup)
>  
>  /*
>   * First, this test creates the following hierarchy:
> - * A       memory.min = 50M,  memory.max = 200M
> + * A       memory.min = 0,    memory.max = 200M
>   * A/B     memory.min = 50M,  memory.current = 50M
>   * A/B/C   memory.min = 75M,  memory.current = 50M
>   * A/B/D   memory.min = 25M,  memory.current = 50M
> @@ -257,7 +257,7 @@ static int cg_test_proc_killed(const char *cgroup)
>   * Usages are pagecache, but the test keeps a running
>   * process in every leaf cgroup.
>   * Then it creates A/G and creates a significant
> - * memory pressure in it.
> + * memory pressure in A.
>   *
>   * A/B    memory.current ~= 50M
>   * A/B/C  memory.current ~= 29M
> @@ -335,8 +335,6 @@ static int test_memcg_min(const char *root)
>  			      (void *)(long)fd);
>  	}
>  
> -	if (cg_write(parent[0], "memory.min", "50M"))
> -		goto cleanup;
>  	if (cg_write(parent[1], "memory.min", "50M"))
>  		goto cleanup;
>  	if (cg_write(children[0], "memory.min", "75M"))
> @@ -404,8 +402,8 @@ static int test_memcg_min(const char *root)
>  
>  /*
>   * First, this test creates the following hierarchy:
> - * A       memory.low = 50M,  memory.max = 200M
> - * A/B     memory.low = 50M,  memory.current = 50M
> + * A       memory.low = 0,    memory.max = 200M
> + * A/B     memory.low = 50M,  memory.current = ...

Can you, please, just remove "memory.current = ...", it's not
because obvious what "..." means here.

Other than that: Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Thank you!

On Fri, 13 May 2022 11:59:56 -0700 Roman Gushchin <roman.gushchin@linux.dev> wrote:

> On Fri, May 13, 2022 at 07:18:11PM +0200, Michal Koutny wrote:
> > The reclaim is triggered by memory limit in a subtree, therefore the
> > testcase does not need configured protection against external reclaim.
> > 
> > Also, correct/deduplicate respective comments
> > 
> > Signed-off-by: Michal Koutný <mkoutny@suse.com>
> > ---
> >  tools/testing/selftests/cgroup/test_memcontrol.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> > index 9ffacf024bbd..9d370aafd799 100644
> > --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> > @@ -247,7 +247,7 @@ static int cg_test_proc_killed(const char *cgroup)
> >  
> >  /*
> >   * First, this test creates the following hierarchy:
> > - * A       memory.min = 50M,  memory.max = 200M
> > + * A       memory.min = 0,    memory.max = 200M
> >   * A/B     memory.min = 50M,  memory.current = 50M
> >   * A/B/C   memory.min = 75M,  memory.current = 50M
> >   * A/B/D   memory.min = 25M,  memory.current = 50M
> > @@ -257,7 +257,7 @@ static int cg_test_proc_killed(const char *cgroup)
> >   * Usages are pagecache, but the test keeps a running
> >   * process in every leaf cgroup.
> >   * Then it creates A/G and creates a significant
> > - * memory pressure in it.
> > + * memory pressure in A.
> >   *
> >   * A/B    memory.current ~= 50M
> >   * A/B/C  memory.current ~= 29M
> > @@ -335,8 +335,6 @@ static int test_memcg_min(const char *root)
> >  			      (void *)(long)fd);
> >  	}
> >  
> > -	if (cg_write(parent[0], "memory.min", "50M"))
> > -		goto cleanup;
> >  	if (cg_write(parent[1], "memory.min", "50M"))
> >  		goto cleanup;
> >  	if (cg_write(children[0], "memory.min", "75M"))
> > @@ -404,8 +402,8 @@ static int test_memcg_min(const char *root)
> >  
> >  /*
> >   * First, this test creates the following hierarchy:
> > - * A       memory.low = 50M,  memory.max = 200M
> > - * A/B     memory.low = 50M,  memory.current = 50M
> > + * A       memory.low = 0,    memory.max = 200M
> > + * A/B     memory.low = 50M,  memory.current = ...
> 
> Can you, please, just remove "memory.current = ...", it's not
> because obvious what "..." means here.
> 

You mean this?

--- a/tools/testing/selftests/cgroup/test_memcontrol.c~selftests-memcg-remove-protection-from-top-level-memcg-fix
+++ a/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -403,15 +403,14 @@ cleanup:
 /*
  * First, this test creates the following hierarchy:
  * A       memory.low = 0,    memory.max = 200M
- * A/B     memory.low = 50M,  memory.current = ...
+ * A/B     memory.low = 50M
  * A/B/C   memory.low = 75M,  memory.current = 50M
  * A/B/D   memory.low = 25M,  memory.current = 50M
  * A/B/E   memory.low = 0,    memory.current = 50M
  * A/B/F   memory.low = 500M, memory.current = 0
  *
  * Usages are pagecache.
- * Then it creates A/G an creates a significant
- * memory pressure in it.
+ * Then it creates A/G and creates significant memory pressure in it.
  *
  * Then it checks actual memory usages and expects that:
  * A/B    memory.current ~= 50M
_

(includes gratuitous comment cleanup)

I assume your comment in
https://lkml.kernel.org/r/Yn6pBPq+lAXm9NG8@carbon can be addressed in a
later patch.

I'm not sure what to amke of https://lkml.kernel.org/r/Yn6pWPodGPlz+D8G@carbon

Do we feel this series needs more work before merging it up?

On Tue, May 17, 2022 at 05:24:43PM -0700, Andrew Morton wrote:
> On Fri, 13 May 2022 11:59:56 -0700 Roman Gushchin <roman.gushchin@linux.dev> wrote:
> 
> > On Fri, May 13, 2022 at 07:18:11PM +0200, Michal Koutny wrote:
> > > The reclaim is triggered by memory limit in a subtree, therefore the
> > > testcase does not need configured protection against external reclaim.
> > > 
> > > Also, correct/deduplicate respective comments
> > > 
> > > Signed-off-by: Michal Koutný <mkoutny@suse.com>
> > > ---
> > >  tools/testing/selftests/cgroup/test_memcontrol.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> > > index 9ffacf024bbd..9d370aafd799 100644
> > > --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> > > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> > > @@ -247,7 +247,7 @@ static int cg_test_proc_killed(const char *cgroup)
> > >  
> > >  /*
> > >   * First, this test creates the following hierarchy:
> > > - * A       memory.min = 50M,  memory.max = 200M
> > > + * A       memory.min = 0,    memory.max = 200M
> > >   * A/B     memory.min = 50M,  memory.current = 50M
> > >   * A/B/C   memory.min = 75M,  memory.current = 50M
> > >   * A/B/D   memory.min = 25M,  memory.current = 50M
> > > @@ -257,7 +257,7 @@ static int cg_test_proc_killed(const char *cgroup)
> > >   * Usages are pagecache, but the test keeps a running
> > >   * process in every leaf cgroup.
> > >   * Then it creates A/G and creates a significant
> > > - * memory pressure in it.
> > > + * memory pressure in A.
> > >   *
> > >   * A/B    memory.current ~= 50M
> > >   * A/B/C  memory.current ~= 29M
> > > @@ -335,8 +335,6 @@ static int test_memcg_min(const char *root)
> > >  			      (void *)(long)fd);
> > >  	}
> > >  
> > > -	if (cg_write(parent[0], "memory.min", "50M"))
> > > -		goto cleanup;
> > >  	if (cg_write(parent[1], "memory.min", "50M"))
> > >  		goto cleanup;
> > >  	if (cg_write(children[0], "memory.min", "75M"))
> > > @@ -404,8 +402,8 @@ static int test_memcg_min(const char *root)
> > >  
> > >  /*
> > >   * First, this test creates the following hierarchy:
> > > - * A       memory.low = 50M,  memory.max = 200M
> > > - * A/B     memory.low = 50M,  memory.current = 50M
> > > + * A       memory.low = 0,    memory.max = 200M
> > > + * A/B     memory.low = 50M,  memory.current = ...
> > 
> > Can you, please, just remove "memory.current = ...", it's not
> > because obvious what "..." means here.
> > 
> 
> You mean this?
> 
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c~selftests-memcg-remove-protection-from-top-level-memcg-fix
> +++ a/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -403,15 +403,14 @@ cleanup:
>  /*
>   * First, this test creates the following hierarchy:
>   * A       memory.low = 0,    memory.max = 200M
> - * A/B     memory.low = 50M,  memory.current = ...
> + * A/B     memory.low = 50M
>   * A/B/C   memory.low = 75M,  memory.current = 50M
>   * A/B/D   memory.low = 25M,  memory.current = 50M
>   * A/B/E   memory.low = 0,    memory.current = 50M
>   * A/B/F   memory.low = 500M, memory.current = 0
>   *
>   * Usages are pagecache.
> - * Then it creates A/G an creates a significant
> - * memory pressure in it.
> + * Then it creates A/G and creates significant memory pressure in it.
>   *
>   * Then it checks actual memory usages and expects that:
>   * A/B    memory.current ~= 50M
> _
> 
> (includes gratuitous comment cleanup)

Yes, thank you!

> 
> I assume your comment in
> https://lkml.kernel.org/r/Yn6pBPq+lAXm9NG8@carbon can be addressed in a
> later patch.
> 
> I'm not sure what to amke of https://lkml.kernel.org/r/Yn6pWPodGPlz+D8G@carbon
> 
> Do we feel this series needs more work before merging it up?
> 

Please, go ahead with it. If anything comes up, it can be addressed later.

Thanks!

On Tue, May 17, 2022 at 05:52:25PM -0700, Roman Gushchin <roman.gushchin@linux.dev> wrote:
> Please, go ahead with it. If anything comes up, it can be addressed later.

I hope I can still post v2 of the tests cleanups (applying feedback from
here), it's in [1] with more info there. (I sent it to a new thread
based on get_maintainers.pl).

Thanks,
Michal

[1] https://lore.kernel.org/r/20220518154037.18819-1-mkoutny@suse.com