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

On 6/7/21 11:25 AM, Jiri Olsa wrote:

On Sun, Jun 06, 2021 at 10:36:57PM -0700, Yonghong Song wrote:

quoted

On 6/5/21 4:10 AM, Jiri Olsa 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).

The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.

The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.

I am not sure how severe such a limitation could be in practice.
It is possible in production some non-multi fentry/fexit program
may run continuously. Does kprobe program impact this as well?

I did not find a way how to combine current trampolines with the
new ones for multiple programs.. what you described is a limitation
of the current approach

I'm not sure about kprobes and trampolines, but the limitation
should be same as we do have for current trampolines.. I'll check

quoted

quoted

The new bpf_trampoline will store and pass to bpf program the highest
number of arguments from all given functions.

New programs (fentry or fexit) can be added to the existing trampoline
through the link_update interface via new_prog_fd descriptor.

Looks we do not support replacing old programs. Do we support
removing old programs?

we don't.. it's not what bpftrace would do, it just adds programs
to trace and close all when it's done.. I think interface for removal
could be added if you think it's needed

This can be a followup patch. Indeed removing selective old programs 
probably not a common use case.

quoted

quoted

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
   include/linux/bpf.h            |   3 +
   include/uapi/linux/bpf.h       |   5 +
   kernel/bpf/syscall.c           | 185 ++++++++++++++++++++++++++++++++-
   kernel/bpf/trampoline.c        |  53 +++++++---
   tools/include/uapi/linux/bpf.h |   5 +
   5 files changed, 237 insertions(+), 14 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 23221e0e8d3c..99a81c6c22e6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -661,6 +661,7 @@ struct bpf_trampoline {
   	struct bpf_tramp_image *cur_image;
   	u64 selector;
   	struct module *mod;
+	bool multi;
   };
   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)

[...]

quoted

quoted

@@ -363,9 +366,15 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
   		goto out;
   	}
+	if (tr->multi)
+		flags |= BPF_TRAMP_F_IP_ARG;
+
   	if (tprogs[BPF_TRAMP_FEXIT].nr_progs ||
-	    tprogs[BPF_TRAMP_MODIFY_RETURN].nr_progs)
+	    tprogs[BPF_TRAMP_MODIFY_RETURN].nr_progs) {
   		flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
+		if (tr->multi)
+			flags |= BPF_TRAMP_F_ORIG_STACK | BPF_TRAMP_F_IP_ARG;

BPF_TRAMP_F_IP_ARG is not needed. It has been added before.

it's erased in 2 lines above.. which reminds me that I forgot to check
if that's a bug or intended ;-)

Oh, yes, I miss that too :-) I guess it would be good if you can
re-organize the code to avoid resetting of flags.

jirka