| Message ID | tencent_AB461510B10CD484E0B2F62E3754165F2909@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: trace_helpers.c: optimize kallsyms cache | expand |
On 08/10, Rong Tao wrote: > From: Rong Tao <rongtao@cestc.cn> > > Static ksyms often have problems because the number of symbols exceeds the > MAX_SYMS limit. Like changing the MAX_SYMS from 300000 to 400000 in > commit e76a014334a6("selftests/bpf: Bump and validate MAX_SYMS") solves > the problem somewhat, but it's not the perfect way. > > This commit uses dynamic memory allocation, which completely solves the > problem caused by the limitation of the number of kallsyms. Thank you for doing this! I do remember complaining about it on the last "let's bump the limit" patch :-D > Signed-off-by: Rong Tao <rongtao@cestc.cn> > --- > samples/bpf/offwaketime_user.c | 1 + > samples/bpf/sampleip_user.c | 1 + > samples/bpf/spintest_user.c | 1 + > samples/bpf/task_fd_query_user.c | 1 + > samples/bpf/trace_event_user.c | 1 + > .../selftests/bpf/prog_tests/bpf_cookie.c | 1 + > .../bpf/prog_tests/get_stack_raw_tp.c | 1 + > .../bpf/prog_tests/kprobe_multi_test.c | 2 + > .../prog_tests/kprobe_multi_testmod_test.c | 2 + > tools/testing/selftests/bpf/trace_helpers.c | 74 ++++++++++++------- > tools/testing/selftests/bpf/trace_helpers.h | 1 + > 11 files changed, 60 insertions(+), 26 deletions(-) > > diff --git a/samples/bpf/offwaketime_user.c b/samples/bpf/offwaketime_user.c > index b6eedcb98fb9..5e6934f2d932 100644 > --- a/samples/bpf/offwaketime_user.c > +++ b/samples/bpf/offwaketime_user.c > @@ -149,5 +149,6 @@ int main(int argc, char **argv) > bpf_link__destroy(links[i]); > > bpf_object__close(obj); > + free_kallsyms(); Do we really need to free the symbols? IOW, is it enough to load them once and keep them around until test_progs dies? If we do, I wonder what would happen when test_progs runs in parallel mode? Feels like if we're adding this alloc/free, then load_symbols should return some object which free_kallsyms should free? And ksym_get_addr would also work on that object, not on the global state.. > return 0; > } > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c > index 9283f47844fb..548ca1e9fcac 100644 > --- a/samples/bpf/sampleip_user.c > +++ b/samples/bpf/sampleip_user.c > @@ -230,5 +230,6 @@ int main(int argc, char **argv) > > free(links); > bpf_object__close(obj); > + free_kallsyms(); > return error; > } > diff --git a/samples/bpf/spintest_user.c b/samples/bpf/spintest_user.c > index aadac14f748a..895a64afaf78 100644 > --- a/samples/bpf/spintest_user.c > +++ b/samples/bpf/spintest_user.c > @@ -88,5 +88,6 @@ int main(int ac, char **argv) > bpf_link__destroy(links[j]); > > bpf_object__close(obj); > + free_kallsyms(); > return 0; > } > diff --git a/samples/bpf/task_fd_query_user.c b/samples/bpf/task_fd_query_user.c > index 1e61f2180470..3eb9477541fb 100644 > --- a/samples/bpf/task_fd_query_user.c > +++ b/samples/bpf/task_fd_query_user.c > @@ -419,5 +419,6 @@ int main(int argc, char **argv) > bpf_link__destroy(links[i]); > > bpf_object__close(obj); > + free_kallsyms(); > return err; > } > diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c > index 9664749bf618..a8b9343126fa 100644 > --- a/samples/bpf/trace_event_user.c > +++ b/samples/bpf/trace_event_user.c > @@ -348,5 +348,6 @@ int main(int argc, char **argv) > > cleanup: > bpf_object__close(obj); > + free_kallsyms(); > err_exit(error); > } > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > index 26b2d1bffdfd..4786396e554b 100644 > --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c > @@ -170,6 +170,7 @@ static void kprobe_multi_link_api_subtest(void) > cleanup: > close(link1_fd); > close(link2_fd); > + free_kallsyms(); > kprobe_multi__destroy(skel); > } > > diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c > index 858e0575f502..4e1c564746e1 100644 > --- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c > +++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c > @@ -146,4 +146,5 @@ void test_get_stack_raw_tp(void) > bpf_link__destroy(link); > perf_buffer__free(pb); > bpf_object__close(obj); > + free_kallsyms(); > } > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > index 2173c4bb555e..d6c0b5f2f887 100644 > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c > @@ -488,4 +488,6 @@ void test_kprobe_multi_test(void) > test_attach_api_syms(); > if (test__start_subtest("attach_api_fails")) > test_attach_api_fails(); > + > + free_kallsyms(); > } > diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c > index 1fbe7e4ac00a..c25f262832b7 100644 > --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c > +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c > @@ -86,4 +86,6 @@ void serial_test_kprobe_multi_testmod_test(void) > test_testmod_attach_api_syms(); > if (test__start_subtest("testmod_attach_api_addrs")) > test_testmod_attach_api_addrs(); > + > + free_kallsyms(); > } > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > index f83d9f65c65b..08986f70836b 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c > @@ -18,9 +18,32 @@ > #define TRACEFS_PIPE "/sys/kernel/tracing/trace_pipe" > #define DEBUGFS_PIPE "/sys/kernel/debug/tracing/trace_pipe" > > -#define MAX_SYMS 400000 > -static struct ksym syms[MAX_SYMS]; > -static int sym_cnt; > +static struct { > + struct ksym *syms; > + unsigned int sym_cnt; > +} ksyms = { > + .syms = NULL, > + .sym_cnt = 0, > +}; > + > +static int ksyms__add_symbol(const char *name, unsigned long addr) > +{ > + void *tmp; > + unsigned int cnt = ksyms.sym_cnt; > + > + cnt++; > + tmp = realloc(ksyms.syms, sizeof(struct ksym) * cnt); > + if (!tmp) > + return -ENOMEM; Should we do the usual len/capacity scheme here to amortize the cost of realloc (like doubling capacity when we reach it)? Calling realloc on every symbol doesn't seem right.
Thanks for your advise, you are right, i just submit v2 [0]. I just found that, because of the modified patch, your email address was not obtained through scripts/get_maintainer.pl, so the v2 [0] email was not sent to you, sorry. Rong Tao, Good day. [0] v2: https://lore.kernel.org/lkml/tencent_B655EE5E5D463110D70CD2846AB3262EED09@qq.com/
On 08/11, Rong Tao wrote: > Thanks for your advise, you are right, i just submit v2 [0]. > > I just found that, because of the modified patch, your email address was not > obtained through scripts/get_maintainer.pl, so the v2 [0] email was not sent > to you, sorry. No worries, as long as it reaches the list I'll get to it :-)
diff --git a/samples/bpf/offwaketime_user.c b/samples/bpf/offwaketime_user.c index b6eedcb98fb9..5e6934f2d932 100644 --- a/samples/bpf/offwaketime_user.c +++ b/samples/bpf/offwaketime_user.c @@ -149,5 +149,6 @@ int main(int argc, char **argv) bpf_link__destroy(links[i]); bpf_object__close(obj); + free_kallsyms(); return 0; } diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c index 9283f47844fb..548ca1e9fcac 100644 --- a/samples/bpf/sampleip_user.c +++ b/samples/bpf/sampleip_user.c @@ -230,5 +230,6 @@ int main(int argc, char **argv) free(links); bpf_object__close(obj); + free_kallsyms(); return error; } diff --git a/samples/bpf/spintest_user.c b/samples/bpf/spintest_user.c index aadac14f748a..895a64afaf78 100644 --- a/samples/bpf/spintest_user.c +++ b/samples/bpf/spintest_user.c @@ -88,5 +88,6 @@ int main(int ac, char **argv) bpf_link__destroy(links[j]); bpf_object__close(obj); + free_kallsyms(); return 0; } diff --git a/samples/bpf/task_fd_query_user.c b/samples/bpf/task_fd_query_user.c index 1e61f2180470..3eb9477541fb 100644 --- a/samples/bpf/task_fd_query_user.c +++ b/samples/bpf/task_fd_query_user.c @@ -419,5 +419,6 @@ int main(int argc, char **argv) bpf_link__destroy(links[i]); bpf_object__close(obj); + free_kallsyms(); return err; } diff --git a/samples/bpf/trace_event_user.c b/samples/bpf/trace_event_user.c index 9664749bf618..a8b9343126fa 100644 --- a/samples/bpf/trace_event_user.c +++ b/samples/bpf/trace_event_user.c @@ -348,5 +348,6 @@ int main(int argc, char **argv) cleanup: bpf_object__close(obj); + free_kallsyms(); err_exit(error); } diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c index 26b2d1bffdfd..4786396e554b 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c @@ -170,6 +170,7 @@ static void kprobe_multi_link_api_subtest(void) cleanup: close(link1_fd); close(link2_fd); + free_kallsyms(); kprobe_multi__destroy(skel); } diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c index 858e0575f502..4e1c564746e1 100644 --- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c +++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c @@ -146,4 +146,5 @@ void test_get_stack_raw_tp(void) bpf_link__destroy(link); perf_buffer__free(pb); bpf_object__close(obj); + free_kallsyms(); } diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c index 2173c4bb555e..d6c0b5f2f887 100644 --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c @@ -488,4 +488,6 @@ void test_kprobe_multi_test(void) test_attach_api_syms(); if (test__start_subtest("attach_api_fails")) test_attach_api_fails(); + + free_kallsyms(); } diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c index 1fbe7e4ac00a..c25f262832b7 100644 --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c @@ -86,4 +86,6 @@ void serial_test_kprobe_multi_testmod_test(void) test_testmod_attach_api_syms(); if (test__start_subtest("testmod_attach_api_addrs")) test_testmod_attach_api_addrs(); + + free_kallsyms(); } diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c index f83d9f65c65b..08986f70836b 100644 --- a/tools/testing/selftests/bpf/trace_helpers.c +++ b/tools/testing/selftests/bpf/trace_helpers.c @@ -18,9 +18,32 @@ #define TRACEFS_PIPE "/sys/kernel/tracing/trace_pipe" #define DEBUGFS_PIPE "/sys/kernel/debug/tracing/trace_pipe" -#define MAX_SYMS 400000 -static struct ksym syms[MAX_SYMS]; -static int sym_cnt; +static struct { + struct ksym *syms; + unsigned int sym_cnt; +} ksyms = { + .syms = NULL, + .sym_cnt = 0, +}; + +static int ksyms__add_symbol(const char *name, unsigned long addr) +{ + void *tmp; + unsigned int cnt = ksyms.sym_cnt; + + cnt++; + tmp = realloc(ksyms.syms, sizeof(struct ksym) * cnt); + if (!tmp) + return -ENOMEM; + + ksyms.sym_cnt = cnt; + ksyms.syms = tmp; + + ksyms.syms[ksyms.sym_cnt - 1].addr = addr; + ksyms.syms[ksyms.sym_cnt - 1].name = strdup(name); + + return 0; +} static int ksym_cmp(const void *p1, const void *p2) { @@ -33,9 +56,6 @@ int load_kallsyms_refresh(void) char func[256], buf[256]; char symbol; void *addr; - int i = 0; - - sym_cnt = 0; f = fopen("/proc/kallsyms", "r"); if (!f) @@ -46,16 +66,10 @@ int load_kallsyms_refresh(void) break; if (!addr) continue; - if (i >= MAX_SYMS) - return -EFBIG; - - syms[i].addr = (long) addr; - syms[i].name = strdup(func); - i++; + ksyms__add_symbol(func, (unsigned long)addr); } fclose(f); - sym_cnt = i; - qsort(syms, sym_cnt, sizeof(struct ksym), ksym_cmp); + qsort(ksyms.syms, ksyms.sym_cnt, sizeof(struct ksym), ksym_cmp); return 0; } @@ -65,48 +79,56 @@ int load_kallsyms(void) * This is called/used from multiplace places, * load symbols just once. */ - if (sym_cnt) + if (ksyms.sym_cnt) return 0; return load_kallsyms_refresh(); } +void free_kallsyms(void) +{ + if (!ksyms.sym_cnt) + return; + free(ksyms.syms); + ksyms.sym_cnt = 0; +} + struct ksym *ksym_search(long key) { - int start = 0, end = sym_cnt; + int start = 0, end = ksyms.sym_cnt; int result; /* kallsyms not loaded. return NULL */ - if (sym_cnt <= 0) + if (ksyms.sym_cnt <= 0) return NULL; while (start < end) { size_t mid = start + (end - start) / 2; - result = key - syms[mid].addr; + result = key - ksyms.syms[mid].addr; if (result < 0) end = mid; else if (result > 0) start = mid + 1; else - return &syms[mid]; + return &ksyms.syms[mid]; } - if (start >= 1 && syms[start - 1].addr < key && - key < syms[start].addr) + if (start >= 1 && ksyms.syms[start - 1].addr < key && + key < ksyms.syms[start].addr) /* valid ksym */ - return &syms[start - 1]; + return &ksyms.syms[start - 1]; /* out of range. return _stext */ - return &syms[0]; + return &ksyms.syms[0]; } long ksym_get_addr(const char *name) { int i; - for (i = 0; i < sym_cnt; i++) { - if (strcmp(syms[i].name, name) == 0) - return syms[i].addr; + for (i = 0; i < ksyms.sym_cnt; i++) { + if (strcmp(ksyms.syms[i].name, name) == 0) + return ksyms.syms[i].addr; } return 0; diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h index 876f3e711df6..d0d2bc4066c3 100644 --- a/tools/testing/selftests/bpf/trace_helpers.h +++ b/tools/testing/selftests/bpf/trace_helpers.h @@ -14,6 +14,7 @@ struct ksym { int load_kallsyms(void); int load_kallsyms_refresh(void); +void free_kallsyms(void); struct ksym *ksym_search(long key); long ksym_get_addr(const char *name);