Re: [PATCH bpf-next 4/7] libbpf: add tracepoint/raw tracepoint attach API

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Jun 20, 2019 at 5:07 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 06/20, Andrii Nakryiko wrote:
> > Add APIs allowing to attach BPF program to kernel tracepoints. Raw
> > tracepoint attach API is also added for uniform per-BPF-program API,
> > but is mostly a wrapper around existing bpf_raw_tracepoint_open call.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c   | 99 ++++++++++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/libbpf.h   |  5 ++
> >  tools/lib/bpf/libbpf.map |  2 +
> >  3 files changed, 106 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 11329e05530e..cefe67ba160b 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -4176,6 +4176,105 @@ int bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> >       return pfd;
> >  }
> >
> > +static int determine_tracepoint_id(const char* tp_category, const char* tp_name)
> > +{
> > +     char file[PATH_MAX];
> > +     int ret;
> > +
> > +     ret = snprintf(file, sizeof(file),
> > +                    "/sys/kernel/debug/tracing/events/%s/%s/id",
> > +                    tp_category, tp_name);
> > +     if (ret < 0)
> > +             return -errno;
> > +     if (ret >= sizeof(file)) {
> > +             pr_debug("tracepoint %s/%s path is too long\n",
> > +                      tp_category, tp_name);
> > +             return -E2BIG;
> > +     }
> > +     return parse_uint_from_file(file);
> > +}
> > +
> > +static int perf_event_open_tracepoint(const char* tp_category,
> > +                                   const char* tp_name)
> > +{
> > +     struct perf_event_attr attr = {};
> > +     char errmsg[STRERR_BUFSIZE];
> > +     int tp_id, pfd, err;
> > +
> [..]
> > +     tp_id = determine_tracepoint_id(tp_category, tp_name);
> Why no assign to attr.config directly here?

It's used in few places for error-handling branch, so it would look a
bit weird and make lines longer.


> You can move all other constants to the initialization as well:
>
> struct perf_event_attr attr = {
>         .type = PERF_TYPE_TRACEPON,
>         .size = sizeof(struct perf_event_attr),
> };
>
> attr.config = determine_tracepoint_id(...);
>
> (I guess that's a matter of style, but something to consider).

Yeah. It seems like explicit initialization of each member of
attribute structs is prevalent in libbpf.c. I also don't want to have
some fields initialized at declaration site, and some other in code.
Better to group all initialization together.

>
> > +     if (tp_id < 0){
> > +             pr_warning("failed to determine tracepoint '%s/%s' perf ID: %s\n",
> > +                        tp_category, tp_name,
> > +                        libbpf_strerror_r(tp_id, errmsg, sizeof(errmsg)));
> > +             return tp_id;
> > +     }
> > +
> [..]
> > +     memset(&attr, 0, sizeof(attr));
> Not needed since you do attr = {}; above?

Yep, removed.

>
> > +     attr.type = PERF_TYPE_TRACEPOINT;
> > +     attr.size = sizeof(attr);
> > +     attr.config = tp_id;
> > +
> > +     pfd = syscall( __NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
> > +                     -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > +     if (pfd < 0) {
> > +             err = -errno;
> > +             pr_warning("tracepoint '%s/%s' perf_event_open() failed: %s\n",
> > +                        tp_category, tp_name,
> > +                        libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > +             return err;
> > +     }
> > +     return pfd;
> > +}
> > +

<snip>