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

[Jiri Olsa <jolsa@redhat.com>]

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@kernel.org> 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