Message-ID: <20210421060608.ktllw2v3bhgd5pvm@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 20 Apr 2021 23:06:08 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     davem@...emloft.net, daniel@...earbox.net, andrii@...nel.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH bpf-next 13/15] libbpf: Generate loader program out of
 BPF ELF file.

On Tue, Apr 20, 2021 at 10:30:21PM -0700, Yonghong Song wrote:
> > > > +
> > > > +static int bpf_gen__realloc_data_buf(struct bpf_gen *gen, __u32 size)
> > > 
> > > Maybe change the return type to size_t? Esp. in the below
> > > we have off + size > UINT32_MAX.
> > 
> > return type? it's 0 or error. you mean argument type?
> > I think u32 is better. The prog size and all other ways
> > the bpf_gen__add_data is called with 32-bit values.
> 
> Sorry, I mean
> 
> +static int bpf_gen__add_data(struct bpf_gen *gen, const void *data, __u32
> size)
> 
> Since we allow off + size could be close to UINT32_MAX,
> maybe bpf_gen__add_data should return __u32 instead of int.

ahh. that makes sense.

> > This helper is only used as mark_feat_supported(FEAT_FD_IDX)
> > to tell libbpf that it shouldn't probe anything.
> > Otherwise probing via prog_load screw up gen_trace completely.
> > May be it will be mark_all_feat_supported(void), but that seems less flexible.
> 
> Maybe add some comments here to explain why marking explicit supported
> instead if probing?

will do.

> > 
> > > > @@ -9383,7 +9512,13 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd,
> > > >    	}
> > > >    	/* kernel/module BTF ID */
> > > > -	err = find_kernel_btf_id(prog->obj, attach_name, attach_type, btf_obj_fd, btf_type_id);
> > > > +	if (prog->obj->gen_trace) {
> > > > +		bpf_gen__record_find_name(prog->obj->gen_trace, attach_name, attach_type);
> > > > +		*btf_obj_fd = 0;
> > > > +		*btf_type_id = 1;
> > > 
> > > We have quite some codes like this and may add more to support more
> > > features. I am wondering whether we could have some kind of callbacks
> > > to make the code more streamlined. But I am not sure how easy it is.
> > 
> > you mean find_kernel_btf_id() in general?
> > This 'find' operation is translated differently for
> > prog name as seen in this hunk via bpf_gen__record_find_name()
> > and via bpf_gen__record_extern() in another place.
> > For libbpf it's all find_kernel_btf_id(), but semantically they are different,
> > so they cannot map as-is to gen trace bpf_gen__find_kernel_btf_id (if there was
> > such thing).
> > Because such 'generic' callback wouldn't convey the meaning of what to do
> > with the result of the find.
> 
> I mean like calling
>     err = obj->ops->find_kernel_btf_id(...)
> where gen_trace and normal libbpf all registers their own callback functions
> for find_kernel_btf_id(). Similar ideas can be applied to
> other places or not. Not 100% sure this is the best approach or not,
> just want to bring it up for discussion.

What args that 'ops->find_kernel_btf_id' will have?
If it's done as-is with btf_obj_fd, btf_type_id pointers to store the results
how libbpf will invoke it?
Where this destination pointers point to?
In one case the desitination is btf_id inside bpf_attr to load a prog.
In other case the destination is a btf_id inside bpf_insn ld_imm64.
In other case it could be different bpf_insn.
That's what I meant that semantical context matters
and cannot be expressed a single callback.
bpf_gen__record_find_name vs bpf_gen__record_extern have this semantical
difference builtin into their names. They will be called by libbpf differently.

If you mean to allow to specify all such callbacks via ops and indirect
pointers instead of specific bpf_gen__foo/bar callbacks then it's certainly
doable I just don't see a use case for it. No one bothered to do this
kind of 'strace of libbpf'. It's also not exactly an strace. It's
recording the sequence of events that libbpf is doing.
Consider patch 12. It changes the order of
bpf_object__relocate_data and text. It doesn't call any new bpf_gen__ methods.
But the data these methods will see later is different. In this case they will
see relo->insn_idx that is correct for the whole 'main' program after
subprogs were appended to the end instead of relo->insn_idx that points
within a given subprog.
So this gen_trace logic is very tightly built around libbpf loading
internals and will change in the future as more features will be supported
by this loader prog (like CO-RE).
Hence I don't think 'callback' idea fits here, since callback assumes
generic infra that will likely stay. Whereas here bpf_gen__ methods
are more like tracepoints inside libbpf that will be added and removed.
Nothing stable about them.
If this loader prog logic was built from scratch it probably would be different.
It would just parse elf and relocate text and data.
It would certainly not have hacks like "*btf_obj_fd = 0; *btf_type_id = 1;"
They're only there to avoid changing every check inside libbpf that
assumes that if a helper succeeded these values are valid.
Like if map_create is a success the resulting fd != -1.
The alternative is to do 'if (obj->gen_trace)' in a lot more places
which looks less appealing. I hope to reduce the number of such hacks, of course.

Powered by blists - more mailing lists