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

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

[Daniel T. Lee]

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

[PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers

[Daniel T. Lee]

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 <danieltimlee@gmail.com>
---
 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

[PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link

[Daniel T. Lee]

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 <danieltimlee@gmail.com>
---
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

Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link

[Andrii Nakryiko]

On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee <danieltimlee@gmail.com> 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 <danieltimlee@gmail.com>
> ---

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

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

>  {
>         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.

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

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


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

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

>         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]);

> +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);

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

>  }
> @@ -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
>

Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link

[Daniel T. Lee]

On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee <danieltimlee@gmail.com> 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 <danieltimlee@gmail.com>
> > ---
>
> 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 :)

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

> >  {
> >         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!

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

> >  {
> >         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.

>
> >         pmu_fd = malloc(nr_cpus * sizeof(int));
>
> similar naming nit: pmu_fds?
>

Same again, apply this at next version patch.

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

> > +       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().

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

> > +       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;
       }

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

> >         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;
       }

> > +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);
>
> 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.

> >  }
> > @@ -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

Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link

[Andrii Nakryiko]

On Wed, Mar 11, 2020 at 11:16 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee <danieltimlee@gmail.com> 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 <danieltimlee@gmail.com>
> > > ---
> >
> > 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 :)
>
> > > 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.
>
> > >  {
> > >         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!
>
> > > +                       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.
>
> > >  {
> > >         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.
>
> >
> > >         pmu_fd = malloc(nr_cpus * sizeof(int));
> >
> > similar naming nit: pmu_fds?
> >
>
> Same again, apply this at next version patch.
>
> > > -       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.
>
> > > +       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().
>
> > >                 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.
>
> > > +       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;
>        }
>
> > > +       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.
>
> > >         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;
>        }
>
> > > +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);
> >
> > 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.
>
> > >  }
> > > @@ -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

Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link

[Daniel T. Lee]

On Thu, Mar 12, 2020 at 3:27 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Mar 11, 2020 at 11:16 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > On Wed, Mar 11, 2020 at 1:55 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Tue, Mar 10, 2020 at 4:27 PM Daniel T. Lee <danieltimlee@gmail.com> 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 <danieltimlee@gmail.com>
> > > > ---
> > >
> > > 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 :)
> >
> > > > 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.
> >
> > > >  {
> > > >         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!
> >
> > > > +                       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.
> >
> > > >  {
> > > >         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.
> >
> > >
> > > >         pmu_fd = malloc(nr_cpus * sizeof(int));
> > >
> > > similar naming nit: pmu_fds?
> > >
> >
> > Same again, apply this at next version patch.
> >
> > > > -       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.
> >
> > > > +       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().
> >
> > > >                 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.
> >
> > > > +       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.

> >
> > 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;
> >        }
> >
> > > > +       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.
> >
> > > >         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!

> >
> >        static int bpf_link__detach_perf_event(struct bpf_link *link)
> >        // TRUNCATED
> >                close(link->fd);
> >                return err;
> >        }
> >
> > > > +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);
> > >
> > > 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.
> >
> > > >  }
> > > > @@ -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