* [PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link @ 2020-03-10 23:26 Daniel T. Lee 2020-03-10 23:26 ` [PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers Daniel T. Lee 2020-03-10 23:26 ` [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee 0 siblings, 2 replies; 7+ messages in thread From: Daniel T. Lee @ 2020-03-10 23:26 UTC (permalink / raw) To: Daniel Borkmann, Alexei Starovoitov Cc: John Fastabend, Andrii Nakryiko, netdev, bpf 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers 2020-03-10 23:26 [PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link Daniel T. Lee @ 2020-03-10 23:26 ` Daniel T. Lee 2020-03-10 23:26 ` [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee 1 sibling, 0 replies; 7+ messages in thread From: Daniel T. Lee @ 2020-03-10 23:26 UTC (permalink / raw) To: Daniel Borkmann, Alexei Starovoitov Cc: John Fastabend, Andrii Nakryiko, netdev, bpf 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link 2020-03-10 23:26 [PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link Daniel T. Lee 2020-03-10 23:26 ` [PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers Daniel T. Lee @ 2020-03-10 23:26 ` Daniel T. Lee 2020-03-11 4:55 ` Andrii Nakryiko 1 sibling, 1 reply; 7+ messages in thread From: Daniel T. Lee @ 2020-03-10 23:26 UTC (permalink / raw) To: Daniel Borkmann, Alexei Starovoitov Cc: John Fastabend, Andrii Nakryiko, netdev, bpf 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link 2020-03-10 23:26 ` [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee @ 2020-03-11 4:55 ` Andrii Nakryiko 2020-03-12 6:15 ` Daniel T. Lee 0 siblings, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2020-03-11 4:55 UTC (permalink / raw) To: Daniel T. Lee Cc: Daniel Borkmann, Alexei Starovoitov, John Fastabend, Networking, bpf 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link 2020-03-11 4:55 ` Andrii Nakryiko @ 2020-03-12 6:15 ` Daniel T. Lee 2020-03-12 6:27 ` Andrii Nakryiko 0 siblings, 1 reply; 7+ messages in thread From: Daniel T. Lee @ 2020-03-12 6:15 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, John Fastabend, Networking, bpf 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link 2020-03-12 6:15 ` Daniel T. Lee @ 2020-03-12 6:27 ` Andrii Nakryiko 2020-03-12 6:35 ` Daniel T. Lee 0 siblings, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2020-03-12 6:27 UTC (permalink / raw) To: Daniel T. Lee Cc: Daniel Borkmann, Alexei Starovoitov, John Fastabend, Networking, bpf 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link 2020-03-12 6:27 ` Andrii Nakryiko @ 2020-03-12 6:35 ` Daniel T. Lee 0 siblings, 0 replies; 7+ messages in thread From: Daniel T. Lee @ 2020-03-12 6:35 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, John Fastabend, Networking, bpf 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-12 6:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 23:26 [PATCH bpf-next v2 0/2] Refactor perf_event sample user program with libbpf bpf_link Daniel T. Lee 2020-03-10 23:26 ` [PATCH bpf-next v2 1/2] samples: bpf: move read_trace_pipe to trace_helpers Daniel T. Lee 2020-03-10 23:26 ` [PATCH bpf-next v2 2/2] samples: bpf: refactor perf_event user program with libbpf bpf_link Daniel T. Lee 2020-03-11 4:55 ` Andrii Nakryiko 2020-03-12 6:15 ` Daniel T. Lee 2020-03-12 6:27 ` Andrii Nakryiko 2020-03-12 6:35 ` Daniel T. Lee
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).