Re: [LTP] [PATCH v2 5/5] memcontrol03: Copy from kselftest

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

>> +
>> +static void alloc_anon_in_child(const struct tst_cgroup_group *const cg,
>> +				const size_t size, const int expect_oom)
>> +{
>> +	int status;
>> +	const pid_t pid = SAFE_FORK();
>> +
>> +	if (!pid) {
>> +		SAFE_CGROUP_PRINTF(cg, "cgroup.procs", "%d", getpid());
>> +
>> +		tst_res(TINFO, "%d in %s: Allocating anon: %"PRIdPTR,
>> +		getpid(), tst_cgroup_group_name(cg), size);
>> +		alloc_anon(size);
>> +		exit(0);
>> +	}
>> +
>> +	SAFE_WAITPID(pid, &status, 0);
>> +
>> +	if (!expect_oom) {
>> +		if (WIFEXITED(status) && WEXITSTATUS(status) == 0)
>> +			return;
>> +	} else {
>> +		if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL)
>> +			return;
>> +	}
>
> Maybe we should print TPASS in these as well to complement the TFAIL
> below. Also the negation in !expect_oom is kind of useless since we
> handle both cases anyways.

Can't hurt I guess.

>
>> +	tst_res(TFAIL,
>> +		"Expected child %d to %s, but instead %s",
>> +		pid,
>> +		expect_oom ? "be killed" : "exit(0)",
>> +		tst_strstatus(status));
>> +}
>> +
>> +static void alloc_pagecache_in_child(const struct tst_cgroup_group *const cg,
>> +				     const size_t size)
>> +{
>> +	const pid_t pid = SAFE_FORK();
>> +
>> +	if (pid) {
>> +		TST_CHECKPOINT_WAIT(CHILD_IDLE);
>> +		return;
>> +	}
>> +
>> +	SAFE_CGROUP_PRINTF(cg, "cgroup.procs", "%d", getpid());
>> +
>> +	tst_res(TINFO, "PID %d in %s: Allocating pagecache: %"PRIdPTR,
>> +		getpid(), tst_cgroup_group_name(cg), size);
>> +	alloc_pagecache(fd, size);
>> +
>> +	TST_CHECKPOINT_WAKE(CHILD_IDLE);
>> +	TST_CHECKPOINT_WAIT(TEST_DONE);
>> +	exit(0);
>> +}
>> +
>> +static void test_memcg_min(void)
>> +{
>> +	long c[4];
>> +	unsigned int i;
>> +	size_t attempts;
>> +
>> +	fd = SAFE_OPEN(TMPDIR"/tmpfile", O_RDWR | O_CREAT, 0600);
>> +	trunk_cg[A] = tst_cgroup_group_mk(tst_cgroup, "trunk_A");
>> +
>> +	SAFE_CGROUP_SCANF(trunk_cg[A], "memory.min", "%ld", c);
>> +	if (c[0]) {
>> +		tst_brk(TCONF,
>> +			"memory.min already set to %ld on parent group", c[0]);
>> +	}
>> +
>> +	if (!TST_CGROUP_VER_IS_V1(trunk_cg[A], "memory")) {
>> +		SAFE_CGROUP_PRINT(trunk_cg[A], "cgroup.subtree_control",
>> +				  "+memory");
>> +	}
>> +	SAFE_CGROUP_PRINT(trunk_cg[A], "memory.max", "200M");
>> +	SAFE_CGROUP_PRINT(trunk_cg[A], "memory.swap.max", "0");
>> +
>> +	trunk_cg[B] = tst_cgroup_group_mk(trunk_cg[A], "trunk_B");
>> +	if (!TST_CGROUP_VER_IS_V1(trunk_cg[A], "memory")) {
>> +		SAFE_CGROUP_PRINT(trunk_cg[B], "cgroup.subtree_control",
>> +				  "+memory");
>> +	}
>
> Don't we require V2 for the whole test anyways?

+1

>
> Also I think I asked if this si really needed in v1, don't we enable the
> required subgroups in group_mk() anyways?

No, we only automatically add the controllers to subtree_control in the
test's group. Note that this is not the same as enabling the
controller. The controller becomes active as soon as it is added to the
root node's subtree control. Adding it to subtree_control means that the
direct children get their own allotments in addition to sharing the
parent's. I assume that for some tests we don't want that, we want the
children to fully compete over the parent's resources.

Also subtree_control can effect whether a group is a threaded
domain... We could perhaps add a variant of `tst_cgroup_group_mk` which
enables subtrees by default. I just don't want it to be hidden that we
are enabling it.


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp