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

[Stanislav Fomichev <sdf@fomichev.me>]

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?
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).

> +	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?

> +	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;
> +}
> +
> +int bpf_program__attach_tracepoint(struct bpf_program *prog,
> +				   const char *tp_category,
> +				   const char *tp_name)
> +{
> +	char errmsg[STRERR_BUFSIZE];
> +	int pfd, err;
> +
> +	pfd = perf_event_open_tracepoint(tp_category, tp_name);
> +	if (pfd < 0) {
> +		pr_warning("program '%s': failed to create tracepoint '%s/%s' perf event: %s\n",
> +			   bpf_program__title(prog, false),
> +			   tp_category, tp_name,
> +			   libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> +		return pfd;
> +	}
> +	err = bpf_program__attach_perf_event(prog, pfd);
> +	if (err) {
> +		libbpf_perf_event_disable_and_close(pfd);
> +		pr_warning("program '%s': failed to attach to tracepoint '%s/%s': %s\n",
> +			   bpf_program__title(prog, false),
> +			   tp_category, tp_name,
> +			   libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> +		return err;
> +	}
> +	return pfd;
> +}
> +
> +int bpf_program__attach_raw_tracepoint(struct bpf_program *prog,
> +				       const char *tp_name)
> +{
> +	char errmsg[STRERR_BUFSIZE];
> +	int bpf_fd, pfd;
> +
> +	bpf_fd = bpf_program__fd(prog);
> +	if (bpf_fd < 0) {
> +		pr_warning("program '%s': can't attach before loaded\n",
> +			   bpf_program__title(prog, false));
> +		return -EINVAL;
> +	}
> +	pfd = bpf_raw_tracepoint_open(tp_name, bpf_fd);
> +	if (pfd < 0) {
> +		pr_warning("program '%s': failed to attach to raw tracepoint '%s': %s\n",
> +			   bpf_program__title(prog, false), tp_name,
> +			   libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> +		return pfd;
> +	}
> +	return pfd;
> +}
> +
>  enum bpf_perf_event_ret
>  bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
>  			   void **copy_mem, size_t *copy_size,
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index a7264f06aa5f..bf7020a565c6 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -176,6 +176,11 @@ LIBBPF_API int bpf_program__attach_uprobe(struct bpf_program *prog,
>  					  pid_t pid,
>  					  const char *binary_path,
>  					  size_t func_offset);
> +LIBBPF_API int bpf_program__attach_tracepoint(struct bpf_program *prog,
> +					      const char *tp_category,
> +					      const char *tp_name);
> +LIBBPF_API int bpf_program__attach_raw_tracepoint(struct bpf_program *prog,
> +						  const char *tp_name);
>  
>  struct bpf_insn;
>  
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 1a982c2e1751..2382fbda4cbb 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -174,6 +174,8 @@ LIBBPF_0.0.4 {
>  		bpf_object__load_xattr;
>  		bpf_program__attach_kprobe;
>  		bpf_program__attach_perf_event;
> +		bpf_program__attach_raw_tracepoint;
> +		bpf_program__attach_tracepoint;
>  		bpf_program__attach_uprobe;
>  		libbpf_num_possible_cpus;
>  		libbpf_perf_event_disable_and_close;
> -- 
> 2.17.1
>