[LTP] [PATCH] memcontrol03: Account for process size in cgroup allocation

[Martin Doucha]

On 07. 05. 25 16:23, Cyril Hrubis wrote:
> Hi!
>> The first trunk_G allocation has 2MB safety margin to avoid triggering
>> OOM killer. However, on systems with 64K pagesize, this may not be enough.
>> Account for process size as reported by cgroup memory stats before
>> allocating memory in child processes.
> 
> Is there a reason to keep the 2MB safety after this patch?

I'd say there's no reason to remove it. On x86_64, the patch will 
increase the safety margin by only 256KB and that memory is already 
allocated to the cgroup. If we remove the safety margin, any additional 
buffer allocation in glibc may trigger OOM.

> Or can we do:
> 
> diff --git a/testcases/kernel/controllers/memcg/memcontrol03.c b/testcases/kernel/controllers/memcg/memcontrol03.c
> index b5bbb9954..e7f126880 100644
> --- a/testcases/kernel/controllers/memcg/memcontrol03.c
> +++ b/testcases/kernel/controllers/memcg/memcontrol03.c
> @@ -200,7 +200,7 @@ static void test_memcg_min(void)
>                  sleep(1);
>          }
> 
> -       alloc_anon_in_child(trunk_cg[G], MB(148), 0);
> +       alloc_anon_in_child(trunk_cg[G], MB(150), 0);
> 
>          SAFE_CG_SCANF(trunk_cg[B], "memory.current", "%ld", c);
>          TST_EXP_EXPR(values_close(c[0], MB(50), 5),
> 
>> --- a/testcases/kernel/controllers/memcg/memcontrol03.c
>> +++ b/testcases/kernel/controllers/memcg/memcontrol03.c
>> @@ -94,17 +94,23 @@ static void cleanup_sub_groups(void)
>>   }
>>   
>>   static void alloc_anon_in_child(const struct tst_cg_group *const cg,
>> -				const size_t size, const int expect_oom)
>> +	size_t size, const int expect_oom)
>>   {
>>   	int status;
>>   	const pid_t pid = SAFE_FORK();
>> +	size_t cgmem;
>>   
>>   	if (!pid) {
>>   		SAFE_CG_PRINTF(cg, "cgroup.procs", "%d", getpid());
>> +		SAFE_CG_SCANF(cg, "memory.current", "%zu", &cgmem);
>> +		size = size > cgmem ? size - cgmem : 0;
> 
> Here we depend on the fact that process memory has been properly
> accounted for when it starts running its code. Are you sure that we can
> rely on this or does this just happen to work?

Actually, my commit message is slightly misleading because the existing 
process memory does not get migrated to the new cgroup. But the cgroup 
itself may already have non-zero memory usage even when empty, likely 
for internal kernel structures. Any new allocations of kernel structures 
should also be finished when the process migration completes. So unless 
the migration behavior changes in the near future, we can rely on this.

This sentence in the commit message:
"Account for process size as reported by cgroup memory stats before..."
should be changed to:
"Account for existing cgroup memory usage before..."

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic