On Wed, Dec 2, 2020 at 2:43 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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?
You mean like:
union {
__u32 attach_prog_fd; /* valid prog_fd to attach to
bpf prog */
__u32 attach_btf_obj_fd; /* or valid module BTF
object fd or zero to attach to vmlinux */
};
or don't introduce a new field name at all?
Sure. I'm fine with both. I think it's a good idea.
