| Message ID | 20220422155728.3055914-2-void@manifault.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix bugs in memcontroller cgroup tests | expand |
On Fri, Apr 22, 2022 at 08:57:25AM -0700, David Vernet wrote: > In test_memcg_min() and test_memcg_low(), there is an array of four sibling > cgroups. All but one of these sibling groups does a 50MB allocation, and > the group that does no allocation is the third of four in the array. This > is not a problem per se, but makes it a bit tricky to do some assertions in > test_memcg_low(), as we want to make assertions on the siblings based on > whether or not they performed allocations. Having a static index before > which all groups have performed an allocation makes this cleaner. > > This patch therefore reorders the sibling groups so that the group that > performs no allocations is the last in the array. It makes the comment explaining the test just above the test_memcg_min() function obsolete. Please, fix it too. Thanks!
On Fri, Apr 22, 2022 at 04:04:15PM -0700, Roman Gushchin wrote: > Thanks for the reviews on this patchset, Roman. FYI I think Andrew already merged these patches to the -mm tree. I'll send out a follow-on patch that fixes everything you pointed out, both here and on the other patches in the set. > On Fri, Apr 22, 2022 at 08:57:25AM -0700, David Vernet wrote: > > In test_memcg_min() and test_memcg_low(), there is an array of four sibling > > cgroups. All but one of these sibling groups does a 50MB allocation, and > > the group that does no allocation is the third of four in the array. This > > is not a problem per se, but makes it a bit tricky to do some assertions in > > test_memcg_low(), as we want to make assertions on the siblings based on > > whether or not they performed allocations. Having a static index before > > which all groups have performed an allocation makes this cleaner. > > > > This patch therefore reorders the sibling groups so that the group that > > performs no allocations is the last in the array. > > It makes the comment explaining the test just above the test_memcg_min() > function obsolete. Please, fix it too. Thanks for catching that. I'll fix the comment both in test_memcg_min() and test_memcg_low() when I send out that follow-on patch.
> On Apr 23, 2022, at 4:30 AM, David Vernet <void@manifault.com> wrote: > > On Fri, Apr 22, 2022 at 04:04:15PM -0700, Roman Gushchin wrote: >> > > Thanks for the reviews on this patchset, Roman. FYI I think Andrew already > merged these patches to the -mm tree. I'll send out a follow-on patch that > fixes everything you pointed out, both here and on the other patches in the > set. The mm tree isn’t a git tree, but a collection of the text patches, managed by Andrew. So you can send a new version and Andrew can update it in place. It’s happening all the time: mostly for adding reviewed-by/acked-by tags etc, but for code updates as well. It’s not uncommon for some patchset to mature while being in the mm tree, this allows to include them into linux-next and give some more testing, but without doing many reverts/fixups (Andrew is often squashing fixups into the original patch too). So long story short, you can just send a new version, especially because all changes all minor. Thanks!
On Sat, Apr 23, 2022 at 08:19:12AM -0700, Roman Gushchin wrote: > > > On Apr 23, 2022, at 4:30 AM, David Vernet <void@manifault.com> wrote: > > > > On Fri, Apr 22, 2022 at 04:04:15PM -0700, Roman Gushchin wrote: > >> > > > > Thanks for the reviews on this patchset, Roman. FYI I think Andrew already > > merged these patches to the -mm tree. I'll send out a follow-on patch that > > fixes everything you pointed out, both here and on the other patches in the > > set. > > The mm tree isn’t a git tree, but a collection of the text patches, managed by Andrew. So you can send a new version and Andrew can update it in place. It’s happening all the time: mostly for adding reviewed-by/acked-by tags etc, but for code updates as well. > It’s not uncommon for some patchset to mature while being in the mm tree, this allows to include them into linux-next and give some more testing, but without doing many reverts/fixups (Andrew is often squashing fixups into the original patch too). So long story short, you can just send a new version, especially because all changes all minor. Ah, that makes sense. Thanks for explaining that. I'll send out a v2 of the patches shortly, then!
diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c index 6b5259394e68..aa50eaa8b157 100644 --- a/tools/testing/selftests/cgroup/test_memcontrol.c +++ b/tools/testing/selftests/cgroup/test_memcontrol.c @@ -321,7 +321,7 @@ static int test_memcg_min(const char *root) if (cg_create(children[i])) goto cleanup; - if (i == 2) + if (i > 2) continue; cg_run_nowait(children[i], alloc_pagecache_50M_noexit, @@ -336,9 +336,9 @@ static int test_memcg_min(const char *root) goto cleanup; if (cg_write(children[1], "memory.min", "25M")) goto cleanup; - if (cg_write(children[2], "memory.min", "500M")) + if (cg_write(children[2], "memory.min", "0")) goto cleanup; - if (cg_write(children[3], "memory.min", "0")) + if (cg_write(children[3], "memory.min", "500M")) goto cleanup; attempts = 0; @@ -364,7 +364,7 @@ static int test_memcg_min(const char *root) if (!values_close(c[1], MB(17), 20)) goto cleanup; - if (!values_close(c[2], 0, 1)) + if (c[3] != 0) goto cleanup; if (!cg_run(parent[2], alloc_anon, (void *)MB(170))) @@ -476,7 +476,7 @@ static int test_memcg_low(const char *root) if (cg_create(children[i])) goto cleanup; - if (i == 2) + if (i > 2) continue; if (cg_run(children[i], alloc_pagecache_50M, (void *)(long)fd)) @@ -491,9 +491,9 @@ static int test_memcg_low(const char *root) goto cleanup; if (cg_write(children[1], "memory.low", "25M")) goto cleanup; - if (cg_write(children[2], "memory.low", "500M")) + if (cg_write(children[2], "memory.low", "0")) goto cleanup; - if (cg_write(children[3], "memory.low", "0")) + if (cg_write(children[3], "memory.low", "500M")) goto cleanup; if (cg_run(parent[2], alloc_anon, (void *)MB(148))) @@ -511,7 +511,7 @@ static int test_memcg_low(const char *root) if (!values_close(c[1], MB(17), 20)) goto cleanup; - if (!values_close(c[2], 0, 1)) + if (c[3] != 0) goto cleanup; if (cg_run(parent[2], alloc_anon, (void *)MB(166))) {
In test_memcg_min() and test_memcg_low(), there is an array of four sibling cgroups. All but one of these sibling groups does a 50MB allocation, and the group that does no allocation is the third of four in the array. This is not a problem per se, but makes it a bit tricky to do some assertions in test_memcg_low(), as we want to make assertions on the siblings based on whether or not they performed allocations. Having a static index before which all groups have performed an allocation makes this cleaner. This patch therefore reorders the sibling groups so that the group that performs no allocations is the last in the array. A follow-on patch will leverage this to fix a bug in the test that incorrectly asserts that a sibling group that had performed an allocation, but only had protection from its parent, will not observe any memory.events.low events during reclaim. Signed-off-by: David Vernet <void@manifault.com> --- tools/testing/selftests/cgroup/test_memcontrol.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)