selftests/bpf: trace_helpers.c: optimize kallsyms cache
Subsystems (4)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v10 1/2 selftests/bpf: trace_helpers.c: optimize kallsyms cache
From: Rong Tao <[email protected]>
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. At the same time, add APIs:
load_kallsyms_local()
ksym_search_local()
ksym_get_addr_local()
There are used to solve the problem of selftests/bpf updating kallsyms after attach new symbols during testmod testing.
---
v10: Keep the original load_kallsyms().
v9: https://lore.kernel.org/lkml/[email protected]/
Add load_kallsyms_local,ksym_search_local,ksym_get_addr_local functions.
v8: https://lore.kernel.org/lkml/[email protected]/
Resolves inter-thread contention for ksyms global variables.
v7: https://lore.kernel.org/lkml/[email protected]/
Fix __must_check macro.
v6: https://lore.kernel.org/lkml/[email protected]/
Apply libbpf_ensure_mem()
v5: https://lore.kernel.org/lkml/[email protected]/
Release the allocated memory once the load_kallsyms_refresh() upon error given it's dynamically allocated.
v4: https://lore.kernel.org/lkml/[email protected]/
Make sure most cases we don't need the realloc() path to begin with, and check strdup() return value.
v3: https://lore.kernel.org/lkml/[email protected]/
Do not use structs and judge ksyms__add_symbol function return value.
v2: https://lore.kernel.org/lkml/[email protected]/
Do the usual len/capacity scheme here to amortize the cost of realloc, and don't free symbols.
v1: https://lore.kernel.org/lkml/[email protected]/
--- samples/bpf/Makefile | 4 + .../selftests/bpf/prog_tests/fill_link_info.c | 9 +- .../prog_tests/kprobe_multi_testmod_test.c | 24 ++- tools/testing/selftests/bpf/trace_helpers.c | 137 +++++++++++++----- tools/testing/selftests/bpf/trace_helpers.h | 10 +- 5 files changed, 137 insertions(+), 47 deletions(-)
samples/bpf/Makefile +4 -0
@@ -175,6 +175,7 @@ TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/ TPROGS_CFLAGS += -I$(LIBBPF_INCLUDE) TPROGS_CFLAGS += -I$(srctree)/tools/include TPROGS_CFLAGS += -I$(srctree)/tools/perf +TPROGS_CFLAGS += -I$(srctree)/tools/lib TPROGS_CFLAGS += -DHAVE_ATTR_TEST=0 ifdef SYSROOT
@@ -314,6 +315,9 @@ XDP_SAMPLE_CFLAGS += -Wall -O2 \ $(obj)/$(XDP_SAMPLE): TPROGS_CFLAGS = $(XDP_SAMPLE_CFLAGS) $(obj)/$(XDP_SAMPLE): $(src)/xdp_sample_user.h $(src)/xdp_sample_shared.h +# Override includes for trace_helpers.o because __must_check won't be defined +# in our include path. +$(obj)/$(TRACE_HELPERS): TPROGS_CFLAGS := $(TPROGS_CFLAGS) -D__must_check= -include $(BPF_SAMPLES_PATH)/Makefile.target
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +6 -3
@@ -302,16 +302,18 @@ void test_fill_link_info(void) {struct test_fill_link_info *skel;int i; +struct ksyms *ksyms; skel =test_fill_link_info__open_and_load ();if (!ASSERT_OK_PTR (skel,"skel_open" ))return ;/* load kallsyms to compare the addr */ -if (!ASSERT_OK (load_kallsyms_refresh (),"load_kallsyms_refresh" )) + ksyms =load_kallsyms_local (); +if (!ASSERT_OK_PTR (ksyms,"load_kallsyms_local" )) goto cleanup; - kprobe_addr =ksym_get_addr (KPROBE_FUNC ); + kprobe_addr =ksym_get_addr_local (ksyms,KPROBE_FUNC );if (test__start_subtest ("kprobe_link_info" ))test_kprobe_fill_link_info (skel,BPF_PERF_EVENT_KPROBE , false);if (test__start_subtest ("kretprobe_link_info" ))
@@ -329,7 +331,7 @@ void test_fill_link_info(void)qsort (kmulti_syms,KMULTI_CNT ,sizeof (kmulti_syms[0 ]), symbols_cmp_r);for (i =0 ; i <KMULTI_CNT ; i++) - kmulti_addrs[i] =ksym_get_addr (kmulti_syms[i]); + kmulti_addrs[i] =ksym_get_addr_local (ksyms, kmulti_syms[i]);if (test__start_subtest ("kprobe_multi_link_info" ))test_kprobe_multi_fill_link_info (skel, false, false);if (test__start_subtest ("kretprobe_multi_link_info" ))
@@ -339,4 +341,5 @@ void test_fill_link_info(void) cleanup:test_fill_link_info__destroy (skel); +free_kallsyms_local (ksyms); }
tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c +17 -7
@@ -4,6 +4,8 @@#include "trace_helpers.h" #include "bpf/libbpf_internal.h" +static struct ksyms *ksyms; +static void kprobe_multi_testmod_check (struct kprobe_multi *skel) {ASSERT_EQ (skel->bss->kprobe_testmod_test1_result,1 ,"kprobe_test1_result" );
@@ -50,12 +52,12 @@ static void test_testmod_attach_api_addrs(void)LIBBPF_OPTS (bpf_kprobe_multi_opts, opts);unsigned long long addrs[3 ]; - addrs[0 ] =ksym_get_addr ("bpf_testmod_fentry_test1" ); -ASSERT_NEQ (addrs[0 ],0 ,"ksym_get_addr" ); - addrs[1 ] =ksym_get_addr ("bpf_testmod_fentry_test2" ); -ASSERT_NEQ (addrs[1 ],0 ,"ksym_get_addr" ); - addrs[2 ] =ksym_get_addr ("bpf_testmod_fentry_test3" ); -ASSERT_NEQ (addrs[2 ],0 ,"ksym_get_addr" ); + addrs[0 ] =ksym_get_addr_local (ksyms,"bpf_testmod_fentry_test1" ); +ASSERT_NEQ (addrs[0 ],0 ,"ksym_get_addr_local" ); + addrs[1 ] =ksym_get_addr_local (ksyms,"bpf_testmod_fentry_test2" ); +ASSERT_NEQ (addrs[1 ],0 ,"ksym_get_addr_local" ); + addrs[2 ] =ksym_get_addr_local (ksyms,"bpf_testmod_fentry_test3" ); +ASSERT_NEQ (addrs[2 ],0 ,"ksym_get_addr_local" ); opts.addrs = (const unsigned long *) addrs; opts.cnt =ARRAY_SIZE (addrs);
@@ -79,11 +81,19 @@ static void test_testmod_attach_api_syms(void)void serial_test_kprobe_multi_testmod_test (void ) { -if (!ASSERT_OK (load_kallsyms_refresh (),"load_kallsyms_refresh" )) + ksyms =load_kallsyms_local (); +if (!ASSERT_OK_PTR (ksyms,"load_kallsyms_local" ))return ;if (test__start_subtest ("testmod_attach_api_syms" ))test_testmod_attach_api_syms (); + + ksyms =load_kallsyms_refresh (ksyms); +if (!ASSERT_OK_PTR (ksyms,"load_kallsyms_refresh" )) +return ; +if (test__start_subtest ("testmod_attach_api_addrs" ))test_testmod_attach_api_addrs (); + +free_kallsyms_local (ksyms); }
tools/testing/selftests/bpf/trace_helpers.c +102 -35
206 lines not on this page · raw message
tools/testing/selftests/bpf/trace_helpers.h +8 -2
@@ -11,13 +11,19 @@ struct ksym {long addr;char *name; }; +struct ksyms ;int load_kallsyms (void ); -int load_kallsyms_refresh (void ); -struct ksym *ksym_search (long key);long ksym_get_addr (const char *name); +struct ksyms *load_kallsyms_local (void ); +struct ksym *ksym_search_local (struct ksyms *ksyms,long key); +long ksym_get_addr_local (struct ksyms *ksyms,const char *name); + +struct ksyms *load_kallsyms_refresh (struct ksyms *ksyms); +void free_kallsyms_local (struct ksyms *ksyms); +/* open kallsyms and find addresses on the fly, faster than load + search. */ int kallsyms_find (const char *sym,unsigned long long *addr);
Signature · 1 line
2.41.0
Quoted 7 lines · Rong Tao
From: Rong Tao <[email protected]>
Static ksyms often have problems because the number of symbols exceeds the
MAX_SYMSlimit. Like changing theMAX_SYMSfrom 300000 to 400000 in commite76a014334a6("selftests/bpf: Bump and validate MAX_SYMS")solves the problem somewhat, but it's not the perfect way.
Quoted 7 lines
This commit uses dynamic memory allocation, which completely solves the problem caused by the limitation of the number of kallsyms. At the same time, add APIs:
load_kallsyms_local()
ksym_search_local()
ksym_get_addr_local()
missing free_kallsyms_local
Quoted 9 lines
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +1
-0@@ -302,16 +302,18 @@ void test_fill_link_info(void) {struct test_fill_link_info *skel;int i; +struct ksyms *ksyms;
tools/testing/selftests/bpf/prog_tests/fill_link_info.c void test_fill_link_info(void) · line 314
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +2
-1skel =test_fill_link_info__open_and_load ();if (!ASSERT_OK_PTR (skel,"skel_open" ))return ;/* load kallsyms to compare the addr */ -if (!ASSERT_OK (load_kallsyms_refresh (),"load_kallsyms_refresh" )) + ksyms =load_kallsyms_local (); +if (!ASSERT_OK_PTR (ksyms,"load_kallsyms_local" )) goto cleanup;
actually I don't see why this test should need refreshed kallsyms, it doesn't load/unload testmod, I think it can use global ksyms
Quoted 58 lines
tools/testing/selftests/bpf/prog_tests/fill_link_info.c +3
-2- kprobe_addr =ksym_get_addr (KPROBE_FUNC ); + kprobe_addr =ksym_get_addr_local (ksyms,KPROBE_FUNC );if (test__start_subtest ("kprobe_link_info" ))test_kprobe_fill_link_info (skel,BPF_PERF_EVENT_KPROBE , false);if (test__start_subtest ("kretprobe_link_info" ))@@ -329,7 +331,7 @@ void test_fill_link_info(void)qsort (kmulti_syms,KMULTI_CNT ,sizeof (kmulti_syms[0 ]), symbols_cmp_r);for (i =0 ; i <KMULTI_CNT ; i++) - kmulti_addrs[i] =ksym_get_addr (kmulti_syms[i]); + kmulti_addrs[i] =ksym_get_addr_local (ksyms, kmulti_syms[i]);if (test__start_subtest ("kprobe_multi_link_info" ))test_kprobe_multi_fill_link_info (skel, false, false);if (test__start_subtest ("kretprobe_multi_link_info" ))@@ -339,4 +341,5 @@ void test_fill_link_info(void) cleanup:test_fill_link_info__destroy (skel); +free_kallsyms_local (ksyms); }tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c +8
-7@@ -4,6 +4,8 @@#include "trace_helpers.h" #include "bpf/libbpf_internal.h" +static struct ksyms *ksyms; +static void kprobe_multi_testmod_check (struct kprobe_multi *skel) {ASSERT_EQ (skel->bss->kprobe_testmod_test1_result,1 ,"kprobe_test1_result" );@@ -50,12 +52,12 @@ static void test_testmod_attach_api_addrs(void)LIBBPF_OPTS (bpf_kprobe_multi_opts, opts);unsigned long long addrs[3 ]; - addrs[0 ] =ksym_get_addr ("bpf_testmod_fentry_test1" ); -ASSERT_NEQ (addrs[0 ],0 ,"ksym_get_addr" ); - addrs[1 ] =ksym_get_addr ("bpf_testmod_fentry_test2" ); -ASSERT_NEQ (addrs[1 ],0 ,"ksym_get_addr" ); - addrs[2 ] =ksym_get_addr ("bpf_testmod_fentry_test3" ); -ASSERT_NEQ (addrs[2 ],0 ,"ksym_get_addr" ); + addrs[0 ] =ksym_get_addr_local (ksyms,"bpf_testmod_fentry_test1" ); +ASSERT_NEQ (addrs[0 ],0 ,"ksym_get_addr_local" ); + addrs[1 ] =ksym_get_addr_local (ksyms,"bpf_testmod_fentry_test2" ); +ASSERT_NEQ (addrs[1 ],0 ,"ksym_get_addr_local" ); + addrs[2 ] =ksym_get_addr_local (ksyms,"bpf_testmod_fentry_test3" ); +ASSERT_NEQ (addrs[2 ],0 ,"ksym_get_addr_local" ); opts.addrs = (const unsigned long *) addrs; opts.cnt =ARRAY_SIZE (addrs);@@ -79,11 +81,19 @@ static void test_testmod_attach_api_syms(void)void serial_test_kprobe_multi_testmod_test (void ) { -if (!ASSERT_OK (load_kallsyms_refresh (),"load_kallsyms_refresh" ))
tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c static void test_testmod_attach_api_syms(void) · line 93
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c +6
-0+ ksyms =load_kallsyms_local (); +if (!ASSERT_OK_PTR (ksyms,"load_kallsyms_local" ))return ;if (test__start_subtest ("testmod_attach_api_syms" ))test_testmod_attach_api_syms (); + + ksyms =load_kallsyms_refresh (ksyms); +if (!ASSERT_OK_PTR (ksyms,"load_kallsyms_refresh" )) +return ;
hm, this refresh is not needed right? the test got the fresh kallsyms above and both test_testmod_attach_api_syms and test_testmod_attach_api_addrs should be happy
Quoted 22 lines
tools/testing/selftests/bpf/prog_tests/kprobe_multi_testmod_test.c +3
-0+if (test__start_subtest ("testmod_attach_api_addrs" ))test_testmod_attach_api_addrs (); + +free_kallsyms_local (ksyms); }tools/testing/selftests/bpf/trace_helpers.c +1
-3@@ -14,104 +14,171 @@#include <linux/limits.h> #include <libelf.h> #include <gelf.h> +#include "bpf/libbpf_internal.h" #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;
tools/testing/selftests/bpf/trace_helpers.c line 31
Quoted 10 lines
tools/testing/selftests/bpf/trace_helpers.c +10
-0+struct ksyms { +struct ksym *syms; +size_t sym_cap; +size_t sym_cnt; +}; + +static struct ksyms *ksyms; + +static int ksyms__add_symbol (struct ksyms *ksyms,const char *name, +unsigned long addr)
extra white space in here ^^^
Quoted 49 lines
tools/testing/selftests/bpf/trace_helpers.c +34
-3+{ +void *tmp; + + tmp =strdup (name); +if (!tmp) +return -ENOMEM ; + ksyms->syms[ksyms->sym_cnt].addr = addr; + ksyms->syms[ksyms->sym_cnt].name = tmp; + + ksyms->sym_cnt++; + +return 0 ; +} + +void free_kallsyms_local (struct ksyms *ksyms) +{ +unsigned int i; + +if (!ksyms) +return ; + +if (!ksyms->syms) { +free (ksyms); +return ; + } + +for (i =0 ; i < ksyms->sym_cnt; i++) +free (ksyms->syms[i].name); +free (ksyms->syms); +free (ksyms); +}static int ksym_cmp (const void *p1,const void *p2) {return ((struct ksym *)p1)->addr - ((struct ksym *)p2)->addr; } -int load_kallsyms_refresh (void ) +struct ksyms *load_kallsyms_refresh (struct ksyms *ksyms) {FILE *f;char func[256 ], buf[256 ];char symbol;void *addr; -int i =0 ; +int ret; - sym_cnt =0 ; +/* flush kallsyms, free the previously allocated dynamic memory */
tools/testing/selftests/bpf/trace_helpers.c line 86
Quoted 10 lines
tools/testing/selftests/bpf/trace_helpers.c +6
-1+free_kallsyms_local (ksyms); f =fopen ("/proc/kallsyms" ,"r" );if (!f) -return -ENOENT ; +return NULL ; + + ksyms =calloc (1 ,sizeof (struct ksyms )); +if (!ksyms) +return NULL ;
I wonder it could be easier not to allocate ksyms, but just let the caller to pass the pointer to 'ksym' variable on stack.. but there might be other problems, so I guess this is fine
Quoted 25 lines
tools/testing/selftests/bpf/trace_helpers.c +8
-8while (fgets (buf,sizeof (buf), f)) {if (sscanf (buf,"%p %c %s" , &addr, &symbol, func) !=3 )break ;if (!addr)continue ; -if (i >=MAX_SYMS ) -return -EFBIG ; - syms[i].addr = (long ) addr; - syms[i].name =strdup (func); - i++; + ret =libbpf_ensure_mem ((void **) &ksyms->syms, &ksyms->sym_cap, +sizeof (struct ksym ), ksyms->sym_cnt +1 ); +if (ret) + goto error; + ret =ksyms__add_symbol (ksyms, func, (unsigned long )addr); +if (ret) + goto error; }fclose (f); - sym_cnt = i; -qsort (syms, sym_cnt,sizeof (struct ksym ), ksym_cmp); -return 0 ; +qsort (ksyms->syms, ksyms->sym_cnt,sizeof (struct ksym ), ksym_cmp);
tools/testing/selftests/bpf/trace_helpers.c line 113
Quoted 10 lines
tools/testing/selftests/bpf/trace_helpers.c +10
-0+return ksyms; + +error: +free_kallsyms_local (ksyms); +return NULL ; +} + +struct ksyms *load_kallsyms_local (void ) +{ +return load_kallsyms_refresh (NULL );
I think we can have just:
struct ksyms *load_kallsyms_local(struct ksyms *ksyms);
I don't see reason for load_kallsyms_refresh functions, perhaps it's leftover because I don't see the function in the changelog
thanks,
jirka
Hi, jirka.
I just fix all your advise[0], and submit new patchset v11. please review.
Rong Tao
PATCH v10 2/2 selftests/bpf: trace_helpers.c: Add a global ksyms initialization mutex
In reply to a message not in the archiveFrom: Rong Tao <[email protected]>
As Jirka said [0], we just need to make sure that global ksyms initialization won't race.
[0] https://lore.kernel.org/lkml/ZPCbAs3ItjRd8XVh@krava/
--- tools/testing/selftests/bpf/trace_helpers.c | 4 ++++ 1 file changed, 4 insertions(+)
tools/testing/selftests/bpf/trace_helpers.c +4 -0
@@ -7,6 +7,7 @@#include <errno.h> #include <fcntl.h> #include <poll.h> +#include <pthread.h> #include <unistd.h> #include <linux/perf_event.h> #include <sys/mman.h>
@@ -26,6 +27,7 @@ struct ksyms { };static struct ksyms *ksyms; +static pthread_mutex_t ksyms_mutex =PTHREAD_MUTEX_INITIALIZER ;static int ksyms__add_symbol (struct ksyms *ksyms,const char *name,unsigned long addr)
@@ -115,8 +117,10 @@ struct ksyms *load_kallsyms_local(void)int load_kallsyms (void ) { +pthread_mutex_lock (&ksyms_mutex);if (!ksyms) ksyms =load_kallsyms_local (); +pthread_mutex_unlock (&ksyms_mutex);return ksyms ?0 :1 ; }
Signature · 1 line
2.41.0