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

Richard Palethorpe
2023-03-16T11:44:27Z

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.