On Thu, Jun 20, 2019 at 5:07 PM Stanislav Fomichev <sdf@xxxxxxxxxxx> 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@xxxxxx>
> > ---
> > 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>
