Re: [LTP] [PATCH v6] memcg/memcontrol05: add cgroup v2 task migration charge accounting test

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +enum checkpoints {
> +	WORKER_ALLOC_DONE,
> +	WORKER_RESUME,
> +	WORKER_ALLOC2_DONE,
> +	WORKER_EXIT,
> +};
> +
> +static struct tst_cg_group *group_a;
> +static struct tst_cg_group *group_b;
> +static pid_t worker_pid;
> +
> +enum worker_stage {
> +	STAGE_NOT_STARTED,
> +	STAGE_WAITING_RESUME,   /* worker blocked on WORKER_RESUME */
> +	STAGE_WAITING_EXIT,     /* worker blocked on WORKER_EXIT  */
> +	STAGE_DONE,             /* worker reaped                  */
> +};

First of all since we serialize only two processes, all that is needed
is single checkpoint.

> +static enum worker_stage worker_stage;
> +
> +static void touch_pages(char *buf, size_t size)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < size; i += getpagesize())
> +		buf[i] = 1;
> +}
> +
> +static void worker(void)
> +{
> +	char *buf1, *buf2;
> +
> +	SAFE_CG_PRINTF(group_a, "cgroup.procs", "%d", getpid());
> +
> +	buf1 = SAFE_MMAP(NULL, ALLOC_SIZE, PROT_READ | PROT_WRITE,
> +			 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +	touch_pages(buf1, ALLOC_SIZE);
> +
> +	TST_CHECKPOINT_WAKE(WORKER_ALLOC_DONE);
> +
> +	TST_CHECKPOINT_WAIT(WORKER_RESUME);

Should be just TST_CHECKPOINT_WAKE_AND_WAIT(0);

> +	buf2 = SAFE_MMAP(NULL, ALLOC_SIZE2, PROT_READ | PROT_WRITE,
> +			 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +	touch_pages(buf2, ALLOC_SIZE2);
> +
> +	TST_CHECKPOINT_WAKE(WORKER_ALLOC2_DONE);
> +
> +	TST_CHECKPOINT_WAIT(WORKER_EXIT);

Here as well.

> +	SAFE_MUNMAP(buf1, ALLOC_SIZE);
> +	SAFE_MUNMAP(buf2, ALLOC_SIZE2);
> +}

With the checkpoints unified to a single checkpoint we do not need to
track the worker_stage either.

> +static void test_memcg_task_migration(void)
> +{
> +	long baseline_b, after_migrate, after_migrate_a, after_alloc2, current_a;
> +
> +	worker_pid = 0;
> +	worker_stage = STAGE_NOT_STARTED;
> +
> +	group_a = tst_cg_group_mk(tst_cg, "group_a");
> +	group_b = tst_cg_group_mk(tst_cg, "group_b");
> +
> +	if (SAFE_CG_HAS(tst_cg, "memory.swap.max")) {
> +		SAFE_CG_PRINT(group_a, "memory.swap.max", "0");
> +		SAFE_CG_PRINT(group_b, "memory.swap.max", "0");
> +	}
> +
> +	SAFE_CG_SCANF(group_b, "memory.current", "%ld", &baseline_b);
> +	tst_res(TINFO, "group_b baseline memory.current=%ld", baseline_b);
> +
> +	worker_pid = SAFE_FORK();
> +	if (!worker_pid) {
> +		worker();
> +		exit(0);
> +	}
> +	worker_stage = STAGE_WAITING_RESUME;
> +
> +	TST_CHECKPOINT_WAIT(WORKER_ALLOC_DONE);
> +
> +	SAFE_CG_SCANF(group_a, "memory.current", "%ld", &current_a);


> +	tst_res(TINFO, "group_a memory.current=%ld after alloc", current_a);
> +	if (current_a < (long)ALLOC_SIZE) {
> +		tst_res(TFAIL,
> +			"group_a memory.current (%ld) < ALLOC_SIZE (%ld)",
> +			current_a, (long)ALLOC_SIZE);
> +		goto done;
> +	}
> +	tst_res(TPASS,
> +		"group_a memory.current (%ld) >= ALLOC_SIZE (%ld)",
> +		current_a, (long)ALLOC_SIZE);

This whole block should be TST_EXP_LE_LU()


> +	SAFE_CG_PRINTF(group_b, "cgroup.procs", "%d", worker_pid);
> +	tst_res(TINFO, "Migrated worker PID %d to group_b", worker_pid);
> +
> +	SAFE_CG_SCANF(group_b, "memory.current", "%ld", &after_migrate);
> +
> +	tst_res(TINFO,
> +		"group_b memory.current=%ld after migration (baseline=%ld)",
> +		after_migrate, baseline_b);
> +
> +	TST_EXP_EXPR(after_migrate <= baseline_b + (long)MB(4),
> +		     "group_b memory.current (%ld) not increased after migration (baseline=%ld)",
> +		     after_migrate, baseline_b);

Where did the MB(4) came from? Magic constants like that surely wouldn't
work universally.

> +	SAFE_CG_SCANF(group_a, "memory.current", "%ld", &after_migrate_a);
> +	tst_res(TINFO, "group_a memory.current=%ld after migration", after_migrate_a);
> +	TST_EXP_EXPR(after_migrate_a >= (long)ALLOC_SIZE,
> +		     "group_a memory.current (%ld) still holds pre-migration charges (>= ALLOC_SIZE %ld)",
> +		     after_migrate_a, (long)ALLOC_SIZE);
> +
> +	TST_CHECKPOINT_WAKE(WORKER_RESUME);
> +	worker_stage = STAGE_WAITING_EXIT;
> +	TST_CHECKPOINT_WAIT(WORKER_ALLOC2_DONE);

Here as well should be just TST_CHECKPOINT_WAKE_AND_WAIT(0);

> +	SAFE_CG_SCANF(group_b, "memory.current", "%ld", &after_alloc2);
> +	tst_res(TINFO, "group_b memory.current=%ld after second alloc (baseline=%ld)",
> +		after_alloc2, after_migrate);
> +
> +	TST_EXP_EXPR(after_alloc2 >= after_migrate + (long)ALLOC_SIZE2,
> +		     "group_b memory.current (%ld) increased by >= ALLOC_SIZE2 (%ld)",
> +		     after_alloc2, (long)ALLOC_SIZE2);
> +
> +done:
> +	if (worker_stage == STAGE_WAITING_RESUME) {
> +		TST_CHECKPOINT_WAKE(WORKER_RESUME);
> +		worker_stage = STAGE_WAITING_EXIT;
> +		TST_CHECKPOINT_WAIT(WORKER_ALLOC2_DONE);
> +	}
> +	if (worker_stage == STAGE_WAITING_EXIT) {
> +		TST_CHECKPOINT_WAKE(WORKER_EXIT);
> +		tst_reap_children();
> +		worker_pid = 0;
> +		worker_stage = STAGE_DONE;
> +	}

I would be way easier to SAFE_KILL() and SAFE_WAITPID() the worker
process. And the same in the test setup.

> +	group_a = tst_cg_group_rm(group_a);
> +	group_b = tst_cg_group_rm(group_b);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (worker_stage == STAGE_WAITING_RESUME) {
> +		TST_CHECKPOINT_WAKE(WORKER_RESUME);
> +		worker_stage = STAGE_WAITING_EXIT;
> +		TST_CHECKPOINT_WAIT(WORKER_ALLOC2_DONE);
> +	}
> +	if (worker_stage == STAGE_WAITING_EXIT) {
> +		TST_CHECKPOINT_WAKE(WORKER_EXIT);
> +		tst_reap_children();
> +	}
> +
> +	if (group_a)
> +		group_a = tst_cg_group_rm(group_a);
> +	if (group_b)
> +		group_b = tst_cg_group_rm(group_b);
> +}
> +
> +static struct tst_test test = {
> +	.test_all	= test_memcg_task_migration,
> +	.cleanup	= cleanup,
> +	.forks_child	= 1,
> +	.needs_root	= 1,
> +	.needs_checkpoints = 1,
> +	.needs_cgroup_ver  = TST_CG_V2,
> +	.needs_cgroup_ctrls = (const char *const []){ "memory", NULL },
> +	.min_mem_avail	= MIN_MEM_AVAIL,
> +};
> -- 
> 2.55.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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