Re: [PATCH 13/19] bpf: Add support to link multi func tracing program

On Tue, Jun 08, 2021 at 10:18:21PM -0700, Andrii Nakryiko wrote:

On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:

quoted

Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.

Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.

The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).

So I'm not sure why we added a new load flag instead of just using a
new BPF program type or expected attach type?  We have different
trampolines and different kinds of links for them, so why not be
consistent and use the new type of BPF program?.. It does change BPF
verifier's treatment of input arguments, so it's not just a slight
variation, it's quite different type of program.

ok, makes sense ... BPF_PROG_TYPE_TRACING_MULTI ?

SNIP

quoted

 struct bpf_attach_target_info {

@@ -746,6 +747,8 @@ void bpf_ksym_add(struct bpf_ksym *ksym);
 void bpf_ksym_del(struct bpf_ksym *ksym);
 int bpf_jit_charge_modmem(u32 pages);
 void bpf_jit_uncharge_modmem(u32 pages);
+struct bpf_trampoline *bpf_trampoline_multi_alloc(void);
+void bpf_trampoline_multi_free(struct bpf_trampoline *tr);
 #else
 static inline int bpf_trampoline_link_prog(struct bpf_prog *prog,
                                           struct bpf_trampoline *tr)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index ad9340fb14d4..5fd6ff64e8dc 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -1007,6 +1007,7 @@ enum bpf_link_type {
        BPF_LINK_TYPE_ITER = 4,
        BPF_LINK_TYPE_NETNS = 5,
        BPF_LINK_TYPE_XDP = 6,
+       BPF_LINK_TYPE_TRACING_MULTI = 7,

        MAX_BPF_LINK_TYPE,
 };

@@ -1454,6 +1455,10 @@ union bpf_attr {
                                __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
                                __u32           iter_info_len;  /* iter_info length */
                        };
+                       struct {
+                               __aligned_u64   multi_btf_ids;          /* addresses to attach */
+                               __u32           multi_btf_ids_cnt;      /* addresses count */
+                       };

let's do what bpf_link-based TC-BPF API is doing, put it into a named
field (I'd do the same for iter_info/iter_info_len above as well, I'm
not sure why we did this flat naming scheme, we now it's inconvenient
when extending stuff).

struct {
    __aligned_u64 btf_ids;
    __u32 btf_ids_cnt;
} multi;

ok

quoted

                };
        } link_create;

[...]

quoted

+static int bpf_tracing_multi_link_update(struct bpf_link *link,
+                                        struct bpf_prog *new_prog,
+                                        struct bpf_prog *old_prog __maybe_unused)
+{

BPF_LINK_UPDATE command supports passing old_fd and extra flags. We
can use that to implement both updating existing BPF program in-place
(by passing BPF_F_REPLACE and old_fd) or adding the program to the
list of programs, if old_fd == 0. WDYT?

yes, sounds good

thanks,
jirka