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

[Richard Palethorpe <rpalethorpe@suse.de>]

Wei Gao <wegao@suse.com> writes:

> 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.

IMO you don't need to wrap fork or SAFE_CG_PRINTF, just write it inline.

>> > +{
>> > +	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.

I don't see how it will exit? It seems like it will just return.

Possibly I just missed it, but that's why I have an issue with the
function pointer as well.

> 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?

Yes, something like that.

>> > +	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!

Thanks!

>> 
>> > +	.needs_cgroup_ver = TST_CG_V2,
>> > +};
>> > -- 
>> > 2.35.3
>> 
>> -- 
>> Thank you,
>> Richard.


-- 
Thank you,
Richard.

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