On Mon, Apr 26, 2021 at 10:14:45AM -0700, Andrii Nakryiko wrote:

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

quoted

From: Alexei Starovoitov <ast@kernel.org>

Add support for FD_IDX make libbpf prefer that approach to loading programs.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/lib/bpf/bpf.c             |  1 +
 tools/lib/bpf/libbpf.c          | 70 +++++++++++++++++++++++++++++----
 tools/lib/bpf/libbpf_internal.h |  1 +
 3 files changed, 65 insertions(+), 7 deletions(-)

[...]

quoted

+static int probe_kern_fd_idx(void)
+{
+       struct bpf_load_program_attr attr;
+       struct bpf_insn insns[] = {
+               BPF_LD_IMM64_RAW(BPF_REG_0, BPF_PSEUDO_MAP_IDX, 0),
+               BPF_EXIT_INSN(),
+       };
+
+       memset(&attr, 0, sizeof(attr));
+       attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
+       attr.insns = insns;
+       attr.insns_cnt = ARRAY_SIZE(insns);
+       attr.license = "GPL";
+
+       probe_fd(bpf_load_program_xattr(&attr, NULL, 0));

probe_fd() calls close(fd) internally, which technically can interfere
with errno, though close() shouldn't be called because syscall has to
fail on correct kernels... So this should work, but I feel like
open-coding that logic is better than ignoring probe_fd() result.

It will fail on all kernels.
That probe_fd was a left over of earlier detection approach where it would
proceed to load all the way, but then I switched to:

quoted

+       return errno == EPROTO;

since such style of probing is much cheaper for the kernel and user space.
But point taken. Will open code it.

quoted

+}
+

[...]

quoted

@@ -7239,6 +7279,8 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
        struct bpf_program *prog;
        size_t i;
        int err;
+       struct bpf_map *map;
+       int *fd_array = NULL;

        for (i = 0; i < obj->nr_programs; i++) {
                prog = &obj->programs[i];

@@ -7247,6 +7289,16 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
                        return err;
        }

+       if (kernel_supports(FEAT_FD_IDX) && obj->nr_maps) {
+               fd_array = malloc(sizeof(int) * obj->nr_maps);
+               if (!fd_array)
+                       return -ENOMEM;
+               for (i = 0; i < obj->nr_maps; i++) {
+                       map = &obj->maps[i];
+                       fd_array[i] = map->fd;

nit: obj->maps[i].fd will keep it a single line

quoted

+               }
+       }
+
        for (i = 0; i < obj->nr_programs; i++) {
                prog = &obj->programs[i];
                if (prog_is_subprog(obj, prog))

@@ -7256,10 +7308,14 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
                        continue;
                }
                prog->log_level |= log_level;
+               prog->fd_array = fd_array;

you are not freeing this memory on success, as far as I can see. 

hmm. there is free on success below.

And
given multiple programs are sharing fd_array, it's a bit problematic
for prog to have fd_array. This is per-object properly, so let's add
it at bpf_object level and clean it up on bpf_object__close()? And by
assigning to obj->fd_array at malloc() site, you won't need to do all
the error-handling free()s below.

hmm. that sounds worse.
why add another 8 byte to bpf_object that won't be used
until this last step of bpf_object__load_progs.
And only for the duration of this loading.
It's cheaper to have this alloc here with two free()s below.

quoted

                err = bpf_program__load(prog, obj->license, obj->kern_version);
-               if (err)
+               if (err) {
+                       free(fd_array);
                        return err;
+               }
        }
+       free(fd_array);
        return 0;
 }

diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 6017902c687e..9114c7085f2a 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h

@@ -204,6 +204,7 @@ struct bpf_prog_load_params {
        __u32 log_level;
        char *log_buf;
        size_t log_buf_sz;
+       int *fd_array;
 };

 int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr);
--

2.30.2

--