[PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link

Currently, some samples are using ioctl for enabling perf_event and
attaching BPF programs to this event. However, the bpf_program__attach
of libbpf(using bpf_link) is much more intuitive than the previous
method using ioctl.

bpf_program__attach_perf_event manages the enable of perf_event and
attach of BPF programs to it, so there's no neeed to do this
directly with ioctl.

In addition, bpf_link provides consistency in the use of API because it
allows disable (detach, destroy) for multiple events to be treated as
one bpf_link__destroy.

To refactor samples with using this libbpf API, the bpf_load in the
samples were removed and migrated to libbbpf. Because read_trace_pipe
is used in bpf_load, multiple samples cannot be migrated to libbpf,
this function was moved to trace_helpers.

Changes in v2:
 - check memory allocation is successful
 - clean up allocated memory on error

Daniel T. Lee (2):
  samples: bpf: move read_trace_pipe to trace_helpers
  samples: bpf: refactor perf_event user program with libbpf bpf_link

 samples/bpf/Makefile                        |  8 +--
 samples/bpf/bpf_load.c                      | 20 ------
 samples/bpf/bpf_load.h                      |  1 -
 samples/bpf/sampleip_user.c                 | 76 ++++++++++++++-------
 samples/bpf/trace_event_user.c              | 63 ++++++++++++-----
 samples/bpf/tracex1_user.c                  |  1 +
 samples/bpf/tracex5_user.c                  |  1 +
 tools/testing/selftests/bpf/trace_helpers.c | 23 +++++++
 tools/testing/selftests/bpf/trace_helpers.h |  1 +
 9 files changed, 125 insertions(+), 69 deletions(-)

-- 
2.25.1

To reduce the reliance of trace samples (trace*_user) on bpf_load,
move read_trace_pipe to trace_helpers. By moving this bpf_loader helper
elsewhere, trace functions can be easily migrated to libbbpf.

Signed-off-by: Daniel T. Lee <redacted>
---
 samples/bpf/Makefile                        |  4 ++--
 samples/bpf/bpf_load.c                      | 20 ------------------
 samples/bpf/bpf_load.h                      |  1 -
 samples/bpf/tracex1_user.c                  |  1 +
 samples/bpf/tracex5_user.c                  |  1 +
 tools/testing/selftests/bpf/trace_helpers.c | 23 +++++++++++++++++++++
 tools/testing/selftests/bpf/trace_helpers.h |  1 +
 7 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 79b0fee6943b..ff0061467dd3 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile

@@ -64,11 +64,11 @@ fds_example-objs := fds_example.o
 sockex1-objs := sockex1_user.o
 sockex2-objs := sockex2_user.o
 sockex3-objs := bpf_load.o sockex3_user.o
-tracex1-objs := bpf_load.o tracex1_user.o
+tracex1-objs := bpf_load.o tracex1_user.o $(TRACE_HELPERS)
 tracex2-objs := bpf_load.o tracex2_user.o
 tracex3-objs := bpf_load.o tracex3_user.o
 tracex4-objs := bpf_load.o tracex4_user.o
-tracex5-objs := bpf_load.o tracex5_user.o
+tracex5-objs := bpf_load.o tracex5_user.o $(TRACE_HELPERS)
 tracex6-objs := bpf_load.o tracex6_user.o
 tracex7-objs := bpf_load.o tracex7_user.o
 test_probe_write_user-objs := bpf_load.o test_probe_write_user_user.o

diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index 4574b1939e49..c5ad528f046e 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c

@@ -665,23 +665,3 @@ int load_bpf_file_fixup_map(const char *path, fixup_map_cb fixup_map)
 {
 	return do_load_bpf_file(path, fixup_map);
 }
-
-void read_trace_pipe(void)
-{
-	int trace_fd;
-
-	trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
-	if (trace_fd < 0)
-		return;
-
-	while (1) {
-		static char buf[4096];
-		ssize_t sz;
-
-		sz = read(trace_fd, buf, sizeof(buf) - 1);
-		if (sz > 0) {
-			buf[sz] = 0;
-			puts(buf);
-		}
-	}
-}

diff --git a/samples/bpf/bpf_load.h b/samples/bpf/bpf_load.h
index 814894a12974..4fcd258c616f 100644
--- a/samples/bpf/bpf_load.h
+++ b/samples/bpf/bpf_load.h

@@ -53,6 +53,5 @@ extern int map_data_count;
 int load_bpf_file(char *path);
 int load_bpf_file_fixup_map(const char *path, fixup_map_cb fixup_map);
 
-void read_trace_pipe(void);
 int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags);
 #endif

diff --git a/samples/bpf/tracex1_user.c b/samples/bpf/tracex1_user.c
index af8c20608ab5..55fddbd08702 100644
--- a/samples/bpf/tracex1_user.c
+++ b/samples/bpf/tracex1_user.c

@@ -4,6 +4,7 @@
 #include <unistd.h>
 #include <bpf/bpf.h>
 #include "bpf_load.h"
