Messages in this thread | | | Subject | Re: [PATCH bpf-next v4] selftests/bpf: trace_helpers.c: optimize kallsyms cache | From | Daniel Borkmann <> | Date | Wed, 16 Aug 2023 18:58:37 +0200 |
| |
On 8/16/23 3:36 AM, 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> > --- > v4: 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 | 46 ++++++++++++++++----- > 1 file changed, 36 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > index f83d9f65c65b..a1461508925e 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c > @@ -18,10 +18,35 @@ > #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; > +}
Since this patch is about improving the load_kallsyms_refresh(), I mentioned in the v3 that it would also be good to have the counterpart to release the allocated memory once the test concluded or upon error given it's dynamically allocated.
> static int ksym_cmp(const void *p1, const void *p2) > { > return ((struct ksym *)p1)->addr - ((struct ksym *)p2)->addr; > @@ -33,9 +58,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,15 +76,11 @@ 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) > + return ret; > } > fclose(f); > - sym_cnt = i; > qsort(syms, sym_cnt, sizeof(struct ksym), ksym_cmp); > return 0; > } >
Thanks, Daniel
| |