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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
