On Wed, Aug 10, 2022 at 8:10 PM Hao Luo [off-list ref] wrote:

On Tue, Aug 9, 2022 at 11:38 AM Hao Luo [off-list ref] wrote:

quoted

On Tue, Aug 9, 2022 at 9:23 AM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Mon, Aug 08, 2022 at 05:56:57PM -0700, Hao Luo wrote:

quoted

On Mon, Aug 8, 2022 at 5:19 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Fri, Aug 5, 2022 at 2:49 PM Hao Luo [off-list ref] wrote:

quoted

Cgroup_iter is a type of bpf_iter. It walks over cgroups in four modes:

 - walking a cgroup's descendants in pre-order.
 - walking a cgroup's descendants in post-order.
 - walking a cgroup's ancestors.
 - process only the given cgroup.

[...]

quoted

quoted

quoted

quoted

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 59a217ca2dfd..4d758b2e70d6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -87,10 +87,37 @@ struct bpf_cgroup_storage_key {
        __u32   attach_type;            /* program attach type (enum bpf_attach_type) */
 };

+enum bpf_iter_order {
+       BPF_ITER_ORDER_DEFAULT = 0,     /* default order. */

why is this default order necessary? It just adds confusion (I had to
look up source code to know what is default order). I might have
missed some discussion, so if there is some very good reason, then
please document this in commit message. But I'd rather not do some
magical default order instead. We can set 0 to mean invalid and error
out, or just do SELF as the very first value (and if user forgot to
specify more fancy mode, they hopefully will quickly discover this in
their testing).

PRE/POST/UP are tree-specific orders. SELF applies on all iters and
yields only a single object. How does task_iter express a non-self
order? By non-self, I mean something like "I don't care about the
order, just scan _all_ the objects". And this "don't care" order, IMO,
may be the common case. I don't think everyone cares about walking
order for tasks. The DEFAULT is intentionally put at the first value,
so that if users don't care about order, they don't have to specify
this field.

If that sounds valid, maybe using "UNSPEC" instead of "DEFAULT" is better?

I agree with Andrii.
This:
+       if (order == BPF_ITER_ORDER_DEFAULT)
+               order = BPF_ITER_DESCENDANTS_PRE;

looks like an arbitrary choice.
imo
BPF_ITER_DESCENDANTS_PRE = 0,
would have been more obvious. No need to dig into definition of "default".

UNSPEC = 0
is fine too if we want user to always be conscious about the order
and the kernel will error if that field is not initialized.
That would be my preference, since it will match the rest of uapi/bpf.h

Sounds good. In the next version, will use

enum bpf_iter_order {
        BPF_ITER_ORDER_UNSPEC = 0,
        BPF_ITER_SELF_ONLY,             /* process only a single object. */
        BPF_ITER_DESCENDANTS_PRE,       /* walk descendants in pre-order. */
        BPF_ITER_DESCENDANTS_POST,      /* walk descendants in post-order. */
        BPF_ITER_ANCESTORS_UP,          /* walk ancestors upward. */
};

Sigh, I find that having UNSPEC=0 and erroring out when seeing UNSPEC
doesn't work. Basically, if we have a non-iter prog and a cgroup_iter
prog written in the same source file, I can't use
bpf_object__attach_skeleton to attach them. Because the default
prog_attach_fn for iter initializes `order` to 0 (that is, UNSPEC),
which is going to be rejected by the kernel. In order to make
bpf_object__attach_skeleton work on cgroup_iter, I think I need to use
the following

enum bpf_iter_order {
        BPF_ITER_DESCENDANTS_PRE,       /* walk descendants in pre-order. */
        BPF_ITER_DESCENDANTS_POST,      /* walk descendants in post-order. */
        BPF_ITER_ANCESTORS_UP,          /* walk ancestors upward. */
        BPF_ITER_SELF_ONLY,             /* process only a single object. */
};

So that when calling bpf_object__attach_skeleton() on cgroup_iter, a
link can be generated and the generated link defaults to pre-order
walk on the whole hierarchy. Is there a better solution?

I think this can be handled by userspace? We can attach the
cgroup_iter separately first (and maybe we will need to set prog->link
as well) so that bpf_object__attach_skeleton() doesn't try to attach
it? I am following this pattern in the selftest in the final patch,
although I think I might be missing setting prog->link, so I am
wondering why there are no issues in that selftest which has the same
scenario that you are talking about.

I think such a pattern will need to be used anyway if the users need
to set any non-default arguments for the cgroup_iter prog (like the
selftest), right? The only case we are discussing here is the case
where the user wants to attach the cgroup_iter with all default
options (in which case the default order will fail).
I agree that it might be inconvenient if the default/uninitialized
options don't work for cgroup_iter, but Alexei pointed out that this
matches other bpf uapis.

My concern is that in the future we try to reuse enum bpf_iter_order
to set ordering for other iterators, and then the
default/uninitialized value (BPF_ITER_DESCENDANTS_PRE) doesn't make
sense for that iterator (e.g. not a tree). In this case, the same
problem that we are avoiding for cgroup_iter here will show up for
that iterator, and we can't easily change it at this point because
it's uapi.

quoted

and explicitly list the values acceptable by cgroup_iter, error out if
UNSPEC is detected.

Also, following Andrii's comments, will change BPF_ITER_SELF to
BPF_ITER_SELF_ONLY, which does seem a little bit explicit in
comparison.

quoted

I applied the first 3 patches to ease respin.

Thanks! This helps!

quoted

Thanks!