Commit 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") made
memory.events recursive: all events are propagated upwards by the
tree. It was a change in semantics.
It broke the oom group leaf events test: it assumes that after
an OOM the oom_kill counter is zero on parent's level.
Let's adjust the test: it should have similar expectations
for the child and parent levels.
The test passes after this fix.
Signed-off-by: Roman Gushchin <[email protected]>
Cc: Chris Down <[email protected]>
Cc: Johannes Weiner <[email protected]>
---
tools/testing/selftests/cgroup/test_memcontrol.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index 36ccf2322e21..00b430e7f2a2 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -1079,7 +1079,8 @@ static int test_memcg_sock(const char *root)
/*
* This test disables swapping and tries to allocate anonymous memory
* up to OOM with memory.group.oom set. Then it checks that all
- * processes in the leaf (but not the parent) were killed.
+ * processes in the leaf were killed. It also checks that oom_events
+ * were propagated to the parent level.
*/
static int test_memcg_oom_group_leaf_events(const char *root)
{
@@ -1122,7 +1123,7 @@ 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)
+ if (cg_read_key_long(parent, "memory.events", "oom_kill ") <= 0)
goto cleanup;
ret = KSFT_PASS;
--
2.35.1
On Thu, Apr 14, 2022 at 05:01:30PM -0700, Roman Gushchin wrote:
> Commit 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") made
> memory.events recursive: all events are propagated upwards by the
> tree. It was a change in semantics.
In one of our offline discussions you mentioned that we may want to
consider having the test take mount options into account. If we decide to
go that route we should probably have this testcase take memory_localevents
into account as well. If so, I'm happy to take care of that in a follow-on
patch after this is merged as I already have a patch locally that reads and
parses /proc/mounts to detect these mount options.
>
> It broke the oom group leaf events test: it assumes that after
> an OOM the oom_kill counter is zero on parent's level.
>
> Let's adjust the test: it should have similar expectations
> for the child and parent levels.
>
> The test passes after this fix.
>
> Signed-off-by: Roman Gushchin <[email protected]>
> Cc: Chris Down <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> ---
> tools/testing/selftests/cgroup/test_memcontrol.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 36ccf2322e21..00b430e7f2a2 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -1079,7 +1079,8 @@ static int test_memcg_sock(const char *root)
> /*
> * This test disables swapping and tries to allocate anonymous memory
> * up to OOM with memory.group.oom set. Then it checks that all
> - * processes in the leaf (but not the parent) were killed.
> + * processes in the leaf were killed. It also checks that oom_events
> + * were propagated to the parent level.
> */
> static int test_memcg_oom_group_leaf_events(const char *root)
> {
> @@ -1122,7 +1123,7 @@ 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)
> + if (cg_read_key_long(parent, "memory.events", "oom_kill ") <= 0)
> goto cleanup;
>
> ret = KSFT_PASS;
> --
> 2.35.1
>
Looks good, thanks.
Reviewed-by: David Vernet <[email protected]>
> On Apr 15, 2022, at 7:08 AM, David Vernet <[email protected]> wrote:
>
> On Thu, Apr 14, 2022 at 05:01:30PM -0700, Roman Gushchin wrote:
>> Commit 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") made
>> memory.events recursive: all events are propagated upwards by the
>> tree. It was a change in semantics.
>
> In one of our offline discussions you mentioned that we may want to
> consider having the test take mount options into account. If we decide to
> go that route we should probably have this testcase take memory_localevents
> into account as well. If so, I'm happy to take care of that in a follow-on
> patch after this is merged as I already have a patch locally that reads and
> parses /proc/mounts to detect these mount options.
It would be great, thank you!
>> - if (cg_read_key_long(parent, "memory.events", "oom_kill ") != 0)
>> + if (cg_read_key_long(parent, "memory.events", "oom_kill ") <= 0)
>> goto cleanup;
>>
>> ret = KSFT_PASS;
>> --
>> 2.35.1
>>
>
> Looks good, thanks.
>
> Reviewed-by: David Vernet <[email protected]>
Thanks!