| Message ID | 20230517120827.9350-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v5] clone3: Add clone3's clone_args cgroup | expand |
Hello, Wei Gao via ltp <ltp@lists.linux.it> writes: > Signed-off-by: Wei Gao <wegao@suse.com> > --- > include/lapi/sched.h | 8 ++ > include/tst_cgroup.h | 4 + > include/tst_clone.h | 1 + > lib/tst_cgroup.c | 9 ++ > lib/tst_clone.c | 1 + > runtest/syscalls | 1 + > testcases/kernel/syscalls/clone3/.gitignore | 1 + > testcases/kernel/syscalls/clone3/clone303.c | 101 ++++++++++++++++++++ > 8 files changed, 126 insertions(+) > create mode 100644 testcases/kernel/syscalls/clone3/clone303.c > > diff --git a/include/lapi/sched.h b/include/lapi/sched.h > index 1065665d1..ac766efc5 100644 > --- a/include/lapi/sched.h > +++ b/include/lapi/sched.h > @@ -13,6 +13,7 @@ > #include <inttypes.h> > #include "config.h" > #include "lapi/syscalls.h" > +#include "lapi/sched.h" > > struct sched_attr { > uint32_t size; > @@ -54,6 +55,9 @@ struct clone_args { > uint64_t __attribute__((aligned(8))) stack; > uint64_t __attribute__((aligned(8))) stack_size; > uint64_t __attribute__((aligned(8))) tls; > + uint64_t __attribute__((aligned(8))) set_tid; > + uint64_t __attribute__((aligned(8))) set_tid_size; > + uint64_t __attribute__((aligned(8))) cgroup; > }; > > static inline int clone3(struct clone_args *args, size_t size) > @@ -133,4 +137,8 @@ static inline int getcpu(unsigned *cpu, unsigned *node) > # define CLONE_NEWTIME 0x00000080 > #endif > > +#ifndef CLONE_INTO_CGROUP > +# define CLONE_INTO_CGROUP 0x200000000ULL > +#endif > + > #endif /* LAPI_SCHED_H__ */ > diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h > index 2826ddad1..be14d07c6 100644 > --- a/include/tst_cgroup.h > +++ b/include/tst_cgroup.h > @@ -157,6 +157,10 @@ const char * > tst_cg_group_name(const struct tst_cg_group *const cg) > __attribute__ ((nonnull, warn_unused_result)); > > +/* This call returns a fd pointing to a v2 directory */ > +int tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg) > + __attribute__ ((nonnull, warn_unused_result)); > + > /* Remove a descendant CGroup */ > struct tst_cg_group * > tst_cg_group_rm(struct tst_cg_group *const cg) > diff --git a/include/tst_clone.h b/include/tst_clone.h > index 9ffdc68d1..7b278dfa7 100644 > --- a/include/tst_clone.h > +++ b/include/tst_clone.h > @@ -11,6 +11,7 @@ > struct tst_clone_args { > uint64_t flags; > uint64_t exit_signal; > + uint64_t cgroup; This is not used in the test being added so I will not merge it because I don't want to do any more work than necessary (I would still merge the rest of the test, but there is another issue below). The reason is because it may cause some test which does use tst_clone_args to fail because it increases the struct size. If some other test does not initialise the members correctly we may start sending uninitialised data to the kernel. In general I don't want to add anything which isn't immediately necessary without having to think about any potential problems it could cause. > }; > > /* clone3 with fallbacks to clone when possible. Be aware that it > diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c > index 274c73fea..43055e8cf 100644 > --- a/lib/tst_cgroup.c > +++ b/lib/tst_cgroup.c > @@ -1112,6 +1112,15 @@ const char *tst_cg_group_name(const struct tst_cg_group *const cg) > return cg->group_name; > } > > +int tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg) > +{ > + for (int i = 0; cg->dirs[i]; i++) { > + if (cg->dirs[i]->dir_root->ver == TST_CG_V2) > + return cg->dirs[i]->dir_fd; The loop is unecessary; cg->dirs_by_ctrl[0] is always the V2 directory if it exists. Otherwise the test LGTM. I'll set to changes requested in patchwork.
Hi Richard Thanks for your review. I flag my comments in former email start with [GW]. Thanks. Regards Gao Wei -----Original Message----- From: Richard Palethorpe <rpalethorpe@suse.de> Sent: Friday, August 25, 2023 6:36 PM To: Wei Gao <wegao@suse.com> Cc: ltp@lists.linux.it Subject: Re: [LTP] [PATCH v5] clone3: Add clone3's clone_args cgroup Hello, Wei Gao via ltp <ltp@lists.linux.it> writes: > Signed-off-by: Wei Gao <wegao@suse.com> > --- > include/lapi/sched.h | 8 ++ > include/tst_cgroup.h | 4 + > include/tst_clone.h | 1 + > lib/tst_cgroup.c | 9 ++ > lib/tst_clone.c | 1 + > runtest/syscalls | 1 + > testcases/kernel/syscalls/clone3/.gitignore | 1 + > testcases/kernel/syscalls/clone3/clone303.c | 101 > ++++++++++++++++++++ > 8 files changed, 126 insertions(+) > create mode 100644 testcases/kernel/syscalls/clone3/clone303.c > > diff --git a/include/lapi/sched.h b/include/lapi/sched.h index > 1065665d1..ac766efc5 100644 > --- a/include/lapi/sched.h > +++ b/include/lapi/sched.h > @@ -13,6 +13,7 @@ > #include <inttypes.h> > #include "config.h" > #include "lapi/syscalls.h" > +#include "lapi/sched.h" > > struct sched_attr { > uint32_t size; > @@ -54,6 +55,9 @@ struct clone_args { > uint64_t __attribute__((aligned(8))) stack; > uint64_t __attribute__((aligned(8))) stack_size; > uint64_t __attribute__((aligned(8))) tls; > + uint64_t __attribute__((aligned(8))) set_tid; > + uint64_t __attribute__((aligned(8))) set_tid_size; > + uint64_t __attribute__((aligned(8))) cgroup; > }; > > static inline int clone3(struct clone_args *args, size_t size) @@ > -133,4 +137,8 @@ static inline int getcpu(unsigned *cpu, unsigned *node) > # define CLONE_NEWTIME 0x00000080 > #endif > > +#ifndef CLONE_INTO_CGROUP > +# define CLONE_INTO_CGROUP 0x200000000ULL #endif > + > #endif /* LAPI_SCHED_H__ */ > diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h index > 2826ddad1..be14d07c6 100644 > --- a/include/tst_cgroup.h > +++ b/include/tst_cgroup.h > @@ -157,6 +157,10 @@ const char * > tst_cg_group_name(const struct tst_cg_group *const cg) > __attribute__ ((nonnull, warn_unused_result)); > > +/* This call returns a fd pointing to a v2 directory */ int > +tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg) > + __attribute__ ((nonnull, warn_unused_result)); > + > /* Remove a descendant CGroup */ > struct tst_cg_group * > tst_cg_group_rm(struct tst_cg_group *const cg) diff --git > a/include/tst_clone.h b/include/tst_clone.h index 9ffdc68d1..7b278dfa7 > 100644 > --- a/include/tst_clone.h > +++ b/include/tst_clone.h > @@ -11,6 +11,7 @@ > struct tst_clone_args { > uint64_t flags; > uint64_t exit_signal; > + uint64_t cgroup; This is not used in the test being added so I will not merge it because I don't want to do any more work than necessary (I would still merge the rest of the test, but there is another issue below). The reason is because it may cause some test which does use tst_clone_args to fail because it increases the struct size. If some other test does not initialise the members correctly we may start sending uninitialised data to the kernel. In general I don't want to add anything which isn't immediately necessary without having to think about any potential problems it could cause. [GW]: The point of this case is test cgroup parameter, if you remove this then following error will happen: tst_clone.c:18:21: error: 'const struct tst_clone_args' has no member named 'cgroup' .cgroup = tst_args->cgroup, > }; > > /* clone3 with fallbacks to clone when possible. Be aware that it > diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c index > 274c73fea..43055e8cf 100644 > --- a/lib/tst_cgroup.c > +++ b/lib/tst_cgroup.c > @@ -1112,6 +1112,15 @@ const char *tst_cg_group_name(const struct tst_cg_group *const cg) > return cg->group_name; > } > > +int tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg) > +{ > + for (int i = 0; cg->dirs[i]; i++) { > + if (cg->dirs[i]->dir_root->ver == TST_CG_V2) > + return cg->dirs[i]->dir_fd; The loop is unecessary; cg->dirs_by_ctrl[0] is always the V2 directory if it exists. [GW]: I have updated and sent new Patch Otherwise the test LGTM. I'll set to changes requested in patchwork. -- Thank you, Richard.
Hello, Wei Gao <wegao@suse.com> writes: > Hi Richard > > Thanks for your review. I flag my comments in former email start with [GW]. > > Thanks. > Regards > Gao Wei > > > -----Original Message----- > From: Richard Palethorpe <rpalethorpe@suse.de> > Sent: Friday, August 25, 2023 6:36 PM > To: Wei Gao <wegao@suse.com> > Cc: ltp@lists.linux.it > Subject: Re: [LTP] [PATCH v5] clone3: Add clone3's clone_args cgroup > > Hello, > > Wei Gao via ltp <ltp@lists.linux.it> writes: > >> Signed-off-by: Wei Gao <wegao@suse.com> >> --- >> include/lapi/sched.h | 8 ++ >> include/tst_cgroup.h | 4 + >> include/tst_clone.h | 1 + >> lib/tst_cgroup.c | 9 ++ >> lib/tst_clone.c | 1 + >> runtest/syscalls | 1 + >> testcases/kernel/syscalls/clone3/.gitignore | 1 + >> testcases/kernel/syscalls/clone3/clone303.c | 101 >> ++++++++++++++++++++ >> 8 files changed, 126 insertions(+) >> create mode 100644 testcases/kernel/syscalls/clone3/clone303.c >> >> diff --git a/include/lapi/sched.h b/include/lapi/sched.h index >> 1065665d1..ac766efc5 100644 >> --- a/include/lapi/sched.h >> +++ b/include/lapi/sched.h >> @@ -13,6 +13,7 @@ >> #include <inttypes.h> >> #include "config.h" >> #include "lapi/syscalls.h" >> +#include "lapi/sched.h" >> >> struct sched_attr { >> uint32_t size; >> @@ -54,6 +55,9 @@ struct clone_args { >> uint64_t __attribute__((aligned(8))) stack; >> uint64_t __attribute__((aligned(8))) stack_size; >> uint64_t __attribute__((aligned(8))) tls; >> + uint64_t __attribute__((aligned(8))) set_tid; >> + uint64_t __attribute__((aligned(8))) set_tid_size; >> + uint64_t __attribute__((aligned(8))) cgroup; >> }; >> >> static inline int clone3(struct clone_args *args, size_t size) @@ >> -133,4 +137,8 @@ static inline int getcpu(unsigned *cpu, unsigned *node) >> # define CLONE_NEWTIME 0x00000080 >> #endif >> >> +#ifndef CLONE_INTO_CGROUP >> +# define CLONE_INTO_CGROUP 0x200000000ULL #endif >> + >> #endif /* LAPI_SCHED_H__ */ >> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h index >> 2826ddad1..be14d07c6 100644 >> --- a/include/tst_cgroup.h >> +++ b/include/tst_cgroup.h >> @@ -157,6 +157,10 @@ const char * >> tst_cg_group_name(const struct tst_cg_group *const cg) >> __attribute__ ((nonnull, warn_unused_result)); >> >> +/* This call returns a fd pointing to a v2 directory */ int >> +tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg) >> + __attribute__ ((nonnull, warn_unused_result)); >> + >> /* Remove a descendant CGroup */ >> struct tst_cg_group * >> tst_cg_group_rm(struct tst_cg_group *const cg) diff --git >> a/include/tst_clone.h b/include/tst_clone.h index 9ffdc68d1..7b278dfa7 >> 100644 >> --- a/include/tst_clone.h >> +++ b/include/tst_clone.h >> @@ -11,6 +11,7 @@ >> struct tst_clone_args { >> uint64_t flags; >> uint64_t exit_signal; >> + uint64_t cgroup; > > This is not used in the test being added so I will not merge it because I don't want to do any more work than necessary (I would still merge the rest of the test, but there is another issue below). The reason is because it may cause some test which does use tst_clone_args to fail because it increases the struct size. If some other test does not initialise the members correctly we may start sending uninitialised data to the kernel. > > In general I don't want to add anything which isn't immediately necessary without having to think about any potential problems it could cause. > > [GW]: The point of this case is test cgroup parameter, if you remove this then following error will happen: > tst_clone.c:18:21: error: 'const struct tst_clone_args' has no member named 'cgroup' > .cgroup = tst_args->cgroup, But you use clone3 directly, I don't understand where tst_clone is being used in the test? > > >> }; >> >> /* clone3 with fallbacks to clone when possible. Be aware that it >> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c index >> 274c73fea..43055e8cf 100644 >> --- a/lib/tst_cgroup.c >> +++ b/lib/tst_cgroup.c >> @@ -1112,6 +1112,15 @@ const char *tst_cg_group_name(const struct tst_cg_group *const cg) >> return cg->group_name; >> } >> >> +int tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg) >> +{ >> + for (int i = 0; cg->dirs[i]; i++) { >> + if (cg->dirs[i]->dir_root->ver == TST_CG_V2) >> + return cg->dirs[i]->dir_fd; > > The loop is unecessary; cg->dirs_by_ctrl[0] is always the V2 directory if it exists. > > [GW]: I have updated and sent new Patch > > Otherwise the test LGTM. I'll set to changes requested in patchwork.
diff --git a/include/lapi/sched.h b/include/lapi/sched.h index 1065665d1..ac766efc5 100644 --- a/include/lapi/sched.h +++ b/include/lapi/sched.h @@ -13,6 +13,7 @@ #include <inttypes.h> #include "config.h" #include "lapi/syscalls.h" +#include "lapi/sched.h" struct sched_attr { uint32_t size; @@ -54,6 +55,9 @@ struct clone_args { uint64_t __attribute__((aligned(8))) stack; uint64_t __attribute__((aligned(8))) stack_size; uint64_t __attribute__((aligned(8))) tls; + uint64_t __attribute__((aligned(8))) set_tid; + uint64_t __attribute__((aligned(8))) set_tid_size; + uint64_t __attribute__((aligned(8))) cgroup; }; static inline int clone3(struct clone_args *args, size_t size) @@ -133,4 +137,8 @@ static inline int getcpu(unsigned *cpu, unsigned *node) # define CLONE_NEWTIME 0x00000080 #endif +#ifndef CLONE_INTO_CGROUP +# define CLONE_INTO_CGROUP 0x200000000ULL +#endif + #endif /* LAPI_SCHED_H__ */ diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h index 2826ddad1..be14d07c6 100644 --- a/include/tst_cgroup.h +++ b/include/tst_cgroup.h @@ -157,6 +157,10 @@ const char * tst_cg_group_name(const struct tst_cg_group *const cg) __attribute__ ((nonnull, warn_unused_result)); +/* This call returns a fd pointing to a v2 directory */ +int tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg) + __attribute__ ((nonnull, warn_unused_result)); + /* Remove a descendant CGroup */ struct tst_cg_group * tst_cg_group_rm(struct tst_cg_group *const cg) diff --git a/include/tst_clone.h b/include/tst_clone.h index 9ffdc68d1..7b278dfa7 100644 --- a/include/tst_clone.h +++ b/include/tst_clone.h @@ -11,6 +11,7 @@ struct tst_clone_args { uint64_t flags; uint64_t exit_signal; + uint64_t cgroup; }; /* clone3 with fallbacks to clone when possible. Be aware that it diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c index 274c73fea..43055e8cf 100644 --- a/lib/tst_cgroup.c +++ b/lib/tst_cgroup.c @@ -1112,6 +1112,15 @@ const char *tst_cg_group_name(const struct tst_cg_group *const cg) return cg->group_name; } +int tst_cg_group_unified_dir_fd(const struct tst_cg_group *const cg) +{ + for (int i = 0; cg->dirs[i]; i++) { + if (cg->dirs[i]->dir_root->ver == TST_CG_V2) + return cg->dirs[i]->dir_fd; + } + return -1; +} + struct tst_cg_group *tst_cg_group_rm(struct tst_cg_group *const cg) { struct cgroup_dir **dir; diff --git a/lib/tst_clone.c b/lib/tst_clone.c index ecc84408c..2aa00beb1 100644 --- a/lib/tst_clone.c +++ b/lib/tst_clone.c @@ -15,6 +15,7 @@ pid_t tst_clone(const struct tst_clone_args *tst_args) struct clone_args args = { .flags = tst_args->flags, .exit_signal = tst_args->exit_signal, + .cgroup = tst_args->cgroup, }; int flags; pid_t pid = -1; diff --git a/runtest/syscalls b/runtest/syscalls index 9c23a4248..0b6adfd7f 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -117,6 +117,7 @@ clone09 clone09 clone301 clone301 clone302 clone302 +clone303 clone303 close01 close01 close02 close02 diff --git a/testcases/kernel/syscalls/clone3/.gitignore b/testcases/kernel/syscalls/clone3/.gitignore index 604cb903e..10369954b 100644 --- a/testcases/kernel/syscalls/clone3/.gitignore +++ b/testcases/kernel/syscalls/clone3/.gitignore @@ -1,2 +1,3 @@ clone301 clone302 +clone303 diff --git a/testcases/kernel/syscalls/clone3/clone303.c b/testcases/kernel/syscalls/clone3/clone303.c new file mode 100644 index 000000000..5bf02edef --- /dev/null +++ b/testcases/kernel/syscalls/clone3/clone303.c @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2023 SUSE LLC <wegao@suse.com> + */ + +/*\ + * [Description] + * + * This test case check clone3 CLONE_INTO_CGROUP flag + * + */ + +#define _GNU_SOURCE +#include <stdio.h> +#include <stdlib.h> +#include <sys/wait.h> + +#include "tst_test.h" +#include "lapi/sched.h" +#include "lapi/pidfd.h" + +#define BUF_LEN 20 + +static struct tst_cg_group *cg_child_test_simple; +static int fd; +static struct clone_args *args; + +static pid_t clone_into_cgroup(int cgroup_fd) +{ + pid_t pid; + + args->flags = CLONE_INTO_CGROUP; + args->exit_signal = SIGCHLD; + args->cgroup = cgroup_fd; + + pid = clone3(args, sizeof(*args)); + + if (pid < 0) + tst_res(TFAIL | TTERRNO, "clone3() failed !"); + + return pid; +} + +static void run(void) +{ + pid_t pid; + + pid = clone_into_cgroup(fd); + + if (!pid) { + TST_CHECKPOINT_WAIT(0); + return; + } + + char buf[BUF_LEN]; + + SAFE_CG_READ(cg_child_test_simple, "cgroup.procs", buf, BUF_LEN); + + if (atoi(buf) == pid) + tst_res(TPASS, "clone3 case pass!"); + else + tst_brk(TFAIL | TTERRNO, "clone3() failed !"); + + TST_CHECKPOINT_WAKE(0); + + SAFE_WAITPID(pid, NULL, 0); + +} + +static void setup(void) +{ + clone3_supported_by_kernel(); + + cg_child_test_simple = tst_cg_group_mk(tst_cg, "cg_test_simple"); + + fd = tst_cg_group_unified_dir_fd(cg_child_test_simple); + + if (fd < 0) + tst_brk(TBROK, "get dir fd failed!"); +} + +static void cleanup(void) +{ + cg_child_test_simple = tst_cg_group_rm(cg_child_test_simple); +} + +static struct tst_test test = { + .test_all = run, + .setup = setup, + .cleanup = cleanup, + .forks_child = 1, + .max_runtime = 20, + .needs_cgroup_ctrls = (const char *const []){ "base", NULL }, + .needs_cgroup_ver = TST_CG_V2, + .needs_checkpoints = 1, + .min_kver = "5.7", + .bufs = (struct tst_buffers []) { + {&args, .size = sizeof(*args)}, + {}, + } +};
Signed-off-by: Wei Gao <wegao@suse.com> --- include/lapi/sched.h | 8 ++ include/tst_cgroup.h | 4 + include/tst_clone.h | 1 + lib/tst_cgroup.c | 9 ++ lib/tst_clone.c | 1 + runtest/syscalls | 1 + testcases/kernel/syscalls/clone3/.gitignore | 1 + testcases/kernel/syscalls/clone3/clone303.c | 101 ++++++++++++++++++++ 8 files changed, 126 insertions(+) create mode 100644 testcases/kernel/syscalls/clone3/clone303.c