| Message ID | 20201117145644.1166255-6-danieltimlee@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bpf: remove bpf_load loader completely | expand |
On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > This commit refactors the existing ibumad program with libbpf bpf > loader. Attach/detach of Tracepoint bpf programs has been managed > with the generic bpf_program__attach() and bpf_link__destroy() from > the libbpf. > > Also, instead of using the previous BPF MAP definition, this commit > refactors ibumad MAP definition with the new BTF-defined MAP format. > > To verify that this bpf program works without an infiniband device, > try loading ib_umad kernel module and test the program as follows: > > # modprobe ib_umad > # ./ibumad > > Moreover, TRACE_HELPERS has been removed from the Makefile since it is > not used on this program. > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > --- > samples/bpf/Makefile | 2 +- > samples/bpf/ibumad_kern.c | 26 +++++++-------- > samples/bpf/ibumad_user.c | 66 ++++++++++++++++++++++++++++++--------- > 3 files changed, 65 insertions(+), 29 deletions(-) > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index 36b261c7afc7..bfa595379493 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -109,7 +109,7 @@ xsk_fwd-objs := xsk_fwd.o > xdp_fwd-objs := xdp_fwd_user.o > task_fd_query-objs := task_fd_query_user.o $(TRACE_HELPERS) > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > -ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > +ibumad-objs := ibumad_user.o > hbm-objs := hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > # Tell kbuild to always build the programs > diff --git a/samples/bpf/ibumad_kern.c b/samples/bpf/ibumad_kern.c > index 3a91b4c1989a..26dcd4dde946 100644 > --- a/samples/bpf/ibumad_kern.c > +++ b/samples/bpf/ibumad_kern.c > @@ -16,19 +16,19 @@ > #include <bpf/bpf_helpers.h> > > > -struct bpf_map_def SEC("maps") read_count = { > - .type = BPF_MAP_TYPE_ARRAY, > - .key_size = sizeof(u32), /* class; u32 required */ > - .value_size = sizeof(u64), /* count of mads read */ > - .max_entries = 256, /* Room for all Classes */ > -}; > - > -struct bpf_map_def SEC("maps") write_count = { > - .type = BPF_MAP_TYPE_ARRAY, > - .key_size = sizeof(u32), /* class; u32 required */ > - .value_size = sizeof(u64), /* count of mads written */ > - .max_entries = 256, /* Room for all Classes */ > -}; > +struct { > + __uint(type, BPF_MAP_TYPE_ARRAY); > + __type(key, u32); /* class; u32 required */ > + __type(value, u64); /* count of mads read */ > + __uint(max_entries, 256); /* Room for all Classes */ > +} read_count SEC(".maps"); > + > +struct { > + __uint(type, BPF_MAP_TYPE_ARRAY); > + __type(key, u32); /* class; u32 required */ > + __type(value, u64); /* count of mads written */ > + __uint(max_entries, 256); /* Room for all Classes */ > +} write_count SEC(".maps"); > > #undef DEBUG > #ifndef DEBUG > diff --git a/samples/bpf/ibumad_user.c b/samples/bpf/ibumad_user.c > index fa06eef31a84..66a06272f242 100644 > --- a/samples/bpf/ibumad_user.c > +++ b/samples/bpf/ibumad_user.c > @@ -23,10 +23,15 @@ > #include <getopt.h> > #include <net/if.h> > > -#include "bpf_load.h" > +#include <bpf/bpf.h> > #include "bpf_util.h" > #include <bpf/libbpf.h> > > +struct bpf_link *tp_links[3] = {}; > +struct bpf_object *obj; statics and you can drop = {} part. > +static int map_fd[2]; > +static int tp_cnt; > + > static void dump_counts(int fd) > { > __u32 key; > @@ -53,6 +58,11 @@ static void dump_all_counts(void) > static void dump_exit(int sig) > { > dump_all_counts(); > + /* Detach tracepoints */ > + while (tp_cnt) > + bpf_link__destroy(tp_links[--tp_cnt]); > + > + bpf_object__close(obj); > exit(0); > } > > @@ -73,19 +83,11 @@ static void usage(char *cmd) > > int main(int argc, char **argv) > { > + struct bpf_program *prog; > unsigned long delay = 5; > + char filename[256]; > int longindex = 0; > int opt; > - char bpf_file[256]; > - > - /* Create the eBPF kernel code path name. > - * This follows the pattern of all of the other bpf samples > - */ > - snprintf(bpf_file, sizeof(bpf_file), "%s_kern.o", argv[0]); > - > - /* Do one final dump when exiting */ > - signal(SIGINT, dump_exit); > - signal(SIGTERM, dump_exit); > > while ((opt = getopt_long(argc, argv, "hd:rSw", > long_options, &longindex)) != -1) { > @@ -107,10 +109,38 @@ int main(int argc, char **argv) > } > } > > - if (load_bpf_file(bpf_file)) { > - fprintf(stderr, "ERROR: failed to load eBPF from file : %s\n", > - bpf_file); > - return 1; > + /* Do one final dump when exiting */ > + signal(SIGINT, dump_exit); > + signal(SIGTERM, dump_exit); > + > + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > + obj = bpf_object__open_file(filename, NULL); > + if (libbpf_get_error(obj)) { > + fprintf(stderr, "ERROR: opening BPF object file failed\n"); > + return 0; not really a success, no? > + } > + > + /* load BPF program */ > + if (bpf_object__load(obj)) { > + fprintf(stderr, "ERROR: loading BPF object file failed\n"); > + goto cleanup; > + } > + > + map_fd[0] = bpf_object__find_map_fd_by_name(obj, "read_count"); > + map_fd[1] = bpf_object__find_map_fd_by_name(obj, "write_count"); > + if (map_fd[0] < 0 || map_fd[1] < 0) { > + fprintf(stderr, "ERROR: finding a map in obj file failed\n"); > + goto cleanup; > + } > + > + bpf_object__for_each_program(prog, obj) { > + tp_links[tp_cnt] = bpf_program__attach(prog); > + if (libbpf_get_error(tp_links[tp_cnt])) { > + fprintf(stderr, "ERROR: bpf_program__attach failed\n"); > + tp_links[tp_cnt] = NULL; > + goto cleanup; > + } > + tp_cnt++; > } This cries for the BPF skeleton... But one step at a time :) > > while (1) { > @@ -118,5 +148,11 @@ int main(int argc, char **argv) > dump_all_counts(); > } > > +cleanup: > + /* Detach tracepoints */ > + while (tp_cnt) > + bpf_link__destroy(tp_links[--tp_cnt]); > + > + bpf_object__close(obj); > return 0; same, in a lot of cases it's not a success, probably need int err variable somewhere. > } > -- > 2.25.1 >
On Wed, Nov 18, 2020 at 11:52 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > This commit refactors the existing ibumad program with libbpf bpf > > loader. Attach/detach of Tracepoint bpf programs has been managed > > with the generic bpf_program__attach() and bpf_link__destroy() from > > the libbpf. > > > > Also, instead of using the previous BPF MAP definition, this commit > > refactors ibumad MAP definition with the new BTF-defined MAP format. > > > > To verify that this bpf program works without an infiniband device, > > try loading ib_umad kernel module and test the program as follows: > > > > # modprobe ib_umad > > # ./ibumad > > > > Moreover, TRACE_HELPERS has been removed from the Makefile since it is > > not used on this program. > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > --- > > samples/bpf/Makefile | 2 +- > > samples/bpf/ibumad_kern.c | 26 +++++++-------- > > samples/bpf/ibumad_user.c | 66 ++++++++++++++++++++++++++++++--------- > > 3 files changed, 65 insertions(+), 29 deletions(-) > > > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > > index 36b261c7afc7..bfa595379493 100644 > > --- a/samples/bpf/Makefile > > +++ b/samples/bpf/Makefile > > @@ -109,7 +109,7 @@ xsk_fwd-objs := xsk_fwd.o > > xdp_fwd-objs := xdp_fwd_user.o > > task_fd_query-objs := task_fd_query_user.o $(TRACE_HELPERS) > > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > > -ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > > +ibumad-objs := ibumad_user.o > > hbm-objs := hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > > > # Tell kbuild to always build the programs > > diff --git a/samples/bpf/ibumad_kern.c b/samples/bpf/ibumad_kern.c > > index 3a91b4c1989a..26dcd4dde946 100644 > > --- a/samples/bpf/ibumad_kern.c > > +++ b/samples/bpf/ibumad_kern.c > > @@ -16,19 +16,19 @@ > > #include <bpf/bpf_helpers.h> > > > > > > -struct bpf_map_def SEC("maps") read_count = { > > - .type = BPF_MAP_TYPE_ARRAY, > > - .key_size = sizeof(u32), /* class; u32 required */ > > - .value_size = sizeof(u64), /* count of mads read */ > > - .max_entries = 256, /* Room for all Classes */ > > -}; > > - > > -struct bpf_map_def SEC("maps") write_count = { > > - .type = BPF_MAP_TYPE_ARRAY, > > - .key_size = sizeof(u32), /* class; u32 required */ > > - .value_size = sizeof(u64), /* count of mads written */ > > - .max_entries = 256, /* Room for all Classes */ > > -}; > > +struct { > > + __uint(type, BPF_MAP_TYPE_ARRAY); > > + __type(key, u32); /* class; u32 required */ > > + __type(value, u64); /* count of mads read */ > > + __uint(max_entries, 256); /* Room for all Classes */ > > +} read_count SEC(".maps"); > > + > > +struct { > > + __uint(type, BPF_MAP_TYPE_ARRAY); > > + __type(key, u32); /* class; u32 required */ > > + __type(value, u64); /* count of mads written */ > > + __uint(max_entries, 256); /* Room for all Classes */ > > +} write_count SEC(".maps"); > > > > #undef DEBUG > > #ifndef DEBUG > > diff --git a/samples/bpf/ibumad_user.c b/samples/bpf/ibumad_user.c > > index fa06eef31a84..66a06272f242 100644 > > --- a/samples/bpf/ibumad_user.c > > +++ b/samples/bpf/ibumad_user.c > > @@ -23,10 +23,15 @@ > > #include <getopt.h> > > #include <net/if.h> > > > > -#include "bpf_load.h" > > +#include <bpf/bpf.h> > > #include "bpf_util.h" > > #include <bpf/libbpf.h> > > > > +struct bpf_link *tp_links[3] = {}; > > +struct bpf_object *obj; > > statics and you can drop = {} part. > > > +static int map_fd[2]; > > +static int tp_cnt; > > + > > static void dump_counts(int fd) > > { > > __u32 key; > > @@ -53,6 +58,11 @@ static void dump_all_counts(void) > > static void dump_exit(int sig) > > { > > dump_all_counts(); > > + /* Detach tracepoints */ > > + while (tp_cnt) > > + bpf_link__destroy(tp_links[--tp_cnt]); > > + > > + bpf_object__close(obj); > > exit(0); > > } > > > > @@ -73,19 +83,11 @@ static void usage(char *cmd) > > > > int main(int argc, char **argv) > > { > > + struct bpf_program *prog; > > unsigned long delay = 5; > > + char filename[256]; > > int longindex = 0; > > int opt; > > - char bpf_file[256]; > > - > > - /* Create the eBPF kernel code path name. > > - * This follows the pattern of all of the other bpf samples > > - */ > > - snprintf(bpf_file, sizeof(bpf_file), "%s_kern.o", argv[0]); > > - > > - /* Do one final dump when exiting */ > > - signal(SIGINT, dump_exit); > > - signal(SIGTERM, dump_exit); > > > > while ((opt = getopt_long(argc, argv, "hd:rSw", > > long_options, &longindex)) != -1) { > > @@ -107,10 +109,38 @@ int main(int argc, char **argv) > > } > > } > > > > - if (load_bpf_file(bpf_file)) { > > - fprintf(stderr, "ERROR: failed to load eBPF from file : %s\n", > > - bpf_file); > > - return 1; > > + /* Do one final dump when exiting */ > > + signal(SIGINT, dump_exit); > > + signal(SIGTERM, dump_exit); > > + > > + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > > + obj = bpf_object__open_file(filename, NULL); > > + if (libbpf_get_error(obj)) { > > + fprintf(stderr, "ERROR: opening BPF object file failed\n"); > > + return 0; > > not really a success, no? > > > + } > > + > > + /* load BPF program */ > > + if (bpf_object__load(obj)) { > > + fprintf(stderr, "ERROR: loading BPF object file failed\n"); > > + goto cleanup; > > + } > > + > > + map_fd[0] = bpf_object__find_map_fd_by_name(obj, "read_count"); > > + map_fd[1] = bpf_object__find_map_fd_by_name(obj, "write_count"); > > + if (map_fd[0] < 0 || map_fd[1] < 0) { > > + fprintf(stderr, "ERROR: finding a map in obj file failed\n"); > > + goto cleanup; > > + } > > + > > + bpf_object__for_each_program(prog, obj) { > > + tp_links[tp_cnt] = bpf_program__attach(prog); > > + if (libbpf_get_error(tp_links[tp_cnt])) { > > + fprintf(stderr, "ERROR: bpf_program__attach failed\n"); > > + tp_links[tp_cnt] = NULL; > > + goto cleanup; > > + } > > + tp_cnt++; > > } > > This cries for the BPF skeleton... But one step at a time :) > I will make sure it'll be updated by using skeleton next time. :D > > > > while (1) { > > @@ -118,5 +148,11 @@ int main(int argc, char **argv) > > dump_all_counts(); > > } > > > > +cleanup: > > + /* Detach tracepoints */ > > + while (tp_cnt) > > + bpf_link__destroy(tp_links[--tp_cnt]); > > + > > + bpf_object__close(obj); > > return 0; > > same, in a lot of cases it's not a success, probably need int err > variable somewhere. > I will correct the return code and send the next version of patch. Thanks for your time and effort for the review.
On Tue, Nov 17, 2020 at 7:05 PM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > On Wed, Nov 18, 2020 at 11:52 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > > > This commit refactors the existing ibumad program with libbpf bpf > > > loader. Attach/detach of Tracepoint bpf programs has been managed > > > with the generic bpf_program__attach() and bpf_link__destroy() from > > > the libbpf. > > > > > > Also, instead of using the previous BPF MAP definition, this commit > > > refactors ibumad MAP definition with the new BTF-defined MAP format. > > > > > > To verify that this bpf program works without an infiniband device, > > > try loading ib_umad kernel module and test the program as follows: > > > > > > # modprobe ib_umad > > > # ./ibumad > > > > > > Moreover, TRACE_HELPERS has been removed from the Makefile since it is > > > not used on this program. > > > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > > --- > > > samples/bpf/Makefile | 2 +- > > > samples/bpf/ibumad_kern.c | 26 +++++++-------- > > > samples/bpf/ibumad_user.c | 66 ++++++++++++++++++++++++++++++--------- > > > 3 files changed, 65 insertions(+), 29 deletions(-) > > > > > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > > > index 36b261c7afc7..bfa595379493 100644 > > > --- a/samples/bpf/Makefile > > > +++ b/samples/bpf/Makefile > > > @@ -109,7 +109,7 @@ xsk_fwd-objs := xsk_fwd.o > > > xdp_fwd-objs := xdp_fwd_user.o > > > task_fd_query-objs := task_fd_query_user.o $(TRACE_HELPERS) > > > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > > > -ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > > > +ibumad-objs := ibumad_user.o > > > hbm-objs := hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > > > > > # Tell kbuild to always build the programs > > > diff --git a/samples/bpf/ibumad_kern.c b/samples/bpf/ibumad_kern.c > > > index 3a91b4c1989a..26dcd4dde946 100644 > > > --- a/samples/bpf/ibumad_kern.c > > > +++ b/samples/bpf/ibumad_kern.c > > > @@ -16,19 +16,19 @@ > > > #include <bpf/bpf_helpers.h> > > > > > > > > > -struct bpf_map_def SEC("maps") read_count = { > > > - .type = BPF_MAP_TYPE_ARRAY, > > > - .key_size = sizeof(u32), /* class; u32 required */ > > > - .value_size = sizeof(u64), /* count of mads read */ > > > - .max_entries = 256, /* Room for all Classes */ > > > -}; > > > - > > > -struct bpf_map_def SEC("maps") write_count = { > > > - .type = BPF_MAP_TYPE_ARRAY, > > > - .key_size = sizeof(u32), /* class; u32 required */ > > > - .value_size = sizeof(u64), /* count of mads written */ > > > - .max_entries = 256, /* Room for all Classes */ > > > -}; > > > +struct { > > > + __uint(type, BPF_MAP_TYPE_ARRAY); > > > + __type(key, u32); /* class; u32 required */ > > > + __type(value, u64); /* count of mads read */ > > > + __uint(max_entries, 256); /* Room for all Classes */ > > > +} read_count SEC(".maps"); > > > + > > > +struct { > > > + __uint(type, BPF_MAP_TYPE_ARRAY); > > > + __type(key, u32); /* class; u32 required */ > > > + __type(value, u64); /* count of mads written */ > > > + __uint(max_entries, 256); /* Room for all Classes */ > > > +} write_count SEC(".maps"); > > > > > > #undef DEBUG > > > #ifndef DEBUG > > > diff --git a/samples/bpf/ibumad_user.c b/samples/bpf/ibumad_user.c > > > index fa06eef31a84..66a06272f242 100644 > > > --- a/samples/bpf/ibumad_user.c > > > +++ b/samples/bpf/ibumad_user.c > > > @@ -23,10 +23,15 @@ > > > #include <getopt.h> > > > #include <net/if.h> > > > > > > -#include "bpf_load.h" > > > +#include <bpf/bpf.h> > > > #include "bpf_util.h" > > > #include <bpf/libbpf.h> > > > > > > +struct bpf_link *tp_links[3] = {}; > > > +struct bpf_object *obj; > > > > statics and you can drop = {} part. > > > > > +static int map_fd[2]; > > > +static int tp_cnt; > > > + > > > static void dump_counts(int fd) > > > { > > > __u32 key; > > > @@ -53,6 +58,11 @@ static void dump_all_counts(void) > > > static void dump_exit(int sig) > > > { > > > dump_all_counts(); > > > + /* Detach tracepoints */ > > > + while (tp_cnt) > > > + bpf_link__destroy(tp_links[--tp_cnt]); > > > + > > > + bpf_object__close(obj); > > > exit(0); > > > } > > > > > > @@ -73,19 +83,11 @@ static void usage(char *cmd) > > > > > > int main(int argc, char **argv) > > > { > > > + struct bpf_program *prog; > > > unsigned long delay = 5; > > > + char filename[256]; > > > int longindex = 0; > > > int opt; > > > - char bpf_file[256]; > > > - > > > - /* Create the eBPF kernel code path name. > > > - * This follows the pattern of all of the other bpf samples > > > - */ > > > - snprintf(bpf_file, sizeof(bpf_file), "%s_kern.o", argv[0]); > > > - > > > - /* Do one final dump when exiting */ > > > - signal(SIGINT, dump_exit); > > > - signal(SIGTERM, dump_exit); > > > > > > while ((opt = getopt_long(argc, argv, "hd:rSw", > > > long_options, &longindex)) != -1) { > > > @@ -107,10 +109,38 @@ int main(int argc, char **argv) > > > } > > > } > > > > > > - if (load_bpf_file(bpf_file)) { > > > - fprintf(stderr, "ERROR: failed to load eBPF from file : %s\n", > > > - bpf_file); > > > - return 1; > > > + /* Do one final dump when exiting */ > > > + signal(SIGINT, dump_exit); > > > + signal(SIGTERM, dump_exit); > > > + > > > + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > > > + obj = bpf_object__open_file(filename, NULL); > > > + if (libbpf_get_error(obj)) { > > > + fprintf(stderr, "ERROR: opening BPF object file failed\n"); > > > + return 0; > > > > not really a success, no? > > > > > + } > > > + > > > + /* load BPF program */ > > > + if (bpf_object__load(obj)) { > > > + fprintf(stderr, "ERROR: loading BPF object file failed\n"); > > > + goto cleanup; > > > + } > > > + > > > + map_fd[0] = bpf_object__find_map_fd_by_name(obj, "read_count"); > > > + map_fd[1] = bpf_object__find_map_fd_by_name(obj, "write_count"); > > > + if (map_fd[0] < 0 || map_fd[1] < 0) { > > > + fprintf(stderr, "ERROR: finding a map in obj file failed\n"); > > > + goto cleanup; > > > + } > > > + > > > + bpf_object__for_each_program(prog, obj) { > > > + tp_links[tp_cnt] = bpf_program__attach(prog); > > > + if (libbpf_get_error(tp_links[tp_cnt])) { > > > + fprintf(stderr, "ERROR: bpf_program__attach failed\n"); > > > + tp_links[tp_cnt] = NULL; > > > + goto cleanup; > > > + } > > > + tp_cnt++; > > > } > > > > This cries for the BPF skeleton... But one step at a time :) > > > > I will make sure it'll be updated by using skeleton next time. :D > > > > > > > while (1) { > > > @@ -118,5 +148,11 @@ int main(int argc, char **argv) > > > dump_all_counts(); > > > } > > > > > > +cleanup: > > > + /* Detach tracepoints */ > > > + while (tp_cnt) > > > + bpf_link__destroy(tp_links[--tp_cnt]); > > > + > > > + bpf_object__close(obj); > > > return 0; > > > > same, in a lot of cases it's not a success, probably need int err > > variable somewhere. > > > > I will correct the return code and send the next version of patch. > > Thanks for your time and effort for the review. You don't have to write that every time :) It's an implicit contract: you contribute your time to prepare, test, and submit a patch. Maintainers and reviewers contribute theirs to review, maybe improve, and eventually apply it. Together as a community we move the needle. > > -- > Best, > Daniel T. Lee
On Wed, Nov 18, 2020 at 12:10 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Nov 17, 2020 at 7:05 PM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > On Wed, Nov 18, 2020 at 11:52 AM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > > > > > This commit refactors the existing ibumad program with libbpf bpf > > > > loader. Attach/detach of Tracepoint bpf programs has been managed > > > > with the generic bpf_program__attach() and bpf_link__destroy() from > > > > the libbpf. > > > > > > > > Also, instead of using the previous BPF MAP definition, this commit > > > > refactors ibumad MAP definition with the new BTF-defined MAP format. > > > > > > > > To verify that this bpf program works without an infiniband device, > > > > try loading ib_umad kernel module and test the program as follows: > > > > > > > > # modprobe ib_umad > > > > # ./ibumad > > > > > > > > Moreover, TRACE_HELPERS has been removed from the Makefile since it is > > > > not used on this program. > > > > > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > > > --- > > > > samples/bpf/Makefile | 2 +- > > > > samples/bpf/ibumad_kern.c | 26 +++++++-------- > > > > samples/bpf/ibumad_user.c | 66 ++++++++++++++++++++++++++++++--------- > > > > 3 files changed, 65 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > > > > index 36b261c7afc7..bfa595379493 100644 > > > > --- a/samples/bpf/Makefile > > > > +++ b/samples/bpf/Makefile > > > > @@ -109,7 +109,7 @@ xsk_fwd-objs := xsk_fwd.o > > > > xdp_fwd-objs := xdp_fwd_user.o > > > > task_fd_query-objs := task_fd_query_user.o $(TRACE_HELPERS) > > > > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > > > > -ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > > > > +ibumad-objs := ibumad_user.o > > > > hbm-objs := hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > > > > > > > # Tell kbuild to always build the programs > > > > diff --git a/samples/bpf/ibumad_kern.c b/samples/bpf/ibumad_kern.c > > > > index 3a91b4c1989a..26dcd4dde946 100644 > > > > --- a/samples/bpf/ibumad_kern.c > > > > +++ b/samples/bpf/ibumad_kern.c > > > > @@ -16,19 +16,19 @@ > > > > #include <bpf/bpf_helpers.h> > > > > > > > > > > > > -struct bpf_map_def SEC("maps") read_count = { > > > > - .type = BPF_MAP_TYPE_ARRAY, > > > > - .key_size = sizeof(u32), /* class; u32 required */ > > > > - .value_size = sizeof(u64), /* count of mads read */ > > > > - .max_entries = 256, /* Room for all Classes */ > > > > -}; > > > > - > > > > -struct bpf_map_def SEC("maps") write_count = { > > > > - .type = BPF_MAP_TYPE_ARRAY, > > > > - .key_size = sizeof(u32), /* class; u32 required */ > > > > - .value_size = sizeof(u64), /* count of mads written */ > > > > - .max_entries = 256, /* Room for all Classes */ > > > > -}; > > > > +struct { > > > > + __uint(type, BPF_MAP_TYPE_ARRAY); > > > > + __type(key, u32); /* class; u32 required */ > > > > + __type(value, u64); /* count of mads read */ > > > > + __uint(max_entries, 256); /* Room for all Classes */ > > > > +} read_count SEC(".maps"); > > > > + > > > > +struct { > > > > + __uint(type, BPF_MAP_TYPE_ARRAY); > > > > + __type(key, u32); /* class; u32 required */ > > > > + __type(value, u64); /* count of mads written */ > > > > + __uint(max_entries, 256); /* Room for all Classes */ > > > > +} write_count SEC(".maps"); > > > > > > > > #undef DEBUG > > > > #ifndef DEBUG > > > > diff --git a/samples/bpf/ibumad_user.c b/samples/bpf/ibumad_user.c > > > > index fa06eef31a84..66a06272f242 100644 > > > > --- a/samples/bpf/ibumad_user.c > > > > +++ b/samples/bpf/ibumad_user.c > > > > @@ -23,10 +23,15 @@ > > > > #include <getopt.h> > > > > #include <net/if.h> > > > > > > > > -#include "bpf_load.h" > > > > +#include <bpf/bpf.h> > > > > #include "bpf_util.h" > > > > #include <bpf/libbpf.h> > > > > > > > > +struct bpf_link *tp_links[3] = {}; > > > > +struct bpf_object *obj; > > > > > > statics and you can drop = {} part. > > > > > > > +static int map_fd[2]; > > > > +static int tp_cnt; > > > > + > > > > static void dump_counts(int fd) > > > > { > > > > __u32 key; > > > > @@ -53,6 +58,11 @@ static void dump_all_counts(void) > > > > static void dump_exit(int sig) > > > > { > > > > dump_all_counts(); > > > > + /* Detach tracepoints */ > > > > + while (tp_cnt) > > > > + bpf_link__destroy(tp_links[--tp_cnt]); > > > > + > > > > + bpf_object__close(obj); > > > > exit(0); > > > > } > > > > > > > > @@ -73,19 +83,11 @@ static void usage(char *cmd) > > > > > > > > int main(int argc, char **argv) > > > > { > > > > + struct bpf_program *prog; > > > > unsigned long delay = 5; > > > > + char filename[256]; > > > > int longindex = 0; > > > > int opt; > > > > - char bpf_file[256]; > > > > - > > > > - /* Create the eBPF kernel code path name. > > > > - * This follows the pattern of all of the other bpf samples > > > > - */ > > > > - snprintf(bpf_file, sizeof(bpf_file), "%s_kern.o", argv[0]); > > > > - > > > > - /* Do one final dump when exiting */ > > > > - signal(SIGINT, dump_exit); > > > > - signal(SIGTERM, dump_exit); > > > > > > > > while ((opt = getopt_long(argc, argv, "hd:rSw", > > > > long_options, &longindex)) != -1) { > > > > @@ -107,10 +109,38 @@ int main(int argc, char **argv) > > > > } > > > > } > > > > > > > > - if (load_bpf_file(bpf_file)) { > > > > - fprintf(stderr, "ERROR: failed to load eBPF from file : %s\n", > > > > - bpf_file); > > > > - return 1; > > > > + /* Do one final dump when exiting */ > > > > + signal(SIGINT, dump_exit); > > > > + signal(SIGTERM, dump_exit); > > > > + > > > > + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); > > > > + obj = bpf_object__open_file(filename, NULL); > > > > + if (libbpf_get_error(obj)) { > > > > + fprintf(stderr, "ERROR: opening BPF object file failed\n"); > > > > + return 0; > > > > > > not really a success, no? > > > > > > > + } > > > > + > > > > + /* load BPF program */ > > > > + if (bpf_object__load(obj)) { > > > > + fprintf(stderr, "ERROR: loading BPF object file failed\n"); > > > > + goto cleanup; > > > > + } > > > > + > > > > + map_fd[0] = bpf_object__find_map_fd_by_name(obj, "read_count"); > > > > + map_fd[1] = bpf_object__find_map_fd_by_name(obj, "write_count"); > > > > + if (map_fd[0] < 0 || map_fd[1] < 0) { > > > > + fprintf(stderr, "ERROR: finding a map in obj file failed\n"); > > > > + goto cleanup; > > > > + } > > > > + > > > > + bpf_object__for_each_program(prog, obj) { > > > > + tp_links[tp_cnt] = bpf_program__attach(prog); > > > > + if (libbpf_get_error(tp_links[tp_cnt])) { > > > > + fprintf(stderr, "ERROR: bpf_program__attach failed\n"); > > > > + tp_links[tp_cnt] = NULL; > > > > + goto cleanup; > > > > + } > > > > + tp_cnt++; > > > > } > > > > > > This cries for the BPF skeleton... But one step at a time :) > > > > > > > I will make sure it'll be updated by using skeleton next time. :D > > > > > > > > > > while (1) { > > > > @@ -118,5 +148,11 @@ int main(int argc, char **argv) > > > > dump_all_counts(); > > > > } > > > > > > > > +cleanup: > > > > + /* Detach tracepoints */ > > > > + while (tp_cnt) > > > > + bpf_link__destroy(tp_links[--tp_cnt]); > > > > + > > > > + bpf_object__close(obj); > > > > return 0; > > > > > > same, in a lot of cases it's not a success, probably need int err > > > variable somewhere. > > > > > > > I will correct the return code and send the next version of patch. > > > > Thanks for your time and effort for the review. > > You don't have to write that every time :) It's an implicit contract: > you contribute your time to prepare, test, and submit a patch. > Maintainers and reviewers contribute theirs to review, maybe improve, > and eventually apply it. Together as a community we move the needle. > Thank you for your kindness, and the tip!
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile index 36b261c7afc7..bfa595379493 100644 --- a/samples/bpf/Makefile +++ b/samples/bpf/Makefile @@ -109,7 +109,7 @@ xsk_fwd-objs := xsk_fwd.o xdp_fwd-objs := xdp_fwd_user.o task_fd_query-objs := task_fd_query_user.o $(TRACE_HELPERS) xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) -ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) +ibumad-objs := ibumad_user.o hbm-objs := hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) # Tell kbuild to always build the programs diff --git a/samples/bpf/ibumad_kern.c b/samples/bpf/ibumad_kern.c index 3a91b4c1989a..26dcd4dde946 100644 --- a/samples/bpf/ibumad_kern.c +++ b/samples/bpf/ibumad_kern.c @@ -16,19 +16,19 @@ #include <bpf/bpf_helpers.h> -struct bpf_map_def SEC("maps") read_count = { - .type = BPF_MAP_TYPE_ARRAY, - .key_size = sizeof(u32), /* class; u32 required */ - .value_size = sizeof(u64), /* count of mads read */ - .max_entries = 256, /* Room for all Classes */ -}; - -struct bpf_map_def SEC("maps") write_count = { - .type = BPF_MAP_TYPE_ARRAY, - .key_size = sizeof(u32), /* class; u32 required */ - .value_size = sizeof(u64), /* count of mads written */ - .max_entries = 256, /* Room for all Classes */ -}; +struct { + __uint(type, BPF_MAP_TYPE_ARRAY); + __type(key, u32); /* class; u32 required */ + __type(value, u64); /* count of mads read */ + __uint(max_entries, 256); /* Room for all Classes */ +} read_count SEC(".maps"); + +struct { + __uint(type, BPF_MAP_TYPE_ARRAY); + __type(key, u32); /* class; u32 required */ + __type(value, u64); /* count of mads written */ + __uint(max_entries, 256); /* Room for all Classes */ +} write_count SEC(".maps"); #undef DEBUG #ifndef DEBUG diff --git a/samples/bpf/ibumad_user.c b/samples/bpf/ibumad_user.c index fa06eef31a84..66a06272f242 100644 --- a/samples/bpf/ibumad_user.c +++ b/samples/bpf/ibumad_user.c @@ -23,10 +23,15 @@ #include <getopt.h> #include <net/if.h> -#include "bpf_load.h" +#include <bpf/bpf.h> #include "bpf_util.h" #include <bpf/libbpf.h> +struct bpf_link *tp_links[3] = {}; +struct bpf_object *obj; +static int map_fd[2]; +static int tp_cnt; + static void dump_counts(int fd) { __u32 key; @@ -53,6 +58,11 @@ static void dump_all_counts(void) static void dump_exit(int sig) { dump_all_counts(); + /* Detach tracepoints */ + while (tp_cnt) + bpf_link__destroy(tp_links[--tp_cnt]); + + bpf_object__close(obj); exit(0); } @@ -73,19 +83,11 @@ static void usage(char *cmd) int main(int argc, char **argv) { + struct bpf_program *prog; unsigned long delay = 5; + char filename[256]; int longindex = 0; int opt; - char bpf_file[256]; - - /* Create the eBPF kernel code path name. - * This follows the pattern of all of the other bpf samples - */ - snprintf(bpf_file, sizeof(bpf_file), "%s_kern.o", argv[0]); - - /* Do one final dump when exiting */ - signal(SIGINT, dump_exit); - signal(SIGTERM, dump_exit); while ((opt = getopt_long(argc, argv, "hd:rSw", long_options, &longindex)) != -1) { @@ -107,10 +109,38 @@ int main(int argc, char **argv) } } - if (load_bpf_file(bpf_file)) { - fprintf(stderr, "ERROR: failed to load eBPF from file : %s\n", - bpf_file); - return 1; + /* Do one final dump when exiting */ + signal(SIGINT, dump_exit); + signal(SIGTERM, dump_exit); + + snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]); + obj = bpf_object__open_file(filename, NULL); + if (libbpf_get_error(obj)) { + fprintf(stderr, "ERROR: opening BPF object file failed\n"); + return 0; + } + + /* load BPF program */ + if (bpf_object__load(obj)) { + fprintf(stderr, "ERROR: loading BPF object file failed\n"); + goto cleanup; + } + + map_fd[0] = bpf_object__find_map_fd_by_name(obj, "read_count"); + map_fd[1] = bpf_object__find_map_fd_by_name(obj, "write_count"); + if (map_fd[0] < 0 || map_fd[1] < 0) { + fprintf(stderr, "ERROR: finding a map in obj file failed\n"); + goto cleanup; + } + + bpf_object__for_each_program(prog, obj) { + tp_links[tp_cnt] = bpf_program__attach(prog); + if (libbpf_get_error(tp_links[tp_cnt])) { + fprintf(stderr, "ERROR: bpf_program__attach failed\n"); + tp_links[tp_cnt] = NULL; + goto cleanup; + } + tp_cnt++; } while (1) { @@ -118,5 +148,11 @@ int main(int argc, char **argv) dump_all_counts(); } +cleanup: + /* Detach tracepoints */ + while (tp_cnt) + bpf_link__destroy(tp_links[--tp_cnt]); + + bpf_object__close(obj); return 0; }
This commit refactors the existing ibumad program with libbpf bpf loader. Attach/detach of Tracepoint bpf programs has been managed with the generic bpf_program__attach() and bpf_link__destroy() from the libbpf. Also, instead of using the previous BPF MAP definition, this commit refactors ibumad MAP definition with the new BTF-defined MAP format. To verify that this bpf program works without an infiniband device, try loading ib_umad kernel module and test the program as follows: # modprobe ib_umad # ./ibumad Moreover, TRACE_HELPERS has been removed from the Makefile since it is not used on this program. Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> --- samples/bpf/Makefile | 2 +- samples/bpf/ibumad_kern.c | 26 +++++++-------- samples/bpf/ibumad_user.c | 66 ++++++++++++++++++++++++++++++--------- 3 files changed, 65 insertions(+), 29 deletions(-)