+#include "trace_helpers.h"
 
 int main(int ac, char **argv)
 {

diff --git a/samples/bpf/tracex5_user.c b/samples/bpf/tracex5_user.c
index c4ab91c89494..c2317b39e0d2 100644
--- a/samples/bpf/tracex5_user.c
+++ b/samples/bpf/tracex5_user.c

@@ -8,6 +8,7 @@
 #include <bpf/bpf.h>
 #include "bpf_load.h"
 #include <sys/resource.h>
+#include "trace_helpers.h"
 
 /* install fake seccomp program to enable seccomp code path inside the kernel,
  * so that our kprobe attached to seccomp_phase1() can be triggered

diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
index 7f989b3e4e22..4d0e913bbb22 100644
--- a/tools/testing/selftests/bpf/trace_helpers.c
+++ b/tools/testing/selftests/bpf/trace_helpers.c

@@ -4,12 +4,15 @@
 #include <string.h>
 #include <assert.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <poll.h>
 #include <unistd.h>
 #include <linux/perf_event.h>
 #include <sys/mman.h>
 #include "trace_helpers.h"
 
+#define DEBUGFS "/sys/kernel/debug/tracing/"
+
 #define MAX_SYMS 300000
 static struct ksym syms[MAX_SYMS];
 static int sym_cnt;

@@ -86,3 +89,23 @@ long ksym_get_addr(const char *name)
 
 	return 0;
 }
+
+void read_trace_pipe(void)
+{
+	int trace_fd;
+
+	trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0);
+	if (trace_fd < 0)
+		return;
+
+	while (1) {
+		static char buf[4096];
+		ssize_t sz;
+
+		sz = read(trace_fd, buf, sizeof(buf) - 1);
+		if (sz > 0) {
+			buf[sz] = 0;
+			puts(buf);
+		}
+	}
+}

diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h
index 0383c9b8adc1..25ef597dd03f 100644
--- a/tools/testing/selftests/bpf/trace_helpers.h
+++ b/tools/testing/selftests/bpf/trace_helpers.h

@@ -12,5 +12,6 @@ struct ksym {
 int load_kallsyms(void);
 struct ksym *ksym_search(long key);
 long ksym_get_addr(const char *name);
+void read_trace_pipe(void);
 
 #endif

-- 
2.25.1

The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
than the previous method using ioctl.

bpf_program__attach_perf_event manages the enable of perf_event and
attach of BPF programs to it, so there's no neeed to do this
directly with ioctl.

In addition, bpf_link provides consistency in the use of API because it
allows disable (detach, destroy) for multiple events to be treated as
one bpf_link__destroy.

This commit refactors samples that attach the bpf program to perf_event
by using libbbpf instead of ioctl. Also the bpf_load in the samples were
removed and migrated to use libbbpf API.

Signed-off-by: Daniel T. Lee <redacted>
---
Changes in v2:
 - check memory allocation is successful
 - clean up allocated memory on error

 samples/bpf/Makefile           |  4 +-
 samples/bpf/sampleip_user.c    | 76 ++++++++++++++++++++++------------
 samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
 3 files changed, 97 insertions(+), 46 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index ff0061467dd3..424f6fe7ce38 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile

@@ -88,8 +88,8 @@ xdp2-objs := xdp1_user.o
 xdp_router_ipv4-objs := xdp_router_ipv4_user.o
 test_current_task_under_cgroup-objs := bpf_load.o $(CGROUP_HELPERS) \
 				       test_current_task_under_cgroup_user.o
-trace_event-objs := bpf_load.o trace_event_user.o $(TRACE_HELPERS)
-sampleip-objs := bpf_load.o sampleip_user.o $(TRACE_HELPERS)
+trace_event-objs := trace_event_user.o $(TRACE_HELPERS)
+sampleip-objs := sampleip_user.o $(TRACE_HELPERS)
 tc_l2_redirect-objs := bpf_load.o tc_l2_redirect_user.o
 lwt_len_hist-objs := bpf_load.o lwt_len_hist_user.o
 xdp_tx_iptunnel-objs := xdp_tx_iptunnel_user.o

diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
index b0f115f938bc..fd763a49c913 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c

@@ -10,13 +10,11 @@
 #include <errno.h>
 #include <signal.h>
 #include <string.h>
-#include <assert.h>
 #include <linux/perf_event.h>
 #include <linux/ptrace.h>
 #include <linux/bpf.h>
-#include <sys/ioctl.h>
+#include <bpf/bpf.h>
 #include <bpf/libbpf.h>
-#include "bpf_load.h"
 #include "perf-sys.h"
 #include "trace_helpers.h"
 

@@ -25,6 +23,7 @@
 #define MAX_IPS		8192
 #define PAGE_OFFSET	0xffff880000000000
 
+static int map_fd;
 static int nr_cpus;
 
 static void usage(void)

@@ -34,7 +33,8 @@ static void usage(void)
 	printf("       duration   # sampling duration (seconds), default 5\n");
 }
 
-static int sampling_start(int *pmu_fd, int freq)
+static int sampling_start(int *pmu_fd, int freq, struct bpf_program *prog,
+			  struct bpf_link **link)
 {
 	int i;
 

@@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
 			fprintf(stderr, "ERROR: Initializing perf sampling\n");
 			return 1;
 		}
-		assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF,
-			     prog_fd[0]) == 0);
-		assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE, 0) == 0);
+		link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+		if (link[i] < 0) {
+			fprintf(stderr, "ERROR: Attach perf event\n");
+			return 1;
+		}
 	}
 
 	return 0;
 }
 
-static void sampling_end(int *pmu_fd)
+static void sampling_end(struct bpf_link **link)
 {
 	int i;
 
 	for (i = 0; i < nr_cpus; i++)
-		close(pmu_fd[i]);
+		bpf_link__destroy(link[i]);
 }
 
 struct ipcount {

@@ -128,14 +130,18 @@ static void print_ip_map(int fd)
 static void int_exit(int sig)
 {
 	printf("\n");
-	print_ip_map(map_fd[0]);
+	print_ip_map(map_fd);
 	exit(0);
 }
 
 int main(int argc, char **argv)
 {
+	int prog_fd, *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	struct bpf_link **link;
 	char filename[256];
-	int *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
+	int error = 0;
 
 	/* process arguments */
 	while ((opt = getopt(argc, argv, "F:h")) != -1) {

@@ -165,36 +171,54 @@ int main(int argc, char **argv)
 	/* create perf FDs for each CPU */
 	nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
 	pmu_fd = malloc(nr_cpus * sizeof(int));
-	if (pmu_fd == NULL) {
-		fprintf(stderr, "ERROR: malloc of pmu_fd\n");
-		return 1;
+	link = malloc(nr_cpus * sizeof(struct bpf_link *));
+	if (pmu_fd == NULL || link == NULL) {
+		fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
+		error = 1;
+		goto cleanup;
 	}
 
 	/* load BPF program */
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
-	if (load_bpf_file(filename)) {
+	if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd)) {
 		fprintf(stderr, "ERROR: loading BPF program (errno %d):\n",
 			errno);
-		if (strcmp(bpf_log_buf, "") == 0)
-			fprintf(stderr, "Try: ulimit -l unlimited\n");
-		else
-			fprintf(stderr, "%s", bpf_log_buf);
-		return 1;
+		error = 1;
+		goto cleanup;
+	}
+
+	prog = bpf_program__next(NULL, obj);
+	if (!prog) {
+		fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
+		error = 1;
+		goto cleanup;
+	}
+
+	map_fd = bpf_object__find_map_fd_by_name(obj, "ip_map");
+	if (map_fd < 0) {
+		fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+		error = 1;
+		goto cleanup;
 	}
+
 	signal(SIGINT, int_exit);
 	signal(SIGTERM, int_exit);
 
 	/* do sampling */
 	printf("Sampling at %d Hertz for %d seconds. Ctrl-C also ends.\n",
 	       freq, secs);
-	if (sampling_start(pmu_fd, freq) != 0)
-		return 1;
+	if (sampling_start(pmu_fd, freq, prog, link) != 0) {
+		error = 1;
+		goto cleanup;
+	}
 	sleep(secs);
-	sampling_end(pmu_fd);
-	free(pmu_fd);
+	sampling_end(link);
 
 	/* output sample counts */
-	print_ip_map(map_fd[0]);
+	print_ip_map(map_fd);
 
-	return 0;
+cleanup:
+	free(pmu_fd);
+	free(link);
+	return error;
 }

diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index 356171bc392b..30c25ef99fc5 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c

@@ -6,22 +6,21 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <string.h>
-#include <fcntl.h>
-#include <poll.h>
-#include <sys/ioctl.h>
 #include <linux/perf_event.h>
 #include <linux/bpf.h>
 #include <signal.h>
-#include <assert.h>
 #include <errno.h>
 #include <sys/resource.h>
+#include <bpf/bpf.h>
 #include <bpf/libbpf.h>
-#include "bpf_load.h"
 #include "perf-sys.h"
 #include "trace_helpers.h"
 
 #define SAMPLE_FREQ 50
 
+/* counts, stackmap */
+static int map_fd[2];
+struct bpf_program *prog;
 static bool sys_read_seen, sys_write_seen;
 
 static void print_ksym(__u64 addr)

@@ -137,9 +136,16 @@ static inline int generate_load(void)
 static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 {
 	int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
+	struct bpf_link **link = malloc(nr_cpus * sizeof(struct bpf_link *));
 	int *pmu_fd = malloc(nr_cpus * sizeof(int));
 	int i, error = 0;
 
+	if (pmu_fd == NULL || link == NULL) {
+		printf("malloc of pmu_fd/link failed\n");
+		error = 1;
+		goto err;
+	}
+
 	/* system wide perf event, no need to inherit */
 	attr->inherit = 0;
 

@@ -151,8 +157,12 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 			error = 1;
 			goto all_cpu_err;
 		}
-		assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
-		assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE) == 0);
+		link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+		if (link[i] < 0) {
+			printf("bpf_program__attach_perf_event failed\n");
+			error = 1;
+			goto all_cpu_err;
+		}
 	}
 
 	if (generate_load() < 0) {

@@ -161,11 +171,11 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 	}
 	print_stacks();
 all_cpu_err:
-	for (i--; i >= 0; i--) {
-		ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
-		close(pmu_fd[i]);
-	}
+	for (i--; i >= 0; i--)
+		bpf_link__destroy(link[i]);
+err:
 	free(pmu_fd);
+	free(link);
 	if (error)
 		int_exit(0);
 }

@@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 static void test_perf_event_task(struct perf_event_attr *attr)
 {
 	int pmu_fd, error = 0;
+	struct bpf_link *link;
 
 	/* per task perf event, enable inherit so the "dd ..." command can be traced properly.
 	 * Enabling inherit will cause bpf_perf_prog_read_time helper failure.

@@ -185,8 +196,12 @@ static void test_perf_event_task(struct perf_event_attr *attr)
 		printf("sys_perf_event_open failed\n");
 		int_exit(0);
 	}
-	assert(ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
-	assert(ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE) == 0);
+	link = bpf_program__attach_perf_event(prog, pmu_fd);
+	if (link < 0) {
+		printf("bpf_program__attach_perf_event failed\n");
+		close(pmu_fd);
+		int_exit(0);
+	}
 
 	if (generate_load() < 0) {
 		error = 1;

@@ -194,8 +209,7 @@ static void test_perf_event_task(struct perf_event_attr *attr)
 	}
 	print_stacks();
 err:
-	ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
-	close(pmu_fd);
+	bpf_link__destroy(link);
 	if (error)
 		int_exit(0);
 }

@@ -282,7 +296,9 @@ static void test_bpf_perf_event(void)
 int main(int argc, char **argv)
 {
 	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	struct bpf_object *obj;
 	char filename[256];
+	int prog_fd;
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 	setrlimit(RLIMIT_MEMLOCK, &r);

@@ -295,9 +311,20 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	if (load_bpf_file(filename)) {
-		printf("%s", bpf_log_buf);
-		return 2;
+	if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
+		return 1;
+
+	prog = bpf_program__next(NULL, obj);
+	if (!prog) {
+		printf("finding a prog in obj file failed\n");
+		return 1;
+	}
+
+	map_fd[0] = bpf_object__find_map_fd_by_name(obj, "counts");
+	map_fd[1] = bpf_object__find_map_fd_by_name(obj, "stackmap");
+	if (map_fd[0] < 0 || map_fd[1] < 0) {
+		printf("finding a counts/stackmap map in obj file failed\n");
+		return 1;
 	}
 
 	if (fork() == 0) {

-- 
2.25.1

On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee [off-list ref] wrote:

The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
than the previous method using ioctl.

bpf_program__attach_perf_event manages the enable of perf_event and
attach of BPF programs to it, so there's no neeed to do this
directly with ioctl.

In addition, bpf_link provides consistency in the use of API because it
allows disable (detach, destroy) for multiple events to be treated as
one bpf_link__destroy.

This commit refactors samples that attach the bpf program to perf_event
by using libbbpf instead of ioctl. Also the bpf_load in the samples were
removed and migrated to use libbbpf API.

Signed-off-by: Daniel T. Lee <redacted>
---

Daniel, thanks for this clean up! It's good to see samples be
modernized a bit :)

It's not apparent from looking at struct bpf_link **link whether it's
an output parameter (so sampling_start is supposed to assign *single*
link to return it to calling function) or it's an array of pointers.
Seems like it's the latter, so I'd prefer this written as

struct bpf_link *links[] (notice also plural name).

Please consider this.

link is a pointer, < 0 doesn't make sense and is always going to be
false on x86. Use IS_ERR(link[i]). It's also a good idea to set it to
NULL, if link creation failed to prevent accidental
bpf_link__destroy(link[i]) later on, trying to free bogus pointer.

+                       fprintf(stderr, "ERROR: Attach perf event\n");
+                       return 1;
+               }
        }

        return 0;
 }

-static void sampling_end(int *pmu_fd)
+static void sampling_end(struct bpf_link **link)

same as above, struct bpf_link *links[] would be much better here, IMO.

While neither approach is ideal, using number of online CPUs
(_SC_NPROCESSORS_ONLN) will probably work in slightly more cases
(there are machines configured with, say, 256 possible CPUs, but only
32 available, for instance).

        pmu_fd = malloc(nr_cpus * sizeof(int));

similar naming nit: pmu_fds?

-       if (pmu_fd == NULL) {
-               fprintf(stderr, "ERROR: malloc of pmu_fd\n");
-               return 1;
+       link = malloc(nr_cpus * sizeof(struct bpf_link *));

Use calloc() to have those links initialized to NULL automatically.
Makes clean up so much easier.

+       if (pmu_fd == NULL || link == NULL) {
+               fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
+               error = 1;
+               goto cleanup;
        }

        /* load BPF program */
        snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
-       if (load_bpf_file(filename)) {
+       if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd)) {

Using skeleton would be best, but it's probably more appropriate for
another patch to integrate skeleton generation with samples/bpf. So
the next one would be bpf_object__open_file(), instead of legacy
bpf_prog_load().

                fprintf(stderr, "ERROR: loading BPF program (errno %d):\n",
                        errno);
-               if (strcmp(bpf_log_buf, "") == 0)
-                       fprintf(stderr, "Try: ulimit -l unlimited\n");
-               else
-                       fprintf(stderr, "%s", bpf_log_buf);
-               return 1;
+               error = 1;
+               goto cleanup;
+       }
+
+       prog = bpf_program__next(NULL, obj);

I'm a bit lazy here, sorry, but isn't the name of the program known?
bpf_object__find_program_by_title() is preferable.

+       if (!prog) {
+               fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
+               error = 1;
+               goto cleanup;
+       }
+
+       map_fd = bpf_object__find_map_fd_by_name(obj, "ip_map");
+       if (map_fd < 0) {
+               fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+               error = 1;
+               goto cleanup;
        }
+
        signal(SIGINT, int_exit);
        signal(SIGTERM, int_exit);

        /* do sampling */
        printf("Sampling at %d Hertz for %d seconds. Ctrl-C also ends.\n",
               freq, secs);
-       if (sampling_start(pmu_fd, freq) != 0)
-               return 1;
+       if (sampling_start(pmu_fd, freq, prog, link) != 0) {
+               error = 1;
+               goto cleanup;
+       }
        sleep(secs);
-       sampling_end(pmu_fd);
-       free(pmu_fd);
+       sampling_end(link);

        /* output sample counts */
-       print_ip_map(map_fd[0]);
+       print_ip_map(map_fd);

-       return 0;
+cleanup:
+       free(pmu_fd);
+       free(link);

Uhm... you are freeing this only on clean up. Also, you need to
bpf_link__destroy() first. And close all pmu_fds. Surely process exit
will ensure all this is cleaned up, but it's a good tone to clean up
all resources explicitly.

same as above, calloc() is better choice here

still need close(pmu_fd[i]);

+err:
        free(pmu_fd);
+       free(link);
        if (error)
                int_exit(0);

This will exit with 0 error code and won't notify about error... Pass
through err?

On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
[off-list ref] wrote:

On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee [off-list ref] wrote:

quoted

The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
than the previous method using ioctl.

bpf_program__attach_perf_event manages the enable of perf_event and
attach of BPF programs to it, so there's no neeed to do this
directly with ioctl.

In addition, bpf_link provides consistency in the use of API because it
allows disable (detach, destroy) for multiple events to be treated as
one bpf_link__destroy.

This commit refactors samples that attach the bpf program to perf_event
by using libbbpf instead of ioctl. Also the bpf_load in the samples were
removed and migrated to use libbbpf API.

Signed-off-by: Daniel T. Lee <redacted>
---

Daniel, thanks for this clean up! It's good to see samples be
modernized a bit :)

Thank you for your time and effort for the review :)

quoted

Changes in v2:
 - check memory allocation is successful
 - clean up allocated memory on error

 samples/bpf/Makefile           |  4 +-
 samples/bpf/sampleip_user.c    | 76 ++++++++++++++++++++++------------
 samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
 3 files changed, 97 insertions(+), 46 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index ff0061467dd3..424f6fe7ce38 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile

@@ -88,8 +88,8 @@ xdp2-objs := xdp1_user.o
 xdp_router_ipv4-objs := xdp_router_ipv4_user.o
 test_current_task_under_cgroup-objs := bpf_load.o $(CGROUP_HELPERS) \
                                       test_current_task_under_cgroup_user.o
-trace_event-objs := bpf_load.o trace_event_user.o $(TRACE_HELPERS)
-sampleip-objs := bpf_load.o sampleip_user.o $(TRACE_HELPERS)
+trace_event-objs := trace_event_user.o $(TRACE_HELPERS)
+sampleip-objs := sampleip_user.o $(TRACE_HELPERS)
 tc_l2_redirect-objs := bpf_load.o tc_l2_redirect_user.o
 lwt_len_hist-objs := bpf_load.o lwt_len_hist_user.o
 xdp_tx_iptunnel-objs := xdp_tx_iptunnel_user.o

diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
index b0f115f938bc..fd763a49c913 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c

@@ -10,13 +10,11 @@
 #include <errno.h>
 #include <signal.h>
 #include <string.h>
-#include <assert.h>
 #include <linux/perf_event.h>
 #include <linux/ptrace.h>
 #include <linux/bpf.h>
-#include <sys/ioctl.h>
+#include <bpf/bpf.h>
 #include <bpf/libbpf.h>
-#include "bpf_load.h"
 #include "perf-sys.h"
 #include "trace_helpers.h"

@@ -25,6 +23,7 @@
 #define MAX_IPS                8192
 #define PAGE_OFFSET    0xffff880000000000

+static int map_fd;
 static int nr_cpus;

 static void usage(void)

@@ -34,7 +33,8 @@ static void usage(void)
        printf("       duration   # sampling duration (seconds), default 5\n");
 }

-static int sampling_start(int *pmu_fd, int freq)
+static int sampling_start(int *pmu_fd, int freq, struct bpf_program *prog,
+                         struct bpf_link **link)

It's not apparent from looking at struct bpf_link **link whether it's
an output parameter (so sampling_start is supposed to assign *single*
link to return it to calling function) or it's an array of pointers.
Seems like it's the latter, so I'd prefer this written as

struct bpf_link *links[] (notice also plural name).

Please consider this.

This approach looks more apparent!
I'll update code using this way.

quoted

 {
        int i;

@@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
                        fprintf(stderr, "ERROR: Initializing perf sampling\n");
                        return 1;
                }
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF,
-                            prog_fd[0]) == 0);
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE, 0) == 0);
+               link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+               if (link[i] < 0) {

link is a pointer, < 0 doesn't make sense and is always going to be
false on x86. Use IS_ERR(link[i]). It's also a good idea to set it to
NULL, if link creation failed to prevent accidental
bpf_link__destroy(link[i]) later on, trying to free bogus pointer.

Failure on link creation is exactly what I was concerned about.
Thank you for giving me a clear solution!

quoted

+                       fprintf(stderr, "ERROR: Attach perf event\n");
+                       return 1;
+               }
        }

        return 0;
 }

-static void sampling_end(int *pmu_fd)
+static void sampling_end(struct bpf_link **link)

same as above, struct bpf_link *links[] would be much better here, IMO.

Also, I'll apply this at next version patch.

quoted

 {
        int i;

        for (i = 0; i < nr_cpus; i++)
-               close(pmu_fd[i]);
+               bpf_link__destroy(link[i]);
 }

 struct ipcount {

@@ -128,14 +130,18 @@ static void print_ip_map(int fd)
 static void int_exit(int sig)
 {
        printf("\n");
-       print_ip_map(map_fd[0]);
+       print_ip_map(map_fd);
        exit(0);
 }

 int main(int argc, char **argv)
 {
+       int prog_fd, *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
+       struct bpf_program *prog;
+       struct bpf_object *obj;
+       struct bpf_link **link;
        char filename[256];
-       int *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
+       int error = 0;

        /* process arguments */
        while ((opt = getopt(argc, argv, "F:h")) != -1) {

@@ -165,36 +171,54 @@ int main(int argc, char **argv)
        /* create perf FDs for each CPU */
        nr_cpus = sysconf(_SC_NPROCESSORS_CONF);

While neither approach is ideal, using number of online CPUs
(_SC_NPROCESSORS_ONLN) will probably work in slightly more cases
(there are machines configured with, say, 256 possible CPUs, but only
32 available, for instance).

Thank you for pointing me out!
I've never thought about situation when processors may be offline.

quoted

        pmu_fd = malloc(nr_cpus * sizeof(int));

similar naming nit: pmu_fds?

Same again, apply this at next version patch.

quoted

-       if (pmu_fd == NULL) {
-               fprintf(stderr, "ERROR: malloc of pmu_fd\n");
-               return 1;
+       link = malloc(nr_cpus * sizeof(struct bpf_link *));

Use calloc() to have those links initialized to NULL automatically.
Makes clean up so much easier.

About NULL set, like you mentioned, calloc approach looks more neat.

quoted

+       if (pmu_fd == NULL || link == NULL) {
+               fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
+               error = 1;
+               goto cleanup;
        }

        /* load BPF program */
        snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
-       if (load_bpf_file(filename)) {
+       if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd)) {

Using skeleton would be best, but it's probably more appropriate for
another patch to integrate skeleton generation with samples/bpf. So
the next one would be bpf_object__open_file(), instead of legacy
bpf_prog_load().

I'll try skeleton with other sample cleanup. For now, I'll stick with
bpf_object__{open/load}() instead of bpf_prog_load().

quoted

                fprintf(stderr, "ERROR: loading BPF program (errno %d):\n",
                        errno);
-               if (strcmp(bpf_log_buf, "") == 0)
-                       fprintf(stderr, "Try: ulimit -l unlimited\n");
-               else
-                       fprintf(stderr, "%s", bpf_log_buf);
-               return 1;
+               error = 1;
+               goto cleanup;
+       }
+
+       prog = bpf_program__next(NULL, obj);

I'm a bit lazy here, sorry, but isn't the name of the program known?
bpf_object__find_program_by_title() is preferable.

I also think it is good to specify the program title clearly.

quoted

+       if (!prog) {
+               fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
+               error = 1;
+               goto cleanup;
+       }
+
+       map_fd = bpf_object__find_map_fd_by_name(obj, "ip_map");
+       if (map_fd < 0) {
+               fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+               error = 1;
+               goto cleanup;
        }
+
        signal(SIGINT, int_exit);
        signal(SIGTERM, int_exit);

        /* do sampling */
        printf("Sampling at %d Hertz for %d seconds. Ctrl-C also ends.\n",
               freq, secs);
-       if (sampling_start(pmu_fd, freq) != 0)
-               return 1;
+       if (sampling_start(pmu_fd, freq, prog, link) != 0) {
+               error = 1;
+               goto cleanup;
+       }
        sleep(secs);
-       sampling_end(pmu_fd);
-       free(pmu_fd);
+       sampling_end(link);

        /* output sample counts */
-       print_ip_map(map_fd[0]);
+       print_ip_map(map_fd);

-       return 0;
+cleanup:
+       free(pmu_fd);
+       free(link);

Uhm... you are freeing this only on clean up. Also, you need to
bpf_link__destroy() first. And close all pmu_fds. Surely process exit
will ensure all this is cleaned up, but it's a good tone to clean up
all resources explicitly.

Well, cleanup: could cover link destroy (sampling_end), but I think
it is strange to clean up the link even though the bpf program is not
attached to the event. I think it is better to specify the link destroy
after the sampling starts.

And, I've missed the link destroy when sampling got error.
Since sampling_end will destroy the links, so I'll add this on error.

       if (sampling_start(pmu_fd, freq, prog, link) != 0) {
               error = 1;
+             sampling_end(links);
               goto cleanup;
       }

quoted

+       return error;
 }

diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index 356171bc392b..30c25ef99fc5 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c

@@ -6,22 +6,21 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <string.h>
-#include <fcntl.h>
-#include <poll.h>
-#include <sys/ioctl.h>
 #include <linux/perf_event.h>
 #include <linux/bpf.h>
 #include <signal.h>
-#include <assert.h>
 #include <errno.h>
 #include <sys/resource.h>
+#include <bpf/bpf.h>
 #include <bpf/libbpf.h>
-#include "bpf_load.h"
 #include "perf-sys.h"
 #include "trace_helpers.h"

 #define SAMPLE_FREQ 50

+/* counts, stackmap */
+static int map_fd[2];
+struct bpf_program *prog;
 static bool sys_read_seen, sys_write_seen;

 static void print_ksym(__u64 addr)

@@ -137,9 +136,16 @@ static inline int generate_load(void)
 static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 {
        int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
+       struct bpf_link **link = malloc(nr_cpus * sizeof(struct bpf_link *));

same as above, calloc() is better choice here

Will apply this at next version patch.

quoted

        int *pmu_fd = malloc(nr_cpus * sizeof(int));
        int i, error = 0;

+       if (pmu_fd == NULL || link == NULL) {
+               printf("malloc of pmu_fd/link failed\n");
+               error = 1;
+               goto err;
+       }
+
        /* system wide perf event, no need to inherit */
        attr->inherit = 0;

@@ -151,8 +157,12 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
                        error = 1;
                        goto all_cpu_err;
                }
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE) == 0);
+               link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+               if (link[i] < 0) {
+                       printf("bpf_program__attach_perf_event failed\n");
+                       error = 1;
+                       goto all_cpu_err;
+               }
        }

        if (generate_load() < 0) {

@@ -161,11 +171,11 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
        }
        print_stacks();
 all_cpu_err:
-       for (i--; i >= 0; i--) {
-               ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
-               close(pmu_fd[i]);
-       }
+       for (i--; i >= 0; i--)
+               bpf_link__destroy(link[i]);

still need close(pmu_fd[i]);

AFAIK, bpf_link__detach_perf_event() closes the pmu_fd.
Am I missed something?

       static int bpf_link__detach_perf_event(struct bpf_link *link)
       // TRUNCATED
               close(link->fd);
               return err;
       }

quoted

+err:
        free(pmu_fd);
+       free(link);
        if (error)
                int_exit(0);

quoted

 }

@@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 static void test_perf_event_task(struct perf_event_attr *attr)
 {
        int pmu_fd, error = 0;
+       struct bpf_link *link;

        /* per task perf event, enable inherit so the "dd ..." command can be traced properly.
         * Enabling inherit will cause bpf_perf_prog_read_time helper failure.

@@ -185,8 +196,12 @@ static void test_perf_event_task(struct perf_event_attr *attr)
                printf("sys_perf_event_open failed\n");
                int_exit(0);
        }
-       assert(ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
-       assert(ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE) == 0);
+       link = bpf_program__attach_perf_event(prog, pmu_fd);
+       if (link < 0) {
+               printf("bpf_program__attach_perf_event failed\n");
+               close(pmu_fd);
+               int_exit(0);
+       }

        if (generate_load() < 0) {
                error = 1;

@@ -194,8 +209,7 @@ static void test_perf_event_task(struct perf_event_attr *attr)
        }
        print_stacks();
 err:
-       ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
-       close(pmu_fd);
+       bpf_link__destroy(link);
        if (error)
                int_exit(0);

This will exit with 0 error code and won't notify about error... Pass
through err?

You're right. Missed the return code.
Will apply this at next version patch.

quoted

 }

@@ -282,7 +296,9 @@ static void test_bpf_perf_event(void)
 int main(int argc, char **argv)
 {
        struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+       struct bpf_object *obj;
        char filename[256];
+       int prog_fd;

        snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
        setrlimit(RLIMIT_MEMLOCK, &r);

@@ -295,9 +311,20 @@ int main(int argc, char **argv)
                return 1;
        }

-       if (load_bpf_file(filename)) {
-               printf("%s", bpf_log_buf);
-               return 2;
+       if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
+               return 1;
+
+       prog = bpf_program__next(NULL, obj);
+       if (!prog) {
+               printf("finding a prog in obj file failed\n");
+               return 1;
+       }
+
+       map_fd[0] = bpf_object__find_map_fd_by_name(obj, "counts");
+       map_fd[1] = bpf_object__find_map_fd_by_name(obj, "stackmap");
+       if (map_fd[0] < 0 || map_fd[1] < 0) {
+               printf("finding a counts/stackmap map in obj file failed\n");
+               return 1;
        }

        if (fork() == 0) {
--

2.25.1

Thank you for your detailed review!

Best,
Daniel

On Wed, Mar 11, 2020 at 11:16 PM Daniel T. Lee [off-list ref] wrote:

On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee [off-list ref] wrote:

quoted

The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
than the previous method using ioctl.

bpf_program__attach_perf_event manages the enable of perf_event and
attach of BPF programs to it, so there's no neeed to do this
directly with ioctl.

In addition, bpf_link provides consistency in the use of API because it
allows disable (detach, destroy) for multiple events to be treated as
one bpf_link__destroy.

This commit refactors samples that attach the bpf program to perf_event
by using libbbpf instead of ioctl. Also the bpf_load in the samples were
removed and migrated to use libbbpf API.

Signed-off-by: Daniel T. Lee <redacted>
---

Daniel, thanks for this clean up! It's good to see samples be
modernized a bit :)

Thank you for your time and effort for the review :)

quoted

quoted

Changes in v2:
 - check memory allocation is successful
 - clean up allocated memory on error

 samples/bpf/Makefile           |  4 +-
 samples/bpf/sampleip_user.c    | 76 ++++++++++++++++++++++------------
 samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
 3 files changed, 97 insertions(+), 46 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index ff0061467dd3..424f6fe7ce38 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile

@@ -88,8 +88,8 @@ xdp2-objs := xdp1_user.o
 xdp_router_ipv4-objs := xdp_router_ipv4_user.o
 test_current_task_under_cgroup-objs := bpf_load.o $(CGROUP_HELPERS) \
                                       test_current_task_under_cgroup_user.o
-trace_event-objs := bpf_load.o trace_event_user.o $(TRACE_HELPERS)
-sampleip-objs := bpf_load.o sampleip_user.o $(TRACE_HELPERS)
+trace_event-objs := trace_event_user.o $(TRACE_HELPERS)
+sampleip-objs := sampleip_user.o $(TRACE_HELPERS)
 tc_l2_redirect-objs := bpf_load.o tc_l2_redirect_user.o
 lwt_len_hist-objs := bpf_load.o lwt_len_hist_user.o
 xdp_tx_iptunnel-objs := xdp_tx_iptunnel_user.o

diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
index b0f115f938bc..fd763a49c913 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c

@@ -10,13 +10,11 @@
 #include <errno.h>
 #include <signal.h>
 #include <string.h>
-#include <assert.h>
 #include <linux/perf_event.h>
 #include <linux/ptrace.h>
 #include <linux/bpf.h>
-#include <sys/ioctl.h>
+#include <bpf/bpf.h>
 #include <bpf/libbpf.h>
-#include "bpf_load.h"
 #include "perf-sys.h"
 #include "trace_helpers.h"

@@ -25,6 +23,7 @@
 #define MAX_IPS                8192
 #define PAGE_OFFSET    0xffff880000000000

+static int map_fd;
 static int nr_cpus;

 static void usage(void)

@@ -34,7 +33,8 @@ static void usage(void)
        printf("       duration   # sampling duration (seconds), default 5\n");
 }

-static int sampling_start(int *pmu_fd, int freq)
+static int sampling_start(int *pmu_fd, int freq, struct bpf_program *prog,
+                         struct bpf_link **link)

It's not apparent from looking at struct bpf_link **link whether it's
an output parameter (so sampling_start is supposed to assign *single*
link to return it to calling function) or it's an array of pointers.
Seems like it's the latter, so I'd prefer this written as

struct bpf_link *links[] (notice also plural name).

Please consider this.

This approach looks more apparent!
I'll update code using this way.

quoted

quoted

 {
        int i;

@@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
                        fprintf(stderr, "ERROR: Initializing perf sampling\n");
                        return 1;
                }
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF,
-                            prog_fd[0]) == 0);
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE, 0) == 0);
+               link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+               if (link[i] < 0) {

link is a pointer, < 0 doesn't make sense and is always going to be
false on x86. Use IS_ERR(link[i]). It's also a good idea to set it to
NULL, if link creation failed to prevent accidental
bpf_link__destroy(link[i]) later on, trying to free bogus pointer.

Failure on link creation is exactly what I was concerned about.
Thank you for giving me a clear solution!

quoted

quoted

+                       fprintf(stderr, "ERROR: Attach perf event\n");
+                       return 1;
+               }
        }

        return 0;
 }

-static void sampling_end(int *pmu_fd)
+static void sampling_end(struct bpf_link **link)

same as above, struct bpf_link *links[] would be much better here, IMO.

Also, I'll apply this at next version patch.

quoted

quoted

 {
        int i;

        for (i = 0; i < nr_cpus; i++)
-               close(pmu_fd[i]);
+               bpf_link__destroy(link[i]);
 }

 struct ipcount {

@@ -128,14 +130,18 @@ static void print_ip_map(int fd)
 static void int_exit(int sig)
 {
        printf("\n");
-       print_ip_map(map_fd[0]);
+       print_ip_map(map_fd);
        exit(0);
 }

 int main(int argc, char **argv)
 {
+       int prog_fd, *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
+       struct bpf_program *prog;
+       struct bpf_object *obj;
+       struct bpf_link **link;
        char filename[256];
-       int *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
+       int error = 0;

        /* process arguments */
        while ((opt = getopt(argc, argv, "F:h")) != -1) {

@@ -165,36 +171,54 @@ int main(int argc, char **argv)
        /* create perf FDs for each CPU */
        nr_cpus = sysconf(_SC_NPROCESSORS_CONF);

While neither approach is ideal, using number of online CPUs
(_SC_NPROCESSORS_ONLN) will probably work in slightly more cases
(there are machines configured with, say, 256 possible CPUs, but only
32 available, for instance).

Thank you for pointing me out!
I've never thought about situation when processors may be offline.

quoted

quoted

        pmu_fd = malloc(nr_cpus * sizeof(int));

similar naming nit: pmu_fds?

Same again, apply this at next version patch.

quoted

quoted

-       if (pmu_fd == NULL) {
-               fprintf(stderr, "ERROR: malloc of pmu_fd\n");
-               return 1;
+       link = malloc(nr_cpus * sizeof(struct bpf_link *));

Use calloc() to have those links initialized to NULL automatically.
Makes clean up so much easier.

About NULL set, like you mentioned, calloc approach looks more neat.

quoted

quoted

+       if (pmu_fd == NULL || link == NULL) {
+               fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
+               error = 1;
+               goto cleanup;
        }

        /* load BPF program */
        snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
-       if (load_bpf_file(filename)) {
+       if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd)) {

Using skeleton would be best, but it's probably more appropriate for
another patch to integrate skeleton generation with samples/bpf. So
the next one would be bpf_object__open_file(), instead of legacy
bpf_prog_load().

I'll try skeleton with other sample cleanup. For now, I'll stick with
bpf_object__{open/load}() instead of bpf_prog_load().

quoted

quoted

                fprintf(stderr, "ERROR: loading BPF program (errno %d):\n",
                        errno);
-               if (strcmp(bpf_log_buf, "") == 0)
-                       fprintf(stderr, "Try: ulimit -l unlimited\n");
-               else
-                       fprintf(stderr, "%s", bpf_log_buf);
-               return 1;
+               error = 1;
+               goto cleanup;
+       }
+
+       prog = bpf_program__next(NULL, obj);

I'm a bit lazy here, sorry, but isn't the name of the program known?
bpf_object__find_program_by_title() is preferable.

I also think it is good to specify the program title clearly.

quoted

quoted

+       if (!prog) {
+               fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
+               error = 1;
+               goto cleanup;
+       }
+
+       map_fd = bpf_object__find_map_fd_by_name(obj, "ip_map");
+       if (map_fd < 0) {
+               fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+               error = 1;
+               goto cleanup;
        }
+
        signal(SIGINT, int_exit);
        signal(SIGTERM, int_exit);

        /* do sampling */
        printf("Sampling at %d Hertz for %d seconds. Ctrl-C also ends.\n",
               freq, secs);
-       if (sampling_start(pmu_fd, freq) != 0)
-               return 1;
+       if (sampling_start(pmu_fd, freq, prog, link) != 0) {
+               error = 1;
+               goto cleanup;
+       }
        sleep(secs);
-       sampling_end(pmu_fd);
-       free(pmu_fd);
+       sampling_end(link);

        /* output sample counts */
-       print_ip_map(map_fd[0]);
+       print_ip_map(map_fd);

-       return 0;
+cleanup:
+       free(pmu_fd);
+       free(link);

Uhm... you are freeing this only on clean up. Also, you need to
bpf_link__destroy() first. And close all pmu_fds. Surely process exit
will ensure all this is cleaned up, but it's a good tone to clean up
all resources explicitly.

Well, cleanup: could cover link destroy (sampling_end), but I think
it is strange to clean up the link even though the bpf program is not
attached to the event. I think it is better to specify the link destroy
after the sampling starts.

bpf_link__destroy() is designed in such a way that if passed NULL it
will do nothing. So doing unconditional clean up at the end is clean
and straightforward solution. You'll see it in a bunch of places in
selftests.

And, I've missed the link destroy when sampling got error.
Since sampling_end will destroy the links, so I'll add this on error.

       if (sampling_start(pmu_fd, freq, prog, link) != 0) {
               error = 1;
+             sampling_end(links);
               goto cleanup;
       }

quoted

quoted

+       return error;
 }

diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index 356171bc392b..30c25ef99fc5 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c

@@ -6,22 +6,21 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <string.h>
-#include <fcntl.h>
-#include <poll.h>
-#include <sys/ioctl.h>
 #include <linux/perf_event.h>
 #include <linux/bpf.h>
 #include <signal.h>
-#include <assert.h>
 #include <errno.h>
 #include <sys/resource.h>
+#include <bpf/bpf.h>
 #include <bpf/libbpf.h>
-#include "bpf_load.h"
 #include "perf-sys.h"
 #include "trace_helpers.h"

 #define SAMPLE_FREQ 50

+/* counts, stackmap */
+static int map_fd[2];
+struct bpf_program *prog;
 static bool sys_read_seen, sys_write_seen;

 static void print_ksym(__u64 addr)

@@ -137,9 +136,16 @@ static inline int generate_load(void)
 static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 {
        int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
+       struct bpf_link **link = malloc(nr_cpus * sizeof(struct bpf_link *));

same as above, calloc() is better choice here

Will apply this at next version patch.

quoted

quoted

        int *pmu_fd = malloc(nr_cpus * sizeof(int));
        int i, error = 0;

+       if (pmu_fd == NULL || link == NULL) {
+               printf("malloc of pmu_fd/link failed\n");
+               error = 1;
+               goto err;
+       }
+
        /* system wide perf event, no need to inherit */
        attr->inherit = 0;

@@ -151,8 +157,12 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
                        error = 1;
                        goto all_cpu_err;
                }
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE) == 0);
+               link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+               if (link[i] < 0) {
+                       printf("bpf_program__attach_perf_event failed\n");
+                       error = 1;
+                       goto all_cpu_err;
+               }
        }

        if (generate_load() < 0) {

@@ -161,11 +171,11 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
        }
        print_stacks();
 all_cpu_err:
-       for (i--; i >= 0; i--) {
-               ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
-               close(pmu_fd[i]);
-       }
+       for (i--; i >= 0; i--)
+               bpf_link__destroy(link[i]);

still need close(pmu_fd[i]);

AFAIK, bpf_link__detach_perf_event() closes the pmu_fd.
Am I missed something?

Ah, you are right, I missed that fact. But then you don't need pmu_fd
array at all. Do perf_event_open and then immediately
bpf_program__attach_perf_event().

       static int bpf_link__detach_perf_event(struct bpf_link *link)
       // TRUNCATED
               close(link->fd);
               return err;
       }

quoted

quoted

+err:
        free(pmu_fd);
+       free(link);
        if (error)
                int_exit(0);

quoted

 }

@@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 static void test_perf_event_task(struct perf_event_attr *attr)
 {
        int pmu_fd, error = 0;
+       struct bpf_link *link;

        /* per task perf event, enable inherit so the "dd ..." command can be traced properly.
         * Enabling inherit will cause bpf_perf_prog_read_time helper failure.

@@ -185,8 +196,12 @@ static void test_perf_event_task(struct perf_event_attr *attr)
                printf("sys_perf_event_open failed\n");
                int_exit(0);
        }
-       assert(ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
-       assert(ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE) == 0);
+       link = bpf_program__attach_perf_event(prog, pmu_fd);
+       if (link < 0) {
+               printf("bpf_program__attach_perf_event failed\n");
+               close(pmu_fd);
+               int_exit(0);
+       }

        if (generate_load() < 0) {
                error = 1;

@@ -194,8 +209,7 @@ static void test_perf_event_task(struct perf_event_attr *attr)
        }
        print_stacks();
 err:
-       ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
-       close(pmu_fd);
+       bpf_link__destroy(link);
        if (error)
                int_exit(0);

This will exit with 0 error code and won't notify about error... Pass
through err?

You're right. Missed the return code.
Will apply this at next version patch.

quoted

quoted

 }

@@ -282,7 +296,9 @@ static void test_bpf_perf_event(void)
 int main(int argc, char **argv)
 {
        struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+       struct bpf_object *obj;
        char filename[256];
+       int prog_fd;

        snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
        setrlimit(RLIMIT_MEMLOCK, &r);

@@ -295,9 +311,20 @@ int main(int argc, char **argv)
                return 1;
        }

-       if (load_bpf_file(filename)) {
-               printf("%s", bpf_log_buf);
-               return 2;
+       if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
+               return 1;
+
+       prog = bpf_program__next(NULL, obj);
+       if (!prog) {
+               printf("finding a prog in obj file failed\n");
+               return 1;
+       }
+
+       map_fd[0] = bpf_object__find_map_fd_by_name(obj, "counts");
+       map_fd[1] = bpf_object__find_map_fd_by_name(obj, "stackmap");
+       if (map_fd[0] < 0 || map_fd[1] < 0) {
+               printf("finding a counts/stackmap map in obj file failed\n");
+               return 1;
        }

        if (fork() == 0) {
--

2.25.1

Thank you for your detailed review!

Best,
Daniel

On Thu, Mar 12, 2020 at 3:27 PM Andrii Nakryiko
[off-list ref] wrote:

On Wed, Mar 11, 2020 at 11:16 PM Daniel T. Lee [off-list ref] wrote:

quoted

On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee [off-list ref] wrote:

quoted

The bpf_program__attach of libbpf(using bpf_link) is much more intuitive
than the previous method using ioctl.

bpf_program__attach_perf_event manages the enable of perf_event and
attach of BPF programs to it, so there's no neeed to do this
directly with ioctl.

In addition, bpf_link provides consistency in the use of API because it
allows disable (detach, destroy) for multiple events to be treated as
one bpf_link__destroy.

This commit refactors samples that attach the bpf program to perf_event
by using libbbpf instead of ioctl. Also the bpf_load in the samples were
removed and migrated to use libbbpf API.

Signed-off-by: Daniel T. Lee <redacted>
---

Daniel, thanks for this clean up! It's good to see samples be
modernized a bit :)

Thank you for your time and effort for the review :)

quoted

quoted

Changes in v2:
 - check memory allocation is successful
 - clean up allocated memory on error

 samples/bpf/Makefile           |  4 +-
 samples/bpf/sampleip_user.c    | 76 ++++++++++++++++++++++------------
 samples/bpf/trace_event_user.c | 63 ++++++++++++++++++++--------
 3 files changed, 97 insertions(+), 46 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index ff0061467dd3..424f6fe7ce38 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile

@@ -88,8 +88,8 @@ xdp2-objs := xdp1_user.o
 xdp_router_ipv4-objs := xdp_router_ipv4_user.o
 test_current_task_under_cgroup-objs := bpf_load.o $(CGROUP_HELPERS) \
                                       test_current_task_under_cgroup_user.o
-trace_event-objs := bpf_load.o trace_event_user.o $(TRACE_HELPERS)
-sampleip-objs := bpf_load.o sampleip_user.o $(TRACE_HELPERS)
+trace_event-objs := trace_event_user.o $(TRACE_HELPERS)
+sampleip-objs := sampleip_user.o $(TRACE_HELPERS)
 tc_l2_redirect-objs := bpf_load.o tc_l2_redirect_user.o
 lwt_len_hist-objs := bpf_load.o lwt_len_hist_user.o
 xdp_tx_iptunnel-objs := xdp_tx_iptunnel_user.o

diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
index b0f115f938bc..fd763a49c913 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c

@@ -10,13 +10,11 @@
 #include <errno.h>
 #include <signal.h>
 #include <string.h>
-#include <assert.h>
 #include <linux/perf_event.h>
 #include <linux/ptrace.h>
 #include <linux/bpf.h>
-#include <sys/ioctl.h>
+#include <bpf/bpf.h>
 #include <bpf/libbpf.h>
-#include "bpf_load.h"
 #include "perf-sys.h"
 #include "trace_helpers.h"

@@ -25,6 +23,7 @@
 #define MAX_IPS                8192
 #define PAGE_OFFSET    0xffff880000000000

+static int map_fd;
 static int nr_cpus;

 static void usage(void)

@@ -34,7 +33,8 @@ static void usage(void)
        printf("       duration   # sampling duration (seconds), default 5\n");
 }

-static int sampling_start(int *pmu_fd, int freq)
+static int sampling_start(int *pmu_fd, int freq, struct bpf_program *prog,
+                         struct bpf_link **link)

It's not apparent from looking at struct bpf_link **link whether it's
an output parameter (so sampling_start is supposed to assign *single*
link to return it to calling function) or it's an array of pointers.
Seems like it's the latter, so I'd prefer this written as

struct bpf_link *links[] (notice also plural name).

Please consider this.

This approach looks more apparent!
I'll update code using this way.

quoted

quoted

 {
        int i;

@@ -53,20 +53,22 @@ static int sampling_start(int *pmu_fd, int freq)
                        fprintf(stderr, "ERROR: Initializing perf sampling\n");
                        return 1;
                }
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF,
-                            prog_fd[0]) == 0);
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE, 0) == 0);
+               link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+               if (link[i] < 0) {

link is a pointer, < 0 doesn't make sense and is always going to be
false on x86. Use IS_ERR(link[i]). It's also a good idea to set it to
NULL, if link creation failed to prevent accidental
bpf_link__destroy(link[i]) later on, trying to free bogus pointer.

Failure on link creation is exactly what I was concerned about.
Thank you for giving me a clear solution!

quoted

quoted

+                       fprintf(stderr, "ERROR: Attach perf event\n");
+                       return 1;
+               }
        }

        return 0;
 }

-static void sampling_end(int *pmu_fd)
+static void sampling_end(struct bpf_link **link)

same as above, struct bpf_link *links[] would be much better here, IMO.

Also, I'll apply this at next version patch.

quoted

quoted

 {
        int i;

        for (i = 0; i < nr_cpus; i++)
-               close(pmu_fd[i]);
+               bpf_link__destroy(link[i]);
 }

 struct ipcount {

@@ -128,14 +130,18 @@ static void print_ip_map(int fd)
 static void int_exit(int sig)
 {
        printf("\n");
-       print_ip_map(map_fd[0]);
+       print_ip_map(map_fd);
        exit(0);
 }

 int main(int argc, char **argv)
 {
+       int prog_fd, *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
+       struct bpf_program *prog;
+       struct bpf_object *obj;
+       struct bpf_link **link;
        char filename[256];
-       int *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;
+       int error = 0;

        /* process arguments */
        while ((opt = getopt(argc, argv, "F:h")) != -1) {

@@ -165,36 +171,54 @@ int main(int argc, char **argv)
        /* create perf FDs for each CPU */
        nr_cpus = sysconf(_SC_NPROCESSORS_CONF);

While neither approach is ideal, using number of online CPUs
(_SC_NPROCESSORS_ONLN) will probably work in slightly more cases
(there are machines configured with, say, 256 possible CPUs, but only
32 available, for instance).

Thank you for pointing me out!
I've never thought about situation when processors may be offline.

quoted

quoted

        pmu_fd = malloc(nr_cpus * sizeof(int));

similar naming nit: pmu_fds?

Same again, apply this at next version patch.

quoted

quoted

-       if (pmu_fd == NULL) {
-               fprintf(stderr, "ERROR: malloc of pmu_fd\n");
-               return 1;
+       link = malloc(nr_cpus * sizeof(struct bpf_link *));

Use calloc() to have those links initialized to NULL automatically.
Makes clean up so much easier.

About NULL set, like you mentioned, calloc approach looks more neat.

quoted

quoted

+       if (pmu_fd == NULL || link == NULL) {
+               fprintf(stderr, "ERROR: malloc of pmu_fd/link\n");
+               error = 1;
+               goto cleanup;
        }

        /* load BPF program */
        snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
-       if (load_bpf_file(filename)) {
+       if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd)) {

Using skeleton would be best, but it's probably more appropriate for
another patch to integrate skeleton generation with samples/bpf. So
the next one would be bpf_object__open_file(), instead of legacy
bpf_prog_load().

I'll try skeleton with other sample cleanup. For now, I'll stick with
bpf_object__{open/load}() instead of bpf_prog_load().

quoted

quoted

                fprintf(stderr, "ERROR: loading BPF program (errno %d):\n",
                        errno);
-               if (strcmp(bpf_log_buf, "") == 0)
-                       fprintf(stderr, "Try: ulimit -l unlimited\n");
-               else
-                       fprintf(stderr, "%s", bpf_log_buf);
-               return 1;
+               error = 1;
+               goto cleanup;
+       }
+
+       prog = bpf_program__next(NULL, obj);

I'm a bit lazy here, sorry, but isn't the name of the program known?
bpf_object__find_program_by_title() is preferable.

I also think it is good to specify the program title clearly.

quoted

quoted

+       if (!prog) {
+               fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
+               error = 1;
+               goto cleanup;
+       }
+
+       map_fd = bpf_object__find_map_fd_by_name(obj, "ip_map");
+       if (map_fd < 0) {
+               fprintf(stderr, "ERROR: finding a map in obj file failed\n");
+               error = 1;
+               goto cleanup;
        }
+
        signal(SIGINT, int_exit);
        signal(SIGTERM, int_exit);

        /* do sampling */
        printf("Sampling at %d Hertz for %d seconds. Ctrl-C also ends.\n",
               freq, secs);
-       if (sampling_start(pmu_fd, freq) != 0)
-               return 1;
+       if (sampling_start(pmu_fd, freq, prog, link) != 0) {
+               error = 1;
+               goto cleanup;
+       }
        sleep(secs);
-       sampling_end(pmu_fd);
-       free(pmu_fd);
+       sampling_end(link);

        /* output sample counts */
-       print_ip_map(map_fd[0]);
+       print_ip_map(map_fd);

-       return 0;
+cleanup:
+       free(pmu_fd);
+       free(link);

Uhm... you are freeing this only on clean up. Also, you need to
bpf_link__destroy() first. And close all pmu_fds. Surely process exit
will ensure all this is cleaned up, but it's a good tone to clean up
all resources explicitly.

Well, cleanup: could cover link destroy (sampling_end), but I think
it is strange to clean up the link even though the bpf program is not
attached to the event. I think it is better to specify the link destroy
after the sampling starts.

bpf_link__destroy() is designed in such a way that if passed NULL it
will do nothing. So doing unconditional clean up at the end is clean
and straightforward solution. You'll see it in a bunch of places in
selftests.

I see. I will move this to cleanup: for an unconditional clean up at end.

quoted

And, I've missed the link destroy when sampling got error.
Since sampling_end will destroy the links, so I'll add this on error.

       if (sampling_start(pmu_fd, freq, prog, link) != 0) {
               error = 1;
+             sampling_end(links);
               goto cleanup;
       }

quoted

quoted

+       return error;
 }

diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c
index 356171bc392b..30c25ef99fc5 100644
--- a/samples/bpf/trace_event_user.c
+++ b/samples/bpf/trace_event_user.c

@@ -6,22 +6,21 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <string.h>
-#include <fcntl.h>
-#include <poll.h>
-#include <sys/ioctl.h>
 #include <linux/perf_event.h>
 #include <linux/bpf.h>
 #include <signal.h>
-#include <assert.h>
 #include <errno.h>
 #include <sys/resource.h>
+#include <bpf/bpf.h>
 #include <bpf/libbpf.h>
-#include "bpf_load.h"
 #include "perf-sys.h"
 #include "trace_helpers.h"

 #define SAMPLE_FREQ 50

+/* counts, stackmap */
+static int map_fd[2];
+struct bpf_program *prog;
 static bool sys_read_seen, sys_write_seen;

 static void print_ksym(__u64 addr)

@@ -137,9 +136,16 @@ static inline int generate_load(void)
 static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 {
        int nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
+       struct bpf_link **link = malloc(nr_cpus * sizeof(struct bpf_link *));

same as above, calloc() is better choice here

Will apply this at next version patch.

quoted

quoted

        int *pmu_fd = malloc(nr_cpus * sizeof(int));
        int i, error = 0;

+       if (pmu_fd == NULL || link == NULL) {
+               printf("malloc of pmu_fd/link failed\n");
+               error = 1;
+               goto err;
+       }
+
        /* system wide perf event, no need to inherit */
        attr->inherit = 0;

@@ -151,8 +157,12 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
                        error = 1;
                        goto all_cpu_err;
                }
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
-               assert(ioctl(pmu_fd[i], PERF_EVENT_IOC_ENABLE) == 0);
+               link[i] = bpf_program__attach_perf_event(prog, pmu_fd[i]);
+               if (link[i] < 0) {
+                       printf("bpf_program__attach_perf_event failed\n");
+                       error = 1;
+                       goto all_cpu_err;
+               }
        }

        if (generate_load() < 0) {

@@ -161,11 +171,11 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
        }
        print_stacks();
 all_cpu_err:
-       for (i--; i >= 0; i--) {
-               ioctl(pmu_fd[i], PERF_EVENT_IOC_DISABLE);
-               close(pmu_fd[i]);
-       }
+       for (i--; i >= 0; i--)
+               bpf_link__destroy(link[i]);

still need close(pmu_fd[i]);

AFAIK, bpf_link__detach_perf_event() closes the pmu_fd.
Am I missed something?

Ah, you are right, I missed that fact. But then you don't need pmu_fd
array at all. Do perf_event_open and then immediately
bpf_program__attach_perf_event().

Right. I won't need to handle pmu_fds, since bpf_link manages all of it.
Thanks for the tip!

quoted

       static int bpf_link__detach_perf_event(struct bpf_link *link)
       // TRUNCATED
               close(link->fd);
               return err;
       }

quoted

quoted

+err:
        free(pmu_fd);
+       free(link);
        if (error)
                int_exit(0);

quoted

 }

@@ -173,6 +183,7 @@ static void test_perf_event_all_cpu(struct perf_event_attr *attr)
 static void test_perf_event_task(struct perf_event_attr *attr)
 {
        int pmu_fd, error = 0;
+       struct bpf_link *link;

        /* per task perf event, enable inherit so the "dd ..." command can be traced properly.
         * Enabling inherit will cause bpf_perf_prog_read_time helper failure.

@@ -185,8 +196,12 @@ static void test_perf_event_task(struct perf_event_attr *attr)
                printf("sys_perf_event_open failed\n");
                int_exit(0);
        }
-       assert(ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd[0]) == 0);
-       assert(ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE) == 0);
+       link = bpf_program__attach_perf_event(prog, pmu_fd);
+       if (link < 0) {
+               printf("bpf_program__attach_perf_event failed\n");
+               close(pmu_fd);
+               int_exit(0);
+       }

        if (generate_load() < 0) {
                error = 1;

@@ -194,8 +209,7 @@ static void test_perf_event_task(struct perf_event_attr *attr)
        }
        print_stacks();
 err:
-       ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
-       close(pmu_fd);
+       bpf_link__destroy(link);
        if (error)
                int_exit(0);

This will exit with 0 error code and won't notify about error... Pass
through err?

You're right. Missed the return code.
Will apply this at next version patch.

quoted

quoted

 }

@@ -282,7 +296,9 @@ static void test_bpf_perf_event(void)
 int main(int argc, char **argv)
 {
        struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+       struct bpf_object *obj;
        char filename[256];
+       int prog_fd;

        snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
        setrlimit(RLIMIT_MEMLOCK, &r);

@@ -295,9 +311,20 @@ int main(int argc, char **argv)
                return 1;
        }

-       if (load_bpf_file(filename)) {
-               printf("%s", bpf_log_buf);
-               return 2;
+       if (bpf_prog_load(filename, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd))
+               return 1;
+
+       prog = bpf_program__next(NULL, obj);
+       if (!prog) {
+               printf("finding a prog in obj file failed\n");
+               return 1;
+       }
+
+       map_fd[0] = bpf_object__find_map_fd_by_name(obj, "counts");
+       map_fd[1] = bpf_object__find_map_fd_by_name(obj, "stackmap");
+       if (map_fd[0] < 0 || map_fd[1] < 0) {
+               printf("finding a counts/stackmap map in obj file failed\n");
+               return 1;
        }

        if (fork() == 0) {
--

2.25.1

Thank you for your detailed review!

Best,
Daniel

Thanks for the super fast response!

Best,
Daniel