| Message ID | tencent_0E9E1A1C0981678D5E7EA9E4BDBA8EE2200A@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next,v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache | expand |
On Thu, Aug 17, 2023 at 01:03:45PM +0800, 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. > > Acked-by: Stanislav Fomichev <sdf@google.com> > Signed-off-by: Rong Tao <rongtao@cestc.cn> > --- > v5: Release the allocated memory once the load_kallsyms_refresh() upon error > given it's dynamically allocated. > v4: https://lore.kernel.org/lkml/tencent_59C74613113F0C728524B2A82FE5540A5E09@qq.com/ > 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/tencent_50B4B2622FE7546A5FF9464310650C008509@qq.com/ > Do not use structs and judge ksyms__add_symbol function return value. > v2: https://lore.kernel.org/lkml/tencent_B655EE5E5D463110D70CD2846AB3262EED09@qq.com/ > Do the usual len/capacity scheme here to amortize the cost of realloc, and > don't free symbols. > v1: https://lore.kernel.org/lkml/tencent_AB461510B10CD484E0B2F62E3754165F2909@qq.com/ > --- > tools/testing/selftests/bpf/trace_helpers.c | 62 +++++++++++++++++---- > 1 file changed, 52 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > index f83d9f65c65b..0053ba22f0cb 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c > @@ -18,10 +18,47 @@ > #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 struct ksym *syms; > +static int sym_cap; > static int sym_cnt; > > +static int ksyms__add_symbol(const char *name, unsigned long addr) > +{ > + void *tmp; > + unsigned int new_cap; > + > + if (sym_cnt + 1 > sym_cap) { > + new_cap = sym_cap * 4 / 3; > + tmp = realloc(syms, sizeof(struct ksym) * new_cap); > + if (!tmp) > + return -ENOMEM; > + syms = tmp; > + sym_cap = new_cap; > + } sorry I did not notice earlier, but we have helper for realloc libbpf_ensure_mem check the usage for example in prog_tests/kprobe_multi_test.c > + > + tmp = strdup(name); > + if (!tmp) > + return -ENOMEM; > + syms[sym_cnt].addr = addr; > + syms[sym_cnt].name = tmp; > + > + sym_cnt++; > + > + return 0; > +} > + > +static void ksyms__free(void) > +{ > + unsigned int i; > + > + if (!syms) > + return; > + > + for (i = 0; i < sym_cnt; i++) > + free(syms[i].name); > + free(syms); > +} > + > static int ksym_cmp(const void *p1, const void *p2) > { > return ((struct ksym *)p1)->addr - ((struct ksym *)p2)->addr; > @@ -33,9 +70,14 @@ int load_kallsyms_refresh(void) > char func[256], buf[256]; > char symbol; > void *addr; > - int i = 0; > + int ret; > > + /* Make sure most cases we don't need the realloc() path to begin with */ > + sym_cap = 400000; > sym_cnt = 0; > + syms = malloc(sizeof(struct ksym) * sym_cap); > + if (!syms) > + return -ENOMEM; libbpf_ensure_mem will also take care of first allocation and the capacity increase jirka > > f = fopen("/proc/kallsyms", "r"); > if (!f) > @@ -46,17 +88,17 @@ 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++; > + ret = ksyms__add_symbol(func, (unsigned long)addr); > + if (ret) > + goto error; > } > fclose(f); > - sym_cnt = i; > qsort(syms, sym_cnt, sizeof(struct ksym), ksym_cmp); > return 0; > + > +error: > + ksyms__free(); > + return ret; > } > > int load_kallsyms(void) > -- > 2.39.3 > >
Hi, Jiri. Thanks for your reply. libbpf_ensure_mem() is in libbpf_internal.h, samples/bpf/ can't see it, do we reposition the function declaration? Good Day Rong Tao
On Fri, Aug 18, 2023 at 09:39:32AM +0800, Rong Tao wrote: > Hi, Jiri. Thanks for your reply. > > libbpf_ensure_mem() is in libbpf_internal.h, samples/bpf/ can't see it, do we > reposition the function declaration? I see, I had no idea trace_helpers.[ch] is included in samples/bpf but it seems to have libbpf linked statically.. so you could just include libbpf_internal.h and it should work, right? like we do in selftests jirka > > Good Day > Rong Tao >
Hi, jirka
Sadly, we can't include libbpf_internal.h in trace_helpers.{h,c}.
we only have the following headers when compile samples/bpf/:
tree of samples/bpf/libbpf/
+-- bpf_helper_defs.h
+-- include
| '-- bpf
| +-- bpf_core_read.h
| +-- bpf_endian.h
| +-- bpf.h
| +-- bpf_helper_defs.h
| +-- bpf_helpers.h
| +-- bpf_tracing.h
| +-- btf.h
| +-- libbpf_common.h
| +-- libbpf.h
| +-- libbpf_legacy.h
| +-- libbpf_version.h
| +-- skel_internal.h
| '-- usdt.bpf.h
+-- libbpf.a
No libbpf_internal.h here.
What if we add a declaration to libbpf_ensure_mem() in trace_helpers.c?
Good Day,
Rong Tao
On 8/18/23 8:36 AM, Rong Tao wrote: > Hi, jirka > > Sadly, we can't include libbpf_internal.h in trace_helpers.{h,c}. > we only have the following headers when compile samples/bpf/: > > tree of samples/bpf/libbpf/ > +-- bpf_helper_defs.h > +-- include > | '-- bpf > | +-- bpf_core_read.h > | +-- bpf_endian.h > | +-- bpf.h > | +-- bpf_helper_defs.h > | +-- bpf_helpers.h > | +-- bpf_tracing.h > | +-- btf.h > | +-- libbpf_common.h > | +-- libbpf.h > | +-- libbpf_legacy.h > | +-- libbpf_version.h > | +-- skel_internal.h > | '-- usdt.bpf.h > +-- libbpf.a > > No libbpf_internal.h here. > > What if we add a declaration to libbpf_ensure_mem() in trace_helpers.c? [~/work/bpf-next/tools/testing/selftests/bpf/prog_tests (master)]$ grep libbpf_internal.h * cpu_mask.c:#include "bpf/libbpf_internal.h" kprobe_multi_test.c:#include "bpf/libbpf_internal.h" kprobe_multi_testmod_test.c:#include "bpf/libbpf_internal.h" module_fentry_shadow.c:#include "bpf/libbpf_internal.h" perf_branches.c:#include "bpf/libbpf_internal.h" perf_buffer.c:#include "bpf/libbpf_internal.h" raw_tp_test_run.c:#include "bpf/libbpf_internal.h" [~/work/bpf-next/tools/testing/selftests/bpf/prog_tests (master)]$ grep libbpf_ensure_mem *.c kprobe_multi_test.c: err = libbpf_ensure_mem((void **) &syms, &cap, [~/work/bpf-next/tools/testing/selftests/bpf/prog_tests (master)]$ Looks like it is already used for selftets. The libbpf_internal.h exists in the following directory when you build the selftest. [~/work/bpf-next/tools/testing/selftests/bpf/tools/include/bpf (master)]$ ls bpf_core_read.h bpf.h bpf_helpers.h btf.h libbpf_common.h libbpf_internal.h libbpf_version.h relo_core.h usdt.bpf.h bpf_endian.h bpf_helper_defs.h bpf_tracing.h hashmap.h libbpf.h libbpf_legacy.h nlattr.h skel_internal.h > > Good Day, > Rong Tao > >
Sorry Song, I did not state clear. libbpf_ensure_mem() is declared in libbpf_internal.h, we want to use libbpf_ensure_mem() in trace_helpers.c, Unforturnately, we could only include the headers 'install_headers:' defined in tools/lib/bpf/Makefile, the 'install_headers:' target does not include libbpf_internal.h, like: tools/testing/selftests/bpf/trace_helpers.c:17:10: fatal error: libbpf_internal.h: No such file or directory 17 | #include "libbpf_internal.h" | ^~~~~~~~~~~~~~~~~~~ tools/testing/selftests/bpf/trace_helpers.c:17:10: fatal error: bpf/libbpf_internal.h: No such file or directory 17 | #include "bpf/libbpf_internal.h" | ^~~~~~~~~~~~~~~~~~~~~~~ How about 1. dup-declare libbpf_ensure_mem() in trace_helpers.c 2. move libbpf_ensure_mem() declare into libbpf_common.h Which one do you like best. Best wishes, Rong Tao
On 8/20/23 7:03 PM, Rong Tao wrote: > Sorry Song, I did not state clear. > > libbpf_ensure_mem() is declared in libbpf_internal.h, we want to use > libbpf_ensure_mem() in trace_helpers.c, Unforturnately, we could only include > the headers 'install_headers:' defined in tools/lib/bpf/Makefile, the > 'install_headers:' target does not include libbpf_internal.h, like: > > tools/testing/selftests/bpf/trace_helpers.c:17:10: > fatal error: libbpf_internal.h: No such file or directory > 17 | #include "libbpf_internal.h" > | ^~~~~~~~~~~~~~~~~~~ > > tools/testing/selftests/bpf/trace_helpers.c:17:10: > fatal error: bpf/libbpf_internal.h: No such file or directory > 17 | #include "bpf/libbpf_internal.h" > | ^~~~~~~~~~~~~~~~~~~~~~~ It works fine for me. On top of your patch, the following works fine: diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c index 0053ba22f0cb..087383c5dc3a 100644 --- a/tools/testing/selftests/bpf/trace_helpers.c +++ b/tools/testing/selftests/bpf/trace_helpers.c @@ -14,6 +14,7 @@ #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" With V=1 when building selftests, the following is the compilation command line: clang --target=x86_64-linux-gnu -fintegrated-as -g -O0 -rdynamic -Wall -Werror -I/home/yhs/work/bpf-next/tools/testing/selftests/bpf -I/home/yhs/work/bpf-next/tools/testing/selftests/bpf/tools/include -I/home/yhs/work/bpf-next/include/generated -I/home/yhs/work/bpf-next/tools/lib -I/home/yhs/work/bpf-next/tools/include -I/home/yhs/work/bpf-next/tools/include/uapi -I/home/yhs/work/bpf-next/tools/testing/selftests/bpf -Wno-unused-command-line-argument -c trace_helpers.c -lelf -lz -lrt -lpthread -o /home/yhs/work/bpf-next/tools/testing/selftests/bpf/trace_helpers.o This include path /home/yhs/work/bpf-next/tools/testing/selftests/bpf/tools/include will make including 'bpf/libbpf_internal.h' work. > > How about > > 1. dup-declare libbpf_ensure_mem() in trace_helpers.c > 2. move libbpf_ensure_mem() declare into libbpf_common.h > > Which one do you like best. > > Best wishes, > Rong Tao > >
tools/testing/selftests/bpf/ compile OK, however, samples/bpf/ compile failed. >> tools/testing/selftests/bpf/trace_helpers.c:17:10: >> fatal error: libbpf_internal.h: No such file or directory >> 17 | #include "libbpf_internal.h" >> | ^~~~~~~~~~~~~~~~~~~ >> >> tools/testing/selftests/bpf/trace_helpers.c:17:10: >> fatal error: bpf/libbpf_internal.h: No such file or directory >> 17 | #include "bpf/libbpf_internal.h" >> | ^~~~~~~~~~~~~~~~~~~~~~~ I just submit v6 [0], which apply libbpf_ensure_mem() and TPROGS_CFLAGS += -I$(srctree)/tools/lib to samples/bpf/Makefile, make sure, samples/bpf/Makefile could found libbpf_internal.h. Please review, thanks. Rong Tao [0] https://lore.kernel.org/lkml/tencent_4A09A36F883A06EA428A593497642AF8AF08@qq.com/
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c index f83d9f65c65b..0053ba22f0cb 100644 --- a/tools/testing/selftests/bpf/trace_helpers.c +++ b/tools/testing/selftests/bpf/trace_helpers.c @@ -18,10 +18,47 @@ #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 struct ksym *syms; +static int sym_cap; static int sym_cnt; +static int ksyms__add_symbol(const char *name, unsigned long addr) +{ + void *tmp; + unsigned int new_cap; + + if (sym_cnt + 1 > sym_cap) { + new_cap = sym_cap * 4 / 3; + tmp = realloc(syms, sizeof(struct ksym) * new_cap); + if (!tmp) + return -ENOMEM; + syms = tmp; + sym_cap = new_cap; + } + + tmp = strdup(name); + if (!tmp) + return -ENOMEM; + syms[sym_cnt].addr = addr; + syms[sym_cnt].name = tmp; + + sym_cnt++; + + return 0; +} + +static void ksyms__free(void) +{ + unsigned int i; + + if (!syms) + return; + + for (i = 0; i < sym_cnt; i++) + free(syms[i].name); + free(syms); +} + static int ksym_cmp(const void *p1, const void *p2) { return ((struct ksym *)p1)->addr - ((struct ksym *)p2)->addr; @@ -33,9 +70,14 @@ int load_kallsyms_refresh(void) char func[256], buf[256]; char symbol; void *addr; - int i = 0; + int ret; + /* Make sure most cases we don't need the realloc() path to begin with */ + sym_cap = 400000; sym_cnt = 0; + syms = malloc(sizeof(struct ksym) * sym_cap); + if (!syms) + return -ENOMEM; f = fopen("/proc/kallsyms", "r"); if (!f) @@ -46,17 +88,17 @@ 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++; + ret = ksyms__add_symbol(func, (unsigned long)addr); + if (ret) + goto error; } fclose(f); - sym_cnt = i; qsort(syms, sym_cnt, sizeof(struct ksym), ksym_cmp); return 0; + +error: + ksyms__free(); + return ret; } int load_kallsyms(void)