| Message ID | 20221010235845.3379019-3-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix cgroup1 support in get from fd/file interfaces | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | pending | Logs for ${{ matrix.test }} on ${{ matrix.arch }} with ${{ matrix.toolchain }} |
| bpf/vmtest-bpf-next-VM_Test-2 | fail | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for set-matrix |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | fail | Errors and warnings before: 12414 this patch: 12417 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 3328 this patch: 3331 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 13101 this patch: 13104 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 91 lines checked |
| netdev/kdoc | fail | Errors and warnings before: 3 this patch: 6 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Oct 10, 2022 at 11:58:44PM +0000, Yosry Ahmed wrote: > Add cgroup_all_get_from_fd() and cgroup_all_get_from_file() that > support both cgroup1 and cgroup2. Looks generally good. How about cgroup_v1v2_ as the prefix? Thanks.
On Mon, Oct 10, 2022 at 5:11 PM Tejun Heo <tj@kernel.org> wrote: > > On Mon, Oct 10, 2022 at 11:58:44PM +0000, Yosry Ahmed wrote: > > Add cgroup_all_get_from_fd() and cgroup_all_get_from_file() that > > support both cgroup1 and cgroup2. > > Looks generally good. How about cgroup_v1v2_ as the prefix? Thanks for taking a look. I don't have a strong opinion here. I picked cgroup_all_* vs cgroup12_* or cgroup_v1v2_* because it's easier on the eyes. My preference would have been to rename the old versions to cgroup_dfl_* instead, but like you said this might confuse existing users. Anyway, I am fine with whatever you choose. Let me know if you need me to send a v2 for changing the prefix. > > Thanks. > > -- > tejun
On Mon, Oct 10, 2022 at 05:14:24PM -0700, Yosry Ahmed wrote: > On Mon, Oct 10, 2022 at 5:11 PM Tejun Heo <tj@kernel.org> wrote: > > > > On Mon, Oct 10, 2022 at 11:58:44PM +0000, Yosry Ahmed wrote: > > > Add cgroup_all_get_from_fd() and cgroup_all_get_from_file() that > > > support both cgroup1 and cgroup2. > > > > Looks generally good. How about cgroup_v1v2_ as the prefix? > > Thanks for taking a look. > > I don't have a strong opinion here. I picked cgroup_all_* vs > cgroup12_* or cgroup_v1v2_* because it's easier on the eyes. I'd rather have something which is really distinctive because the difference is rather subtle. > My preference would have been to rename the old versions to > cgroup_dfl_* instead, but like you said this might confuse existing > users. Yeah, it's confusing to have parallel file / id lookup functions with different behaviors and we've asssumed that all these from-userspace lookups are cgroup2 only for a while now. > Anyway, I am fine with whatever you choose. Let me know if you need me > to send a v2 for changing the prefix. Please send an updated version. I don't mind whether it's cgroup12_ or cgroup_v1v2_. Thanks.
Hi Yosry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
[also build test WARNING on linus/master next-20221010]
[cannot apply to tj-cgroup/for-next bpf/master v6.0]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/Fix-cgroup1-support-in-get-from-fd-file-interfaces/20221011-080002
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: um-i386_defconfig
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/38847864c4501bc463dedd2ffd6119185efb5d42
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yosry-Ahmed/Fix-cgroup1-support-in-get-from-fd-file-interfaces/20221011-080002
git checkout 38847864c4501bc463dedd2ffd6119185efb5d42
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash kernel/cgroup/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
kernel/cgroup/cgroup.c:6184: warning: Function parameter or member 'f' not described in 'cgroup_get_from_file'
>> kernel/cgroup/cgroup.c:6696: warning: expecting prototype for cgroup_get_from_fd(). Prototype was for cgroup_all_get_from_fd() instead
kernel/cgroup/cgroup.c:6714: warning: Function parameter or member 'fd' not described in 'cgroup_get_from_fd'
vim +6696 kernel/cgroup/cgroup.c
16af439645455f kernel/cgroup.c Tejun Heo 2015-11-20 6685
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6686 /**
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6687 * cgroup_get_from_fd - get a cgroup pointer from a fd
38847864c4501b kernel/cgroup/cgroup.c Yosry Ahmed 2022-10-10 6688 * @fd: fd obtained by open(cgroup_dir)
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6689 *
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6690 * Find the cgroup from a fd which should be obtained
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6691 * by opening a cgroup directory. Returns a pointer to the
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6692 * cgroup on success. ERR_PTR is returned if the cgroup
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6693 * cannot be found.
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6694 */
38847864c4501b kernel/cgroup/cgroup.c Yosry Ahmed 2022-10-10 6695 struct cgroup *cgroup_all_get_from_fd(int fd)
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 @6696 {
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6697 struct cgroup *cgrp;
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6698 struct file *f;
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6699
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6700 f = fget_raw(fd);
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6701 if (!f)
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6702 return ERR_PTR(-EBADF);
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6703
38847864c4501b kernel/cgroup/cgroup.c Yosry Ahmed 2022-10-10 6704 cgrp = cgroup_all_get_from_file(f);
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6705 fput(f);
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6706 return cgrp;
1f3fe7ebf6136c kernel/cgroup.c Martin KaFai Lau 2016-06-30 6707 }
38847864c4501b kernel/cgroup/cgroup.c Yosry Ahmed 2022-10-10 6708
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index 398f0bce7c21..cd847f4f47ed 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -106,6 +106,7 @@ struct cgroup_subsys_state *css_tryget_online_from_dir(struct dentry *dentry, struct cgroup *cgroup_get_from_path(const char *path); struct cgroup *cgroup_get_from_fd(int fd); +struct cgroup *cgroup_all_get_from_fd(int fd); int cgroup_attach_task_all(struct task_struct *from, struct task_struct *); int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from); diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index 72e97422e9d9..c3bd6f17246a 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -6208,16 +6208,36 @@ void cgroup_fork(struct task_struct *child) INIT_LIST_HEAD(&child->cg_list); } -static struct cgroup *cgroup_get_from_file(struct file *f) +/** + * cgroup_all_get_from_file - get a cgroup pointer from a file pointer + * @f: file corresponding to cgroup_dir + * + * Find the cgroup from a file pointer associated with a cgroup directory. + * Returns a pointer to the cgroup on success. ERR_PTR is returned if the + * cgroup cannot be found. + */ +static struct cgroup *cgroup_all_get_from_file(struct file *f) { struct cgroup_subsys_state *css; - struct cgroup *cgrp; css = css_tryget_online_from_dir(f->f_path.dentry, NULL); if (IS_ERR(css)) return ERR_CAST(css); - cgrp = css->cgroup; + return css->cgroup; +} + +/** + * cgroup_get_from_file - same as cgroup_all_get_from_file, but only supports + * cgroup2. + */ +static struct cgroup *cgroup_get_from_file(struct file *f) +{ + struct cgroup *cgrp = cgroup_all_get_from_file(f); + + if (IS_ERR(cgrp)) + return ERR_CAST(cgrp); + if (!cgroup_on_dfl(cgrp)) { cgroup_put(cgrp); return ERR_PTR(-EBADF); @@ -6720,14 +6740,14 @@ EXPORT_SYMBOL_GPL(cgroup_get_from_path); /** * cgroup_get_from_fd - get a cgroup pointer from a fd - * @fd: fd obtained by open(cgroup2_dir) + * @fd: fd obtained by open(cgroup_dir) * * Find the cgroup from a fd which should be obtained * by opening a cgroup directory. Returns a pointer to the * cgroup on success. ERR_PTR is returned if the cgroup * cannot be found. */ -struct cgroup *cgroup_get_from_fd(int fd) +struct cgroup *cgroup_all_get_from_fd(int fd) { struct cgroup *cgrp; struct file *f; @@ -6736,10 +6756,28 @@ struct cgroup *cgroup_get_from_fd(int fd) if (!f) return ERR_PTR(-EBADF); - cgrp = cgroup_get_from_file(f); + cgrp = cgroup_all_get_from_file(f); fput(f); return cgrp; } + +/** + * cgroup_get_from_fd - same as cgroup_all_get_from_fd, but only supports + * cgroup2. + */ +struct cgroup *cgroup_get_from_fd(int fd) +{ + struct cgroup *cgrp = cgroup_all_get_from_fd(fd); + + if (IS_ERR(cgrp)) + return ERR_CAST(cgrp); + + if (!cgroup_on_dfl(cgrp)) { + cgroup_put(cgrp); + return ERR_PTR(-EBADF); + } + return cgrp; +} EXPORT_SYMBOL_GPL(cgroup_get_from_fd); static u64 power_of_ten(int power)
Add cgroup_all_get_from_fd() and cgroup_all_get_from_file() that support both cgroup1 and cgroup2. Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- include/linux/cgroup.h | 1 + kernel/cgroup/cgroup.c | 50 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 45 insertions(+), 6 deletions(-)