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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Oct 9, 2020 at 9:04 AM Daniel T. Lee <danieltimlee@gmail.com> 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@gmail.com>
> ---
>  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 ` = {}`.

>
>  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

>         }
>
>         /* 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.

> +                       return EXIT_FAILURE;
> +               }
> +               map_cnt++;
> +       }
> +

[...]