Messages in this thread | | | Date | Thu, 28 Sep 2023 10:51:43 +0800 | Subject | Re: [PATCH bpf-next v3 4/7] bpf: Introduce css open-coded iterator kfuncs | From | Chuyi Zhou <> |
| |
Hello,
在 2023/9/28 07:24, Andrii Nakryiko 写道: > On Mon, Sep 25, 2023 at 3:56 AM Chuyi Zhou <zhouchuyi@bytedance.com> wrote: >> >> This Patch adds kfuncs bpf_iter_css_{new,next,destroy} which allow >> creation and manipulation of struct bpf_iter_css in open-coded iterator >> style. These kfuncs actually wrapps css_next_descendant_{pre, post}. >> css_iter can be used to: >> >> 1) iterating a sepcific cgroup tree with pre/post/up order >> >> 2) iterating cgroup_subsystem in BPF Prog, like >> for_each_mem_cgroup_tree/cpuset_for_each_descendant_pre in kernel. >> >> The API design is consistent with cgroup_iter. bpf_iter_css_new accepts >> parameters defining iteration order and starting css. Here we also reuse >> BPF_CGROUP_ITER_DESCENDANTS_PRE, BPF_CGROUP_ITER_DESCENDANTS_POST, >> BPF_CGROUP_ITER_ANCESTORS_UP enums. >> >> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> >> --- >> kernel/bpf/cgroup_iter.c | 57 +++++++++++++++++++ >> kernel/bpf/helpers.c | 3 + >> .../testing/selftests/bpf/bpf_experimental.h | 6 ++ >> 3 files changed, 66 insertions(+) >> >> diff --git a/kernel/bpf/cgroup_iter.c b/kernel/bpf/cgroup_iter.c >> index 810378f04fbc..ebc3d9471f52 100644 >> --- a/kernel/bpf/cgroup_iter.c >> +++ b/kernel/bpf/cgroup_iter.c >> @@ -294,3 +294,60 @@ static int __init bpf_cgroup_iter_init(void) >> } >> >> late_initcall(bpf_cgroup_iter_init); >> + >> +struct bpf_iter_css { >> + __u64 __opaque[2]; >> + __u32 __opaque_int[1]; >> +} __attribute__((aligned(8))); >> + > > same as before, __opaque[3] only > > >> +struct bpf_iter_css_kern { >> + struct cgroup_subsys_state *start; >> + struct cgroup_subsys_state *pos; >> + int order; >> +} __attribute__((aligned(8))); >> + >> +__bpf_kfunc int bpf_iter_css_new(struct bpf_iter_css *it, >> + struct cgroup_subsys_state *start, enum bpf_cgroup_iter_order order) > > Similarly, I wonder if we should go for a more generic "flags" argument? > >> +{ >> + struct bpf_iter_css_kern *kit = (void *)it; > > empty line > >> + kit->start = NULL; >> + BUILD_BUG_ON(sizeof(struct bpf_iter_css_kern) != sizeof(struct bpf_iter_css)); >> + BUILD_BUG_ON(__alignof__(struct bpf_iter_css_kern) != __alignof__(struct bpf_iter_css)); > > please move this up before kit->start assignment, and separate by empty lines > >> + switch (order) { >> + case BPF_CGROUP_ITER_DESCENDANTS_PRE: >> + case BPF_CGROUP_ITER_DESCENDANTS_POST: >> + case BPF_CGROUP_ITER_ANCESTORS_UP: >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + kit->start = start; >> + kit->pos = NULL; >> + kit->order = order; >> + return 0; >> +} >> + >> +__bpf_kfunc struct cgroup_subsys_state *bpf_iter_css_next(struct bpf_iter_css *it) >> +{ >> + struct bpf_iter_css_kern *kit = (void *)it; > > empty line > >> + if (!kit->start) >> + return NULL; >> + >> + switch (kit->order) { >> + case BPF_CGROUP_ITER_DESCENDANTS_PRE: >> + kit->pos = css_next_descendant_pre(kit->pos, kit->start); >> + break; >> + case BPF_CGROUP_ITER_DESCENDANTS_POST: >> + kit->pos = css_next_descendant_post(kit->pos, kit->start); >> + break; >> + default: > > we know it's BPF_CGROUP_ITER_ANCESTORS_UP, so why not have that here explicitly? > >> + kit->pos = kit->pos ? kit->pos->parent : kit->start; >> + } >> + >> + return kit->pos; > > wouldn't this implementation never return the "start" css? is that intentional? >
Thanks for the review.
This implementation actually would return the "start" css.
1. BPF_CGROUP_ITER_DESCENDANTS_PRE: 1.1 when we first call next(), css_next_descendant_pre(NULL, kit->start) will return kit->start. 1.2 second call next(), css_next_descendant_pre(kit->start, kit->start) would return a first valid child under kit->start with pre-order 1.3 third call next, css_next_descendant_pre(last_valid_child, kit->start) would return the next valid child ... util css_next_descendant_pre return a NULL pointer, which means we have visited all valid child including "start" css itself.
The above logic is equal to macro 'css_for_each_descendant_pre' in kernel.
Same, BPF_CGROUP_ITER_DESCENDANTS_POST is equal to macro 'css_for_each_descendant_post' which would return 'start' css when we have visited all valid child.
2. BPF_CGROUP_ITER_ANCESTORS_UP 2.1 when we fisrt call next(), kit->pos is NULL, and we would return kit->start.
The selftest in patch7 whould check: 1. when we use BPF_CGROUP_ITER_DESCENDANTS_PRE to iterate a cgroup tree, the first cgroup we visted should be root('start') cgroup. 2. when we use BPF_CGROUP_ITER_DESCENDANTS_POST to iterate a cgroup tree, the last cgroup we visited should be root('start') cgroup.
Am I miss something important?
Thanks.
| |