Re: [PATCHv2 bpf-next 3/3] bpf: Change modules resolving for kprobe multi link

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jan 13, 2023 at 12:41:50PM -0800, Song Liu wrote:
> On Fri, Jan 13, 2023 at 6:44 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > We currently use module_kallsyms_on_each_symbol that iterates all
> > modules/symbols and we try to lookup each such address in user
> > provided symbols/addresses to get list of used modules.
> >
> > This fix instead only iterates provided kprobe addresses and calls
> > __module_address on each to get list of used modules. This turned
> > out to be simpler and also bit faster.
> >
> > On my setup with workload being (executed 10 times):
> >
> >    # test_progs -t kprobe_multi_bench_attach_module
> >
> > Current code:
> >
> >  Performance counter stats for './test.sh' (5 runs):
> >
> >     76,081,161,596      cycles:k                   ( +-  0.47% )
> >
> >            18.3867 +- 0.0992 seconds time elapsed  ( +-  0.54% )
> >
> > With the fix:
> >
> >  Performance counter stats for './test.sh' (5 runs):
> >
> >     74,079,889,063      cycles:k                   ( +-  0.04% )
> >
> >            17.8514 +- 0.0218 seconds time elapsed  ( +-  0.12% )
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c | 95 +++++++++++++++++++++-------------------
> >  1 file changed, 49 insertions(+), 46 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 095f7f8d34a1..90c5d5026831 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2682,69 +2682,79 @@ static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> >         }
> >  }
> >
> > -struct module_addr_args {
> > -       unsigned long *addrs;
> > -       u32 addrs_cnt;
> > +struct modules_array {
> >         struct module **mods;
> >         int mods_cnt;
> >         int mods_cap;
> >  };
> >
> > -static int module_callback(void *data, const char *name,
> > -                          struct module *mod, unsigned long addr)
> > +static int add_module(struct modules_array *arr, struct module *mod)
> >  {
> > -       struct module_addr_args *args = data;
> >         struct module **mods;
> >
> > -       /* We iterate all modules symbols and for each we:
> > -        * - search for it in provided addresses array
> > -        * - if found we check if we already have the module pointer stored
> > -        *   (we iterate modules sequentially, so we can check just the last
> > -        *   module pointer)
> > -        * - take module reference and store it
> > -        */
> > -       if (!bsearch(&addr, args->addrs, args->addrs_cnt, sizeof(addr),
> > -                      bpf_kprobe_multi_addrs_cmp))
> > -               return 0;
> > -
> > -       if (args->mods && args->mods[args->mods_cnt - 1] == mod)
> > -               return 0;
> > -
> > -       if (args->mods_cnt == args->mods_cap) {
> > -               args->mods_cap = max(16, args->mods_cap * 3 / 2);
> > -               mods = krealloc_array(args->mods, args->mods_cap, sizeof(*mods), GFP_KERNEL);
> > +       if (arr->mods_cnt == arr->mods_cap) {
> > +               arr->mods_cap = max(16, arr->mods_cap * 3 / 2);
> > +               mods = krealloc_array(arr->mods, arr->mods_cap, sizeof(*mods), GFP_KERNEL);
> >                 if (!mods)
> >                         return -ENOMEM;
> > -               args->mods = mods;
> > +               arr->mods = mods;
> >         }
> >
> > -       if (!try_module_get(mod))
> > -               return -EINVAL;
> > -
> > -       args->mods[args->mods_cnt] = mod;
> > -       args->mods_cnt++;
> > +       arr->mods[arr->mods_cnt] = mod;
> > +       arr->mods_cnt++;
> >         return 0;
> >  }
> >
> > +static bool has_module(struct modules_array *arr, struct module *mod)
> > +{
> > +       int i;
> > +
> > +       if (!arr->mods)
> > +               return false;
> > +       for (i = arr->mods_cnt; i >= 0; i--) {
> 
> This should be "i = arr->mods_cnt - 1", no?

right

> 
> > +               if (arr->mods[i] == mod)
> > +                       return true;
> > +       }
> > +       return false;
> > +}
> > +
> >  static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u32 addrs_cnt)
> >  {
> > -       struct module_addr_args args = {
> > -               .addrs     = addrs,
> > -               .addrs_cnt = addrs_cnt,
> > -       };
> > -       int err;
> > +       struct modules_array arr = {};
> > +       u32 i, err = 0;
> > +
> > +       for (i = 0; i < addrs_cnt; i++) {
> > +               struct module *mod;
> > +
> > +               preempt_disable();
> > +               mod = __module_address(addrs[i]);
> > +               /* Either no module or we it's already stored  */
> > +               if (!mod || (mod && has_module(&arr, mod))) {
> 
> nit: This can be simplified:
> 
>      if (!mod || has_module(&arr, mod)) {

yep, will change

thanks,
jirka