On Wed, Dec 2, 2020 at 12:58 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Dec 01, 2020 at 04:16:12PM -0800, Andrii Nakryiko wrote:
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index c3458ec1f30a..60b95b51ccb8 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -558,6 +558,7 @@ union bpf_attr {
> > __u32 line_info_cnt; /* number of bpf_line_info records */
> > __u32 attach_btf_id; /* in-kernel BTF type id to attach to */
> > __u32 attach_prog_fd; /* 0 to attach to vmlinux */
> > + __u32 attach_btf_obj_id; /* vmlinux/module BTF object ID for BTF type */
>
> I think the uapi should use attach_btf_obj_fd here.
> Everywhere else uapi is using FDs to point to maps, progs, BTFs of progs.
> BTF of a module isn't different from BTF of a program.
> Looking at libbpf implementation... it has the FD of a module anyway,
> since it needs to fetch it to search for the function btf_id in there.
> So there won't be any inconvenience for libbpf to pass FD in here.
> From the uapi perspective attach_btf_obj_fd will remove potential
> race condition. It's very unlikely race, of course.
Yes, I actually contemplated that, but my preference went the ID way,
because it made libbpf implementation simpler and there was a nice
duality of using ID for types and BTF instances themselves.
The problem with FD is that when I load all module BTF objects, I open
their FD one at a time, and close it as soon as I read BTF raw data
back. If I don't do that on systems with many modules, I'll be keeping
potentially hundreds of FDs open, so I figured I don't want to do
that.
But I do see the FD instead of ID consistency as well, so I can go
with a simple and inefficient implementation of separate FD for each
BTF object for now, and if someone complains, we can teach libbpf to
lazily open FDs of module BTFs that are actually used (later, it will
complicate code unnecessarily). Not really worried about racing with
kernel modules being unloaded.
Also, if we use FD, we might not need a new attach_bpf_obj_id field at
all, we can re-use attach_prog_fd field (put it in union and have
attach_prog_fd/attach_btf_fd). On the kernel side, it would be easy to
check whether provided FD is for bpf_prog or btf. What do you think?
Too mysterious? Or good?
>
> The rest of the series look good to me.
Cool, thanks.
