| Message ID | tencent_50B4B2622FE7546A5FF9464310650C008509@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next,v3] selftests/bpf: trace_helpers.c: optimize kallsyms cache | expand |
On 08/12, 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. > > Signed-off-by: Rong Tao <rongtao@cestc.cn> I believe this is the one that won the pw race: https://patchwork.kernel.org/project/netdevbpf/patch/tencent_50B4B2622FE7546A5FF9464310650C008509@qq.com/ Acked-by: Stanislav Fomichev <sdf@google.com> > --- > v3: 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 | 42 ++++++++++++++++----- > 1 file changed, 32 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > index f83d9f65c65b..d8391a2122b4 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c > @@ -18,10 +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 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; Nit: reverse Christmas tree, not sure we care for the tests though..
On 8/12/23 7:57 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. > > Signed-off-by: Rong Tao <rongtao@cestc.cn> > --- > v3: 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 | 42 ++++++++++++++++----- > 1 file changed, 32 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > index f83d9f65c65b..d8391a2122b4 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c > @@ -18,10 +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 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; > + } > + > + syms[sym_cnt].addr = addr; > + syms[sym_cnt].name = strdup(name); Fwiw, strdup() should error check too.. and for teardown in the test suite, lets also have the counterpart where we release all the allocated mem. > + 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 +55,13 @@ int load_kallsyms_refresh(void) > char func[256], buf[256]; > char symbol; > void *addr; > - int i = 0; > + int ret; > > + sym_cap = 1024; On my dev node, I have: # cat /proc/kallsyms | wc -l 242586 Why starting out so low with 1k? I would have expected that for most cases we don't need the realloc() path to begin with, but just in corner cases like in e76a014334a6. > sym_cnt = 0; > + syms = malloc(sizeof(struct ksym) * sym_cap); > + if (!syms) > + return -ENOMEM; > > f = fopen("/proc/kallsyms", "r"); > if (!f) > @@ -46,15 +72,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. I just submit v4[0], make sure most cases we don't need the realloc() path to begin with, and check strdup() return value. [0] https://lore.kernel.org/lkml/tencent_59C74613113F0C728524B2A82FE5540A5E09@qq.com/ Rong Tao
diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c index f83d9f65c65b..d8391a2122b4 100644 --- a/tools/testing/selftests/bpf/trace_helpers.c +++ b/tools/testing/selftests/bpf/trace_helpers.c @@ -18,10 +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 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; + } + + syms[sym_cnt].addr = addr; + syms[sym_cnt].name = strdup(name); + + 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 +55,13 @@ int load_kallsyms_refresh(void) char func[256], buf[256]; char symbol; void *addr; - int i = 0; + int ret; + sym_cap = 1024; sym_cnt = 0; + syms = malloc(sizeof(struct ksym) * sym_cap); + if (!syms) + return -ENOMEM; f = fopen("/proc/kallsyms", "r"); if (!f) @@ -46,15 +72,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; }