On Sat, Oct 10, 2020 at 3:17 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Fri, Oct 9, 2020 at 9:04 AM Daniel T. Lee <danieltimlee@xxxxxxxxx> wrote: > > > > To avoid confusion caused by the increasing fragmentation of the BPF > > Loader program, this commit would like to change to the libbpf loader > > instead of using the bpf_load. > > > > Thanks to libbpf's bpf_link interface, managing the tracepoint BPF > > program is much easier. bpf_program__attach_tracepoint manages the > > enable of tracepoint event and attach of BPF programs to it with a > > single interface bpf_link, so there is no need to manage event_fd and > > prog_fd separately. > > > > This commit refactors xdp_monitor with using this libbpf API, and the > > bpf_load is removed and migrated to libbpf. > > > > Signed-off-by: Daniel T. Lee <danieltimlee@xxxxxxxxx> > > --- > > samples/bpf/Makefile | 2 +- > > samples/bpf/xdp_monitor_user.c | 144 ++++++++++++++++++++++++--------- > > 2 files changed, 108 insertions(+), 38 deletions(-) > > > > [...] > > > +static int tp_cnt; > > +static int map_cnt; > > static int verbose = 1; > > static bool debug = false; > > +struct bpf_map *map_data[NUM_MAP] = { 0 }; > > +struct bpf_link *tp_links[NUM_TP] = { 0 }; > > this syntax means "initialize *only the first element* to 0 > (explicitly) and the rest of elements to default (which is also 0)". > So it's just misleading, use ` = {}`. > Thanks for the great review! Come to think of it, it could be confusing as you mentioned. I will remove the unnecessary initializer in the next patch and resend it. > > > > static const struct option long_options[] = { > > {"help", no_argument, NULL, 'h' }, > > @@ -41,6 +65,15 @@ static const struct option long_options[] = { > > {0, 0, NULL, 0 } > > }; > > > > +static void int_exit(int sig) > > +{ > > + /* Detach tracepoints */ > > + while (tp_cnt) > > + bpf_link__destroy(tp_links[--tp_cnt]); > > + > > see below about proper cleanup > > > + exit(0); > > +} > > + > > /* C standard specifies two constants, EXIT_SUCCESS(0) and EXIT_FAILURE(1) */ > > #define EXIT_FAIL_MEM 5 > > > > [...] > > > > > -static void print_bpf_prog_info(void) > > +static void print_bpf_prog_info(struct bpf_object *obj) > > { > > - int i; > > + struct bpf_program *prog; > > + struct bpf_map *map; > > + int i = 0; > > > > /* Prog info */ > > - printf("Loaded BPF prog have %d bpf program(s)\n", prog_cnt); > > - for (i = 0; i < prog_cnt; i++) { > > - printf(" - prog_fd[%d] = fd(%d)\n", i, prog_fd[i]); > > + printf("Loaded BPF prog have %d bpf program(s)\n", tp_cnt); > > + bpf_object__for_each_program(prog, obj) { > > + printf(" - prog_fd[%d] = fd(%d)\n", i++, bpf_program__fd(prog)); > > } > > > > + i = 0; > > /* Maps info */ > > - printf("Loaded BPF prog have %d map(s)\n", map_data_count); > > - for (i = 0; i < map_data_count; i++) { > > - char *name = map_data[i].name; > > - int fd = map_data[i].fd; > > + printf("Loaded BPF prog have %d map(s)\n", map_cnt); > > + bpf_object__for_each_map(map, obj) { > > + const char *name = bpf_map__name(map); > > + int fd = bpf_map__fd(map); > > > > - printf(" - map_data[%d] = fd(%d) name:%s\n", i, fd, name); > > + printf(" - map_data[%d] = fd(%d) name:%s\n", i++, fd, name); > > please move out increment into a separate statement, no need to > confuse readers unnecessarily > I will fix it at the following patch. > > } > > > > /* Event info */ > > - printf("Searching for (max:%d) event file descriptor(s)\n", prog_cnt); > > - for (i = 0; i < prog_cnt; i++) { > > - if (event_fd[i] != -1) > > - printf(" - event_fd[%d] = fd(%d)\n", i, event_fd[i]); > > + printf("Searching for (max:%d) event file descriptor(s)\n", tp_cnt); > > + for (i = 0; i < tp_cnt; i++) { > > + int fd = bpf_link__fd(tp_links[i]); > > + > > + if (fd != -1) > > + printf(" - event_fd[%d] = fd(%d)\n", i, fd); > > } > > } > > > > int main(int argc, char **argv) > > { > > [...] > > > + obj = bpf_object__open_file(filename, NULL); > > + if (libbpf_get_error(obj)) { > > + printf("ERROR: opening BPF object file failed\n"); > > + obj = NULL; > > return EXIT_FAILURE; > > } > > - if (!prog_fd[0]) { > > - printf("ERROR - load_bpf_file: %s\n", strerror(errno)); > > + > > + /* load BPF program */ > > + if (bpf_object__load(obj)) { > > would be still good to call bpf_object__close(obj) here, this will > avoid warnings about memory leaks, if you run this program under ASAN > > > + printf("ERROR: loading BPF object file failed\n"); > > return EXIT_FAILURE; > > } > > > > + for (type = 0; type < NUM_MAP; type++) { > > + map_data[type] = > > + bpf_object__find_map_by_name(obj, map_type_strings[type]); > > + > > + if (libbpf_get_error(map_data[type])) { > > + printf("ERROR: finding a map in obj file failed\n"); > > same about cleanup, goto into single cleanup place would be > appropriate throughout this entire function, probably. > Jump to single cleanup will be much more intuitive. I will update and send the next version of patch right away. Thank you for your time and effort for the review. Best, Daniel > > + return EXIT_FAILURE; > > + } > > + map_cnt++; > > + } > > + > > [...]
![]() |