Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <27b57638-48db-7082-2b53-93d84e423350@bytedance.com>
Date:   Thu, 28 Sep 2023 10:51:43 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.1
Subject: Re: [PATCH bpf-next v3 4/7] bpf: Introduce css open-coded iterator
 kfuncs
To:     Andrii Nakryiko <andrii.nakryiko@gmail.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
References: <20230925105552.817513-1-zhouchuyi@bytedance.com>
 <20230925105552.817513-5-zhouchuyi@bytedance.com>
 <CAEf4BzbYgf1t8tfQJ4xwfDH-o_3n+PRMBgC4AZRLbXGM=QJtzQ@mail.gmail.com>
From:   Chuyi Zhou <zhouchuyi@bytedance.com>
In-Reply-To: <CAEf4BzbYgf1t8tfQJ4xwfDH-o_3n+PRMBgC4AZRLbXGM=QJtzQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.