On Wed, Dec 2, 2020 at 3:12 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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 */
> };
like this with union, an aliased field name with a meaningful name
> or don't introduce a new field name at all?
> Sure. I'm fine with both. I think it's a good idea.
ok, will do this then
