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
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ý <[email protected]>
---
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? <[email protected]>
> ---
> 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 <[email protected]>
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ý <[email protected]>
---
.../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 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? <[email protected]>
> ---
> .../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 <[email protected]>
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? <[email protected]>
> ---
> .../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 <[email protected]>
Thanks!
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ý <[email protected]>
---
.../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 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? <[email protected]>
> > ---
> > .../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.