| Message ID | 20220510001807.4132027-7-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: cgroup hierarchical stats collection | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | merge-conflict |
| netdev/tree_selection | success | Clearly marked for bpf-next, async |
| netdev/apply | fail | Patch does not apply to bpf-next |
On Tue, May 10, 2022 at 12:18:04AM +0000, Yosry Ahmed wrote: > The current implementation of cgroup_get_from_id() only searches the > default hierarchy for the given id. Make it compatible with cgroup v1 by > looking through all the roots instead. > > cgrp_dfl_root should be the first element in the list so there shouldn't > be a performance impact for cgroup v2 users (in the case of a valid id). > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > kernel/cgroup/cgroup.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c > index af703cfcb9d2..12700cd21973 100644 > --- a/kernel/cgroup/cgroup.c > +++ b/kernel/cgroup/cgroup.c > @@ -5970,10 +5970,16 @@ void cgroup_path_from_kernfs_id(u64 id, char *buf, size_t buflen) > */ > struct cgroup *cgroup_get_from_id(u64 id) > { > - struct kernfs_node *kn; > + struct kernfs_node *kn = NULL; > struct cgroup *cgrp = NULL; > + struct cgroup_root *root; > + > + for_each_root(root) { > + kn = kernfs_find_and_get_node_by_id(root->kf_root, id); > + if (kn) > + break; > + } I can't see how this can work. You're smashing together separate namespaces and the same IDs can exist across multiple of these hierarchies. You'd need a bigger surgery to make this work for cgroup1 which would prolly involve complications around 32bit ino's and file handle support too, which I'm not likely to ack, so please give it up on adding these things to cgroup1. Nacked-by: Tejun Heo <tj@kernel.org> Thanks.
On Tue, May 10, 2022 at 11:34 AM Tejun Heo <tj@kernel.org> wrote: > > On Tue, May 10, 2022 at 12:18:04AM +0000, Yosry Ahmed wrote: > > The current implementation of cgroup_get_from_id() only searches the > > default hierarchy for the given id. Make it compatible with cgroup v1 by > > looking through all the roots instead. > > > > cgrp_dfl_root should be the first element in the list so there shouldn't > > be a performance impact for cgroup v2 users (in the case of a valid id). > > > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > > --- > > kernel/cgroup/cgroup.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c > > index af703cfcb9d2..12700cd21973 100644 > > --- a/kernel/cgroup/cgroup.c > > +++ b/kernel/cgroup/cgroup.c > > @@ -5970,10 +5970,16 @@ void cgroup_path_from_kernfs_id(u64 id, char *buf, size_t buflen) > > */ > > struct cgroup *cgroup_get_from_id(u64 id) > > { > > - struct kernfs_node *kn; > > + struct kernfs_node *kn = NULL; > > struct cgroup *cgrp = NULL; > > + struct cgroup_root *root; > > + > > + for_each_root(root) { > > + kn = kernfs_find_and_get_node_by_id(root->kf_root, id); > > + if (kn) > > + break; > > + } > > I can't see how this can work. You're smashing together separate namespaces > and the same IDs can exist across multiple of these hierarchies. You'd need > a bigger surgery to make this work for cgroup1 which would prolly involve > complications around 32bit ino's and file handle support too, which I'm not > likely to ack, so please give it up on adding these things to cgroup1. > > Nacked-by: Tejun Heo <tj@kernel.org> > > Thanks. Completely understandable. I sent this patch knowing that it likely will not be accepted, with hopes of hearing feedback on whether this can be done in a simple way or not. Looks like I got my answer, so thanks for the info! Will drop this patch in the incoming versions. > > -- > tejun
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index af703cfcb9d2..12700cd21973 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -5970,10 +5970,16 @@ void cgroup_path_from_kernfs_id(u64 id, char *buf, size_t buflen) */ struct cgroup *cgroup_get_from_id(u64 id) { - struct kernfs_node *kn; + struct kernfs_node *kn = NULL; struct cgroup *cgrp = NULL; + struct cgroup_root *root; + + for_each_root(root) { + kn = kernfs_find_and_get_node_by_id(root->kf_root, id); + if (kn) + break; + } - kn = kernfs_find_and_get_node_by_id(cgrp_dfl_root.kf_root, id); if (!kn) goto out;
The current implementation of cgroup_get_from_id() only searches the default hierarchy for the given id. Make it compatible with cgroup v1 by looking through all the roots instead. cgrp_dfl_root should be the first element in the list so there shouldn't be a performance impact for cgroup v2 users (in the case of a valid id). Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- kernel/cgroup/cgroup.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)