Re: [LTP] [PATCH v5] kill01: New case cgroup kill

[Wei Gao via ltp <ltp@lists.linux.it>]

On Mon, Mar 13, 2023 at 10:45:12AM +0000, Richard Palethorpe wrote:
> Hello,
> 
> > +static int wait_for_pid(pid_t pid)
> > +{
> > +	int status, ret;
> > +
> > +again:
> > +	ret = waitpid(pid, &status, 0);
> > +	if (ret == -1) {
> > +		if (errno == EINTR)
> > +			goto again;
> > +
> > +		return -1;
> > +	}
> > +
> > +	if (!WIFEXITED(status))
> > +		return -1;
> > +
> > +	return WEXITSTATUS(status);
> > +}
> 
> We have tst_reap_children for this, but this just appears to be wrong
> for this test.
tst_reap_children can not return reason of status, such as i need call 
WIFSIGNALED(wstatus) in next patch to make sure children is killed by 
signal.

> > + * A simple process running in a sleep loop until being
> > + * re-parented.
> > + */
> > +static void child_fn(void)
> > +{
> > +	int ppid = getppid();
> > +
> > +	while (getppid() == ppid)
> > +		usleep(1000);
> > +
> > +}
> > +
> > +static int cg_run_nowait(const struct tst_cg_group *const cg,
> > +		  void (*fn)(void))
> 
> Why keep this function?
> 
> If you want to convert tests to LTP, then don't do the minimum possible
> to use the LTP API. Use as much of it as possible otherwise we are just
> importing brittle self tests.
> 
function is useful and wrap the fork action & put pid into cgroup.procs,
is there any LTP API can replace this function? Could you help give example.
> > +{
> > +	int pid;
> > +
> > +	pid = SAFE_FORK();
> > +	if (pid == 0) {
> > +		SAFE_CG_PRINTF(cg, "cgroup.procs", "%d", getpid());
> > +		fn();
> > +	}
> > +
> > +	return pid;
> > +}
> > +
> > +static int cg_wait_for_proc_count(const struct tst_cg_group *cg, int count)
> > +{
> > +	int attempts;
> > +	char *ptr;
> > +
> > +	for (attempts = 100; attempts >= 0; attempts--) {
> > +		int nr = 0;
> > +
> > +		SAFE_CG_READ(cg, "cgroup.procs", buf, buf_len);
> > +
> > +		for (ptr = buf; *ptr; ptr++)
> > +			if (*ptr == '\n')
> > +				nr++;
> > +
> > +		if (nr >= count)
> > +			return 0;
> > +
> > +		usleep(100000);
> 
> It's best to avoid arbitrary sleep values and attempts. You could use
> TST_CHECKPOINT* or increment a counter in some shared memory with
> SAFE_MMAP and tst_atomic_inc.
> 
I will try to use TST_CHECKPOINT* to sync before call this function
> > +	}
> > +
> > +	return -1;
> > +}
> > +
> > +static void run(void)
> > +{
> > +	pid_t pids[MAX_PID_NUM];
> > +	int i;
> > +
> > +	cg_child_test_simple = tst_cg_group_mk(tst_cg,
> > "cg_test_simple");
> > +
> > +	memset(buf, 0, buf_len);
> 
> IIRC guarded buffers are zeroed already.
Already explained by Li Wang in other email.
> 
> > +
> > +	for (i = 0; i < pid_num; i++)
> > +		pids[i] = cg_run_nowait(cg_child_test_simple, child_fn);
> 
> If the parent is killed and the children are moved then they will return
> and cause a fork bomb.
There is no extra fork action in child_fn so all child_fn will reparent and exit.
So i do not think fork bomb will happen.
> 
> This is not obvious because of the unecessary indirection (function
> pointer and functions).
> 
> > +
> > +	TST_EXP_PASS(cg_wait_for_proc_count(cg_child_test_simple,
> > pid_num));
> 
> If this fails then there will be little information to debug it. This is
> a common issue with the self tests which we will be importing into the LTP.
> 
Add extra log info into this function maybe help, what's your suggestion?
> > +	SAFE_CG_PRINTF(cg_child_test_simple, "cgroup.kill", "%d", 1);
> > +
> > +	for (i = 0; i < pid_num; i++) {
> > +		/* wait_for_pid(pids[i]); */
> > +		TST_EXP_PASS_SILENT(wait_for_pid(pids[i]) == SIGKILL);
> 
> It seems wait_for_pid will never == SIGKILL. The function does not
> inspect the signal a process was killed with at all.
> 
> The test passes becaues this is not the correct use of TST_EXP_PASS*.
Good catch! Thanks a lot for finding this, i should use WIFSIGNALED 
to translate status and check children killed by SIGKILL, will fix this
in next patch.
> 
> > +	}
> > +
> > +	cg_child_test_simple = tst_cg_group_rm(cg_child_test_simple);
> > +}
> > +
> > +static void setup(void)
> > +{
> > +	buf = tst_alloc(buf_len);
> 
> Simple allocations like this can be done in the test struct.
This already discussed with Wang Li, compile error will happen since buf_len 
not fixed in my case.
> 
> > +}
> > +
> > +static struct tst_test test = {
> > +	.test_all = run,
> > +	.setup = setup,
> > +	.forks_child = 1,
> > +	.max_runtime = 15,
> > +	.needs_cgroup_ctrls = (const char *const []){ "memory", NULL },
> 
> Why do we need the memory controller?
> 
> If it is just to make the LTP library happy, then you can change the
> library instead (e.g. add a "cgroup" pseudo controller if we didn't do
> that already).
You guess right, i just go quick way to let LTP happy xD
I will check library and try to implement this.

Thanks again for your valuable feedback!
> 
> > +	.needs_cgroup_ver = TST_CG_V2,
> > +};
> > -- 
> > 2.35.3
> 
> -- 
> Thank you,
> Richard.

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