On Thu, Aug 11, 2022 at 7:10 AM Yosry Ahmed [off-list ref] wrote:

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

quoted

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 {

so first of all, this can't be called "bpf_iter_order" as it doesn't
apply to BPF iterators in general. I think this should be called
bpf_iter_cgroup_order (or maybe bpf_cgroup_iter_order) and if/when we
add ability to iterate tasks within cgroups then we'll just reuse enum
bpf_iter_cgroup_order as an extra parameter for task iterator.

And with that future case in mind I do think that we should have 0
being "UNSPEC" case.

quoted

        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 was actually surprised that we specify these additional parameters
at attach (LINK_CREATE) time, and not at bpf_iter_create() call time.
It seems more appropriate to allow to specify such runtime parameters
very late, when we create a specific instance of seq_file. But I guess
this was done because one of the initial motivations for iterators was
to be pinned in BPFFS and read as a file, so it was more convenient to
store such parameters upfront at link creation time to keep
BPF_OBJ_PIN simpler. I guess it makes sense, worst case you'll need to
create multiple bpf_link files, one for each cgroup hierarchy you'd
like to query with the same single BPF program.

But I digress.

As for not being able to auto-attach cgroup iterator. I think that's
sort of expected and is in line with not being able to auto-attach
cgroup programs, as you need cgroup FD at runtime. So even if you had
some reasonable default order, you still would need to specify target
cgroup (either through FD or ID).

So... either don't do skeleton auto-attach, or let's teach libbpf code
to not auto-attach some iter types?

Alternatively, we could teach libbpf to parse some sort of cgroup
iterator spec, like:

SEC("iter/cgroup:/path/to/cgroup:descendants_pre")

But this approach won't work for a bunch of other parameterized
iterators (e.g., task iter, or map elem iter), so I'm hesitant about
adding this to libbpf as a generic functionality.

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.

Yep, valid concern, I agree.

quoted

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!