Re: [PATCH bpf-next 1/3] samples: bpf: Refactor xdp_monitor with libbpf

On Sat, Oct 10, 2020 at 3:17 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Fri, Oct 9, 2020 at 9:04 AM Daniel T. Lee <[email protected]> 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 <[email protected]>
> > ---
> >  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++;
> > +       }
> > +
>
> [...]