On Tue, Jun 08, 2021 at 10:18:21PM -0700, Andrii Nakryiko wrote:
> On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> >
> > 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
> > 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
>
> > };
> > } link_create;
> >
>
> [...]
>
> > +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
