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