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?
> + 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)) {
> + preempt_enable();
> + continue;
> + }
[...]
