[LTP] [PATCH v2] memcg/memcontrol04: Fix judgment for recursive_protection
Li Wang
liwang@redhat.com
Thu Jan 9 07:53:11 CET 2025
Hi Jin,
Could you help resend a new patch version based on the review comments?
(and rebase the code to the latest branch to avoid patch conflict errors)
It makes a lot of sense to merge your work before the new LTP release.
On Wed, Dec 25, 2024 at 7:38 PM Li Wang <liwang@redhat.com> wrote:
> Hi Jin,
>
> On Thu, Dec 19, 2024 at 2:09 PM Jin Guojie <guojie.jin@gmail.com> wrote:
>
>> V2:
>> * Change the expected events in F depending on memory_recursiveprot
>>
>> In Patch v1[1], the role of recursive_protection in cgoup v2 was not
>> considered.
>>
>> By carefully reading the algorithm in the kernel function
>> effective_protection(). When memory_recursiveprot is enabled, a
>> subgroup with usage > 0 can get unclaimed greater than 0.
>>
>> The purpose of doing this should be to achieve the essential purpose
>> of recursive_protection: the sum of all subgroups' unprotected values
>> is equal to parent's unprotected values.
>>
>> Even though the documentation does not give an explicit description
>> for recursive_memoryprot, it looks like the kernel's processing
>> algorithm is reasonable.
>>
>> Based on the idea of [2], Patch v2 is rewritten to determine whether
>> memory_recursiveprot is enabled.
>>
>> On distributions with memory_recursiveprot enabled by default (from
>> Ubuntu 22.04 to 24.10), running this passes:
>>
>> memcontrol04.c:208: TPASS: Expect: (C oom events=0) == 0
>> memcontrol04.c:211: TPASS: Expect: (C low events=966) > 0
>> memcontrol04.c:208: TPASS: Expect: (D oom events=0) == 0
>> memcontrol04.c:211: TPASS: Expect: (D low events=966) > 0
>> memcontrol04.c:208: TPASS: Expect: (E oom events=0) == 0
>> memcontrol04.c:214: TPASS: Expect: (E low events=0) == 0
>> memcontrol04.c:208: TPASS: Expect: (F oom events=0) == 0
>> memcontrol04.c:211: TPASS: Expect: (F low events=874) > 0
>>
>> [1] https://lists.linux.it/pipermail/ltp/2024-November/040946.html
>> [2] https://lore.kernel.org/ltp/20220222144511.GA12037@blackbody.suse.cz/
>>
>> Signed-off-by: Jin Guojie <guojie.jin@gmail.com>
>> Suggested-by: Richard Palethorpe <rpalethorpe@suse.com>
>> Suggested-by: Michal Koutný <mkoutny@suse.com>
>> ---
>> include/tst_cgroup.h | 2 ++
>> lib/tst_cgroup.c | 12 ++++++++++++
>> testcases/kernel/controllers/memcg/memcontrol04.c | 2 +-
>> 3 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
>> index d23a8e652..068ff8306 100644
>> --- a/include/tst_cgroup.h
>> +++ b/include/tst_cgroup.h
>> @@ -256,4 +256,6 @@ int safe_cg_occursin(const char *file, const int
>> lineno,
>> const char *const file_name,
>> const char *const needle);
>>
>> +int tst_cg_memory_recursiveprot(struct tst_cg_group *cg);
>> +
>> #endif /* TST_CGROUP_H */
>> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
>> index 6055015eb..9e3b21ed0 100644
>> --- a/lib/tst_cgroup.c
>> +++ b/lib/tst_cgroup.c
>> @@ -76,6 +76,8 @@ struct cgroup_root {
>> int we_mounted_it:1;
>> /* cpuset is in compatability mode */
>> int no_cpuset_prefix:1;
>> +
>> + int memory_recursiveprot:1;
>>
>
> I prefer to move it up to put it together with the nsdelegate.
>
>
>
>> };
>>
>> /* Controller sub-systems */
>> @@ -592,6 +594,7 @@ static void cgroup_root_scan(const char *const
>> mnt_type,
>> }
>> for (tok = strtok(mnt_opts, ","); tok; tok = strtok(NULL, ",")) {
>> nsdelegate |= !strcmp("nsdelegate", tok);
>> + root->memory_recursiveprot |=
>> !strcmp("memory_recursiveprot", tok);
>>
>
>
> Why not define a single variable memory_recursiveprot to save the value
> and move the root->memory_recursiveprot assignment to the "backref" part?
> (just like what we did for nsdelegate)
>
>
>
>> }
>>
>> if (root->ver && ctrl_field == root->ctrl_field)
>> @@ -718,6 +721,7 @@ mount:
>> tst_res(TINFO, "Mounted V2 CGroups on %s", mnt_path);
>> tst_cg_scan();
>> roots[0].we_mounted_it = 1;
>> + roots[0].memory_recursiveprot = 1;
>>
>
> This is completely wrong, as the first mount operation may fail,
> while CGroupV2 falls back to the default mount and succeeds.
>
> Hence we should remove the line here, as tst_cg_scan() helps
> to automatically detect and set "root->memory_recursiveprot"
> to the correct value.
>
>
>
>> return;
>> }
>>
>> @@ -1509,3 +1513,11 @@ int safe_cg_occursin(const char *const file,
>> const int lineno,
>>
>> return !!strstr(buf, needle);
>> }
>> +
>> +int tst_cg_memory_recursiveprot(struct tst_cg_group *cg)
>> +{
>> + if (cg && cg->dirs_by_ctrl[0]->dir_root)
>> + return
>> cg->dirs_by_ctrl[0]->dir_root->memory_recursiveprot;
>> + return 0;
>> +}
>> +
>> diff --git a/testcases/kernel/controllers/memcg/memcontrol04.c
>> b/testcases/kernel/controllers/memcg/memcontrol04.c
>> index 1b8d115f8..9e9d6ab6e 100644
>> --- a/testcases/kernel/controllers/memcg/memcontrol04.c
>> +++ b/testcases/kernel/controllers/memcg/memcontrol04.c
>> @@ -207,7 +207,7 @@ static void test_memcg_low(void)
>>
>> TST_EXP_EXPR(oom == 0, "(%c oom events=%ld) == 0", id,
>> oom);
>>
>> - if (i < E) {
>> + if (i < E || ((i == F) &&
>> tst_cg_memory_recursiveprot(leaf_cg[F]))) {
>> TST_EXP_EXPR(low > 0,
>> "(%c low events=%ld) > 0", id, low);
>> } else {
>> --
>> 2.34.1
>>
>> --
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>>
>
>
> --
> Regards,
> Li Wang
>
--
Regards,
Li Wang
More information about the ltp
mailing list