Message-ID: <01c72879-0f2e-fe3b-cb4b-5f0a899e4d5c@fb.com>
Date:   Wed, 21 Apr 2021 07:05:23 -0700
From:   Yonghong Song <yhs@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.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 4/20/21 11:06 PM, Alexei Starovoitov wrote:
> 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.

Thanks for explanation. Agree that gen_trace and non-gen_trace are two
totally actions as you said. Trying to reduce the number of hacks
will make codes better too.

Powered by blists - more mailing lists