[PATCH 0/4] mm: memcg kselftests fixes

[PATCH 0/4] mm: memcg kselftests fixes

[Roman Gushchin]

I'm resending two memcg kselftests fixes, which I first posted in 2019 [1],
however they didn't go anywhere. I've rechecked them and they're are both
still actual, even though there were new regressions introduced since that,
so not all tests are passing now. I know that David Vernet (cc'ed) is working
on fixing (some of) them, so hopefully he'll post more fixes soon.

I believe it's better for such patches to go through mm and cgroup trees
(and mailing lists), where there are better chances for them to be properly
reviewed, so adding corresponding entries to the MAINTAINERS file to make it
obvious.

1: https://lore.kernel.org/lkml/20191203165758.GA607734@chrisdown.name/T/


Roman Gushchin (4):
  kselftests: memcg: update the oom group leaf events test
  kselftests: memcg: speed up the memory.high test
  MAINTAINERS: add corresponding kselftests to cgroup entry
  MAINTAINERS: add corresponding kselftests to memcg entry

 MAINTAINERS                                      | 3 +++
 tools/testing/selftests/cgroup/test_memcontrol.c | 7 ++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.35.1

[PATCH 1/4] kselftests: memcg: update the oom group leaf events test

[Roman Gushchin]

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 <roman.gushchin@linux.dev>
Cc: Chris Down <chris@chrisdown.name>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 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

[PATCH 2/4] kselftests: memcg: speed up the memory.high test

[Roman Gushchin]

After commit 0e4b01df8659 ("mm, memcg: throttle allocators when
failing reclaim over memory.high") allocating memory over memory.high
became very time consuming. But it's exactly what the memory.high
test from cgroup kselftests is doing: it tries to allocate 100M with
30M memory.high value. It takes forever to complete.

In order to keep it passing (or failing) in a reasonable amount of
time let's try to allocate only a little over 30M: 31M to be precise.

With this change test_memcontrol finishes in a reasonable amount of
time:
  $ time ./test_memcontrol
  ok 1 test_memcg_subtree_control
  ok 2 test_memcg_current
  ok 3 test_memcg_min
  ok 4 test_memcg_low
  ok 5 test_memcg_high
  ok 6 test_memcg_max
  ok 7 test_memcg_oom_events
  ok 8 test_memcg_swap_max
  ok 9 test_memcg_sock
  ok 10 test_memcg_oom_group_leaf_events
  ok 11 test_memcg_oom_group_parent_events
  ok 12 test_memcg_oom_group_score_events

  real	0m2.273s
  user	0m0.064s
  sys	0m0.739s

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Chris Down <chris@chrisdown.name>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 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 00b430e7f2a2..9c1f19fe2e37 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -607,7 +607,7 @@ static int test_memcg_high(const char *root)
 	if (cg_write(memcg, "memory.high", "30M"))
 		goto cleanup;
 
-	if (cg_run(memcg, alloc_anon, (void *)MB(100)))
+	if (cg_run(memcg, alloc_anon, (void *)MB(31)))
 		goto cleanup;
 
 	if (!cg_run(memcg, alloc_pagecache_50M_check, NULL))
-- 
2.35.1

[PATCH 3/4] MAINTAINERS: add corresponding kselftests to cgroup entry

[Roman Gushchin]

List cgroup kselftests in the cgroup MAINTAINERS entry.
These are tests covering core, freezer and cgroup.kill
functionality.

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Tejun Heo <tj@kernel.org>
Cc: Zefan Li <lizefan.x@bytedance.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: cgroups@vger.kernel.org
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d5731c03a485..44dabe0145ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4996,6 +4996,7 @@ F:	Documentation/admin-guide/cgroup-v1/
 F:	Documentation/admin-guide/cgroup-v2.rst
 F:	include/linux/cgroup*
 F:	kernel/cgroup/
+F:	tools/testing/selftests/cgroup/
 
 CONTROL GROUP - BLOCK IO CONTROLLER (BLKIO)
 M:	Tejun Heo <tj@kernel.org>
-- 
2.35.1

[PATCH 4/4] MAINTAINERS: add corresponding kselftests to memcg entry

[Roman Gushchin]

List memory control and kernel memory control kselftests in the memory
resource controller entry.

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Shakeel Butt <shakeelb@google.com>
Cc: cgroups@vger.kernel.org
Cc: linux-mm@kvack.org
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 44dabe0145ae..0dd3d276f330 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5030,6 +5030,8 @@ L:	linux-mm@kvack.org
 S:	Maintained
 F:	mm/memcontrol.c
 F:	mm/swap_cgroup.c
+F:	tools/testing/selftests/cgroup/test_kmem.c
+F:	tools/testing/selftests/cgroup/test_memcontrol.c
 
 CORETEMP HARDWARE MONITORING DRIVER
 M:	Fenghua Yu <fenghua.yu@intel.com>
-- 
2.35.1

Re: [PATCH 1/4] kselftests: memcg: update the oom group leaf events test

[David Vernet]

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 <roman.gushchin@linux.dev>
> Cc: Chris Down <chris@chrisdown.name>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  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 <void@manifault.com>

Re: [PATCH 2/4] kselftests: memcg: speed up the memory.high test

[David Vernet]

On Thu, Apr 14, 2022 at 05:01:31PM -0700, Roman Gushchin wrote:
> After commit 0e4b01df8659 ("mm, memcg: throttle allocators when
> failing reclaim over memory.high") allocating memory over memory.high
> became very time consuming. But it's exactly what the memory.high
> test from cgroup kselftests is doing: it tries to allocate 100M with
> 30M memory.high value. It takes forever to complete.
> 
> In order to keep it passing (or failing) in a reasonable amount of
> time let's try to allocate only a little over 30M: 31M to be precise.
> 
> With this change test_memcontrol finishes in a reasonable amount of
> time:
>   $ time ./test_memcontrol
>   ok 1 test_memcg_subtree_control
>   ok 2 test_memcg_current
>   ok 3 test_memcg_min
>   ok 4 test_memcg_low
>   ok 5 test_memcg_high
>   ok 6 test_memcg_max
>   ok 7 test_memcg_oom_events
>   ok 8 test_memcg_swap_max
>   ok 9 test_memcg_sock
>   ok 10 test_memcg_oom_group_leaf_events
>   ok 11 test_memcg_oom_group_parent_events
>   ok 12 test_memcg_oom_group_score_events
> 
>   real	0m2.273s
>   user	0m0.064s
>   sys	0m0.739s
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Chris Down <chris@chrisdown.name>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  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 00b430e7f2a2..9c1f19fe2e37 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -607,7 +607,7 @@ static int test_memcg_high(const char *root)
>  	if (cg_write(memcg, "memory.high", "30M"))
>  		goto cleanup;
>  
> -	if (cg_run(memcg, alloc_anon, (void *)MB(100)))
> +	if (cg_run(memcg, alloc_anon, (void *)MB(31)))
>  		goto cleanup;
>  
>  	if (!cg_run(memcg, alloc_pagecache_50M_check, NULL))
> -- 
> 2.35.1
> 

Thanks for re-sending this. Looks good.

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

Re: [PATCH 1/4] kselftests: memcg: update the oom group leaf events test

[Roman Gushchin]

> On Apr 15, 2022, at 7:08 AM, David Vernet <void@manifault.com> 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 <void@manifault.com>

Thanks!

Re: [PATCH 3/4] MAINTAINERS: add corresponding kselftests to cgroup entry

[Tejun Heo]

On Thu, Apr 14, 2022 at 05:01:32PM -0700, Roman Gushchin wrote:
> List cgroup kselftests in the cgroup MAINTAINERS entry.
> These are tests covering core, freezer and cgroup.kill
> functionality.
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Zefan Li <lizefan.x@bytedance.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: cgroups@vger.kernel.org

Acked-by: Tejun Heo <tj@kernel.org>

I suppose this can go with the rest through -mm? Please let me know if I
should pick it up.

Thanks.

-- 
tejun

Re: [PATCH 3/4] MAINTAINERS: add corresponding kselftests to cgroup entry

[Roman Gushchin]

> On Apr 21, 2022, at 12:25 PM, Tejun Heo <tj@kernel.org> wrote:
> 
> On Thu, Apr 14, 2022 at 05:01:32PM -0700, Roman Gushchin wrote:
>> List cgroup kselftests in the cgroup MAINTAINERS entry.
>> These are tests covering core, freezer and cgroup.kill
>> functionality.
>> 
>> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Zefan Li <lizefan.x@bytedance.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: cgroups@vger.kernel.org
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> I suppose this can go with the rest through -mm? Please let me know if I
> should pick it up.

Yep, Andrew picked it up already.

Thanks!