On Fri, Jan 13, 2023 at 12:41:50PM -0800, Song Liu wrote:

On Fri, Jan 13, 2023 at 6:44 AM Jiri Olsa [off-list ref] wrote:

quoted

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

quoted

+               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