Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20230925105552.817513-1-zhouchuyi@bytedance.com>
 <20230925105552.817513-5-zhouchuyi@bytedance.com> <CAEf4BzbYgf1t8tfQJ4xwfDH-o_3n+PRMBgC4AZRLbXGM=QJtzQ@mail.gmail.com>
 <27b57638-48db-7082-2b53-93d84e423350@bytedance.com>
In-Reply-To: <27b57638-48db-7082-2b53-93d84e423350@bytedance.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 29 Sep 2023 14:29:08 -0700
Message-ID: <CAEf4Bza68mRn0KpOX2k7PtbXvO-uYzKHhQ=C8J+zyNS6WTFPpA@mail.gmail.com>
Subject: Re: [PATCH bpf-next v3 4/7] bpf: Introduce css open-coded iterator kfuncs
To:     Chuyi Zhou <zhouchuyi@bytedance.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, martin.lau@kernel.org, tj@kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Sep 27, 2023 at 7:51=E2=80=AFPM Chuyi Zhou <zhouchuyi@bytedance.com=
> wrote:
>
> Hello,
>
> =E5=9C=A8 2023/9/28 07:24, Andrii Nakryiko =E5=86=99=E9=81=93:
> > On Mon, Sep 25, 2023 at 3:56=E2=80=AFAM 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 iterato=
r
> >> 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 accept=
s
> >> parameters defining iteration order and starting css. Here we also reu=
se
> >> 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_ite=
r_order order)
> >
> > Similarly, I wonder if we should go for a more generic "flags" argument=
?
> >
> >> +{
> >> +       struct bpf_iter_css_kern *kit =3D (void *)it;
> >
> > empty line
> >
> >> +       kit->start =3D NULL;
> >> +       BUILD_BUG_ON(sizeof(struct bpf_iter_css_kern) !=3D sizeof(stru=
ct bpf_iter_css));
> >> +       BUILD_BUG_ON(__alignof__(struct bpf_iter_css_kern) !=3D __alig=
nof__(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 =3D start;
> >> +       kit->pos =3D NULL;
> >> +       kit->order =3D order;
> >> +       return 0;
> >> +}
> >> +
> >> +__bpf_kfunc struct cgroup_subsys_state *bpf_iter_css_next(struct bpf_=
iter_css *it)
> >> +{
> >> +       struct bpf_iter_css_kern *kit =3D (void *)it;
> >
> > empty line
> >
> >> +       if (!kit->start)
> >> +               return NULL;
> >> +
> >> +       switch (kit->order) {
> >> +       case BPF_CGROUP_ITER_DESCENDANTS_PRE:
> >> +               kit->pos =3D css_next_descendant_pre(kit->pos, kit->st=
art);
> >> +               break;
> >> +       case BPF_CGROUP_ITER_DESCENDANTS_POST:
> >> +               kit->pos =3D css_next_descendant_post(kit->pos, kit->s=
tart);
> >> +               break;
> >> +       default:
> >
> > we know it's BPF_CGROUP_ITER_ANCESTORS_UP, so why not have that here ex=
plicitly?
> >
> >> +               kit->pos =3D kit->pos ? kit->pos->parent : kit->start;
> >> +       }
> >> +
> >> +       return kit->pos;
> >
> > wouldn't this implementation never return the "start" css? is that inte=
ntional?
> >
>
> 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?
>

No, again, my bad, I didn't trace the logic completely before asking.
All makes sense with kit->pos being initialized to NULL. Thanks for
elaborating!

>
> Thanks.
>
>
>