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

Wei Gao via ltp
2023-03-15T12:55:50Z

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.