| Message ID | tencent_59C74613113F0C728524B2A82FE5540A5E09@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v4] selftests/bpf: trace_helpers.c: optimize kallsyms cache | expand |
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
Thanks, Daniel I just add ksyms__free() function, it is called when ksyms__add_symbol() fails. Good Day, Rong Tao
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; +} + 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; }