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

On Sat, Mar 14, 2020 at 3:48 AM Andrii Nakryiko
[off-list ref] wrote:

On Fri, Mar 13, 2020 at 4:52 AM 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. Also, bpf_link__destroy manages the close() of
perf_event fd.

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

Changes in v3:
 - Improve pointer error check (IS_ERR())
 - change to calloc for easier destroy of bpf_link
 - remove perf_event fd list since bpf_link handles fd
 - use newer bpf_object__{open/load} API instead of bpf_prog_load
 - perf_event for _SC_NPROCESSORS_ONLN instead of _SC_NPROCESSORS_CONF
 - find program with name explicitly instead of bpf_program__next
 - unconditional bpf_link__destroy() on cleanup
 - return error code on int_exit

 samples/bpf/Makefile           |   4 +-
 samples/bpf/sampleip_user.c    | 100 ++++++++++++++++++++++-----------
 samples/bpf/trace_event_user.c |  89 ++++++++++++++++++++---------
 3 files changed, 131 insertions(+), 62 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..05eca7b00e22 100644
--- a/samples/bpf/sampleip_user.c
+++ b/samples/bpf/sampleip_user.c

@@ -10,21 +10,23 @@
 #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"

+#define __must_check
+#include <linux/err.h>
+
 #define DEFAULT_FREQ   99
 #define DEFAULT_SECS   5
 #define MAX_IPS                8192
 #define PAGE_OFFSET    0xffff880000000000

+static int map_fd;
 static int nr_cpus;

 static void usage(void)

@@ -34,9 +36,10 @@ 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 freq, struct bpf_program *prog,
+                         struct bpf_link *links[])
 {
-       int i;
+       int i, pmu_fd;

        struct perf_event_attr pe_sample_attr = {
                .type = PERF_TYPE_SOFTWARE,

@@ -47,26 +50,29 @@ static int sampling_start(int *pmu_fd, int freq)
        };

        for (i = 0; i < nr_cpus; i++) {
-               pmu_fd[i] = sys_perf_event_open(&pe_sample_attr, -1 /* pid */, i,
+               pmu_fd = sys_perf_event_open(&pe_sample_attr, -1 /* pid */, i,
                                            -1 /* group_fd */, 0 /* flags */);
-               if (pmu_fd[i] < 0) {
+               if (pmu_fd < 0) {
                        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);
+               links[i] = bpf_program__attach_perf_event(prog, pmu_fd);
+               if (IS_ERR(links[i])) {

links[i] = NULL;

Since bpf_program__attach_perf_event will return err code such
as ERR_PTR(-EINVAL), so it is natural to reset the pointer to NULL!
Thank you for the heads up!

quoted

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

        return 0;
 }

-static void sampling_end(int *pmu_fd)
+static void sampling_end(struct bpf_link *links[])
 {
        int i;

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

 struct ipcount {

@@ -128,14 +134,17 @@ 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 opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS, error = 0;
+       struct bpf_program *prog;
+       struct bpf_object *obj;

initialize to NULL here

I'll apply this at next version patch.

quoted

+       struct bpf_link **links;
        char filename[256];
-       int *pmu_fd, opt, freq = DEFAULT_FREQ, secs = DEFAULT_SECS;

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

@@ -163,38 +172,61 @@ 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;
+       nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+       links = calloc(nr_cpus, sizeof(struct bpf_link *));
+       if (!links) {
+               fprintf(stderr, "ERROR: malloc of links\n");
+               error = 1;
+               goto cleanup;
        }

-       /* load BPF program */
        snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
-       if (load_bpf_file(filename)) {
-               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;
+       obj = bpf_object__open_file(filename, NULL);
+       if (IS_ERR(obj)) {

obj = NULL;

Also again, bpf_object__open_file will return err code of pointer,
and the pointer needs to be set to NULL for bpf_object__close.

quoted

+               fprintf(stderr, "ERROR: opening BPF object file failed\n");
+               error = 1;
+               goto cleanup;
+       }
+
+       prog = bpf_object__find_program_by_name(obj, "do_sample");
+       if (!prog) {
+               fprintf(stderr, "ERROR: finding a prog in obj file failed\n");
+               error = 1;
+               goto cleanup;
        }
+
+       /* load BPF program */
+       if (bpf_object__load(obj)) {
+               fprintf(stderr, "ERROR: loading BPF object 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(freq, prog, links) != 0) {
+               error = 1;
+               goto cleanup;
+       }
        sleep(secs);
-       sampling_end(pmu_fd);
-       free(pmu_fd);

+cleanup:
+       sampling_end(links);

bpf_object__destroy(obj)

Sorry, but I might not following you, but is there bpf_object__destroy
exist in libbpf? I guess you're talking about bpf_object__close, right?

quoted

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

-       return 0;
+       free(links);
+       return error;
 }

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

@@ -6,22 +6,24 @@
 #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 __must_check
+#include <linux/err.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)

@@ -136,23 +138,34 @@ 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);
-       int *pmu_fd = malloc(nr_cpus * sizeof(int));
-       int i, error = 0;
+       int nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+       struct bpf_link **links = calloc(nr_cpus, sizeof(struct bpf_link *));
+       int i, pmu_fd, error = 0;
+
+       if (!links) {
+               printf("malloc of links failed\n");
+               error = 1;
+               goto err;
+       }

        /* system wide perf event, no need to inherit */
        attr->inherit = 0;

        /* open perf_event on all cpus */
        for (i = 0; i < nr_cpus; i++) {
-               pmu_fd[i] = sys_perf_event_open(attr, -1, i, -1, 0);
-               if (pmu_fd[i] < 0) {
+               pmu_fd = sys_perf_event_open(attr, -1, i, -1, 0);
+               if (pmu_fd < 0) {
                        printf("sys_perf_event_open failed\n");
                        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);
+               links[i] = bpf_program__attach_perf_event(prog, pmu_fd);
+               if (IS_ERR(links[i])) {
+                       printf("bpf_program__attach_perf_event failed\n");
+                       close(pmu_fd);
+                       error = 1;
+                       goto all_cpu_err;
+               }
        }

        if (generate_load() < 0) {

@@ -161,18 +174,18 @@ 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]);
-       }
-       free(pmu_fd);
+       for (i--; i >= 0; i--)
+               bpf_link__destroy(links[i]);
+err:
+       free(links);
        if (error)
-               int_exit(0);
+               int_exit(error);
 }

 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 +198,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 (IS_ERR(link)) {
+               printf("bpf_program__attach_perf_event failed\n");
+               close(pmu_fd);
+               int_exit(0);
+       }

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

@@ -194,10 +211,9 @@ 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);
+               int_exit(error);
 }

 static void test_bpf_perf_event(void)

@@ -282,6 +298,7 @@ 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];

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

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

-       if (load_bpf_file(filename)) {
-               printf("%s", bpf_log_buf);
-               return 2;
+       obj = bpf_object__open_file(filename, NULL);
+       if (IS_ERR(obj)) {
+               printf("opening BPF object file failed\n");
+               return 1;
+       }
+
+       prog = bpf_object__find_program_by_name(obj, "bpf_prog1");
+       if (!prog) {
+               printf("finding a prog in obj file failed\n");

bpf_object__close(obj);

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

quoted

+               return 1;
+       }
+
+       /* load BPF program */
+       if (bpf_object__load(obj)) {

close bpf_object (better do goto clean approach, of course)

I will modify this with goto cleanup approach.

quoted

+               printf("loading BPF object 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 time and effort for the review :)

Best,
Daniel