[PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[Rong Tao]

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;
+	}
+
+	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)
-- 
2.39.3

Re: [PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[Jiri Olsa]

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
> 
> 

Re: [PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[Rong Tao]

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

Re: [PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[Jiri Olsa]

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
> 

Re: [PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[Rong Tao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 773 bytes --]

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

Re: [PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[Yonghong Song]

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
> 
> 

Re: [PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[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

Re: [PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[Yonghong Song]

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
> 
> 

Re: [PATCH bpf-next v5] selftests/bpf: trace_helpers.c: optimize kallsyms cache

[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/