Re: [PATCH v2 bpf-next 15/16] bpftool: Use syscall/loader program in "prog load" and "gen skeleton" command.

On Mon, Apr 26, 2021 at 8:28 PM Alexei Starovoitov
[off-list ref] wrote:

On Mon, Apr 26, 2021 at 03:35:16PM -0700, Andrii Nakryiko wrote:

quoted

On Thu, Apr 22, 2021 at 5:27 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

From: Alexei Starovoitov <ast@kernel.org>

Add -L flag to bpftool to use libbpf gen_trace facility and syscall/loader program
for skeleton generation and program loading.

"bpftool gen skeleton -L" command will generate a "light skeleton" or "loader skeleton"
that is similar to existing skeleton, but has one major difference:
$ bpftool gen skeleton lsm.o > lsm.skel.h
$ bpftool gen skeleton -L lsm.o > lsm.lskel.h
$ diff lsm.skel.h lsm.lskel.h

@@ -5,34 +4,34 @@
 #define __LSM_SKEL_H__

 #include <stdlib.h>
-#include <bpf/libbpf.h>
+#include <bpf/bpf.h>

The light skeleton does not use majority of libbpf infrastructure.
It doesn't need libelf. It doesn't parse .o file.
It only needs few sys_bpf wrappers. All of them are in bpf/bpf.h file.
In future libbpf/bpf.c can be inlined into bpf.h, so not even libbpf.a would be
needed to work with light skeleton.

"bpftool prog load -L file.o" command is introduced for debugging of syscall/loader
program generation. Just like the same command without -L it will try to load
the programs from file.o into the kernel. It won't even try to pin them.

"bpftool prog load -L -d file.o" command will provide additional debug messages
on how syscall/loader program was generated.
Also the execution of syscall/loader program will use bpf_trace_printk() for
each step of loading BTF, creating maps, and loading programs.
The user can do "cat /.../trace_pipe" for further debug.

An example of fexit_sleep.lskel.h generated from progs/fexit_sleep.c:
struct fexit_sleep {
        struct bpf_loader_ctx ctx;
        struct {
                struct bpf_map_desc bss;
        } maps;
        struct {
                struct bpf_prog_desc nanosleep_fentry;
                struct bpf_prog_desc nanosleep_fexit;
        } progs;
        struct {
                int nanosleep_fentry_fd;
                int nanosleep_fexit_fd;
        } links;
        struct fexit_sleep__bss {
                int pid;
                int fentry_cnt;
                int fexit_cnt;
        } *bss;
};

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/bpf/bpftool/Makefile        |   2 +-
 tools/bpf/bpftool/gen.c           | 313 +++++++++++++++++++++++++++---
 tools/bpf/bpftool/main.c          |   7 +-
 tools/bpf/bpftool/main.h          |   1 +
 tools/bpf/bpftool/prog.c          |  80 ++++++++
 tools/bpf/bpftool/xlated_dumper.c |   3 +
 6 files changed, 382 insertions(+), 24 deletions(-)

[...]

quoted

@@ -268,6 +269,254 @@ static void codegen(const char *template, ...)
        free(s);
 }

+static void print_hex(const char *obj_data, int file_sz)
+{
+       int i, len;
+
+       /* embed contents of BPF object file */

nit: this comment should have stayed at the original place

quoted

+       for (i = 0, len = 0; i < file_sz; i++) {
+               int w = obj_data[i] ? 4 : 2;
+

[...]

quoted

+       bpf_object__for_each_map(map, obj) {
+               const char * ident;
+
+               ident = get_map_ident(map);
+               if (!ident)
+                       continue;
+
+               if (!bpf_map__is_internal(map) ||
+                   !(bpf_map__def(map)->map_flags & BPF_F_MMAPABLE))
+                       continue;
+
+               printf("\tskel->%1$s =\n"
+                      "\t\tmmap(NULL, %2$zd, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,\n"
+                      "\t\t\tskel->maps.%1$s.map_fd, 0);\n",
+                      ident, bpf_map_mmap_sz(map));

use codegen()?

why?
codegen() would add extra early \n for no good reason.

for consistency, seems like the rest of the code in that function uses
codegen(). But not critical.

quoted

quoted

+       }
+       codegen("\
+               \n\
+                       return 0;                                           \n\
+               }                                                           \n\
+                                                                           \n\
+               static inline struct %1$s *                                 \n\

[...]

quoted

 static int do_skeleton(int argc, char **argv)
 {
        char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];

@@ -277,7 +526,7 @@ static int do_skeleton(int argc, char **argv)
        struct bpf_object *obj = NULL;
        const char *file, *ident;
        struct bpf_program *prog;
-       int fd, len, err = -1;
+       int fd, err = -1;
        struct bpf_map *map;
        struct btf *btf;
        struct stat st;

@@ -359,7 +608,25 @@ static int do_skeleton(int argc, char **argv)
        }

        get_header_guard(header_guard, obj_name);
-       codegen("\
+       if (use_loader)

please use {} for such a long if/else, even if it's, technically, a
single-statement if

I think it reads fine as-is, but, sure, I can add {}

quoted

quoted

+               codegen("\
+               \n\
+               /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */   \n\
+               /* THIS FILE IS AUTOGENERATED! */                           \n\
+               #ifndef %2$s                                                \n\
+               #define %2$s                                                \n\
+                                                                           \n\
+               #include <stdlib.h>                                         \n\
+               #include <bpf/bpf.h>                                        \n\
+               #include <bpf/skel_internal.h>                              \n\
+                                                                           \n\
+               struct %1$s {                                               \n\
+                       struct bpf_loader_ctx ctx;                          \n\
+               ",
+               obj_name, header_guard
+               );
+       else
+               codegen("\
                \n\
                /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */   \n\
                                                                            \n\

[...]

--