[PATCH v2 dwarves] btf: Remove ftrace filter

[PATCH v2 dwarves] btf: Remove ftrace filter

[Martin KaFai Lau]

BTF is currently generated for functions that are in ftrace list
or extern.

A recent use case also needs BTF generated for functions included in
allowlist.  In particular, the kernel
commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
allows bpf program to directly call a few tcp cc kernel functions. Those
kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
is set to ensure they are in the ftrace list but this kconfig dependency
is unnecessary.

Those kernel functions are specified under an ELF section .BTF_ids.
There was an earlier attempt [0] to add another filter for the functions in
the .BTF_ids section.  That discussion concluded that the ftrace filter
should be removed instead.

This patch is to remove the ftrace filter and its related functions.

Number of BTF FUNC with and without is_ftrace_func():
My kconfig in x86: 40643 vs 46225
Jiri reported on arm: 25022 vs 55812

[0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/

Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
v2: Remove sym_sec_idx, last_idx, and sh. (Jiri Olsa)

 btf_encoder.c | 285 ++------------------------------------------------
 1 file changed, 7 insertions(+), 278 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 80e896961d4e..c711f124b31e 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -27,17 +27,8 @@
  */
 #define KSYM_NAME_LEN 128
 
-struct funcs_layout {
-	unsigned long mcount_start;
-	unsigned long mcount_stop;
-	unsigned long mcount_sec_idx;
-};
-
 struct elf_function {
 	const char	*name;
-	unsigned long	 addr;
-	unsigned long	 size;
-	unsigned long	 sh_addr;
 	bool		 generated;
 };
 
@@ -64,12 +55,9 @@ static void delete_functions(void)
 #define max(x, y) ((x) < (y) ? (y) : (x))
 #endif
 
-static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
-			    size_t sym_sec_idx)
+static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
 {
 	struct elf_function *new;
-	static GElf_Shdr sh;
-	static size_t last_idx;
 	const char *name;
 
 	if (elf_sym__type(sym) != STT_FUNC)
@@ -91,257 +79,12 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
 		functions = new;
 	}
 
-	if (sym_sec_idx != last_idx) {
-		if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
-			return 0;
-		last_idx = sym_sec_idx;
-	}
-
 	functions[functions_cnt].name = name;
-	functions[functions_cnt].addr = elf_sym__value(sym);
-	functions[functions_cnt].size = elf_sym__size(sym);
-	functions[functions_cnt].sh_addr = sh.sh_addr;
 	functions[functions_cnt].generated = false;
 	functions_cnt++;
 	return 0;
 }
 
-static int addrs_cmp(const void *_a, const void *_b)
-{
-	const __u64 *a = _a;
-	const __u64 *b = _b;
-
-	if (*a == *b)
-		return 0;
-	return *a < *b ? -1 : 1;
-}
-
-static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
-			     __u64 **paddrs, __u64 *pcount)
-{
-	__u64 *addrs, count, offset;
-	unsigned int addr_size, i;
-	Elf_Data *data;
-	GElf_Shdr shdr;
-	Elf_Scn *sec;
-
-	/* Initialize for the sake of all error paths below. */
-	*paddrs = NULL;
-	*pcount = 0;
-
-	if (!fl->mcount_start || !fl->mcount_stop)
-		return 0;
-
-	/*
-	 * Find mcount addressed marked by __start_mcount_loc
-	 * and __stop_mcount_loc symbols and load them into
-	 * sorted array.
-	 */
-	sec = elf_getscn(btfe->elf, fl->mcount_sec_idx);
-	if (!sec || !gelf_getshdr(sec, &shdr)) {
-		fprintf(stderr, "Failed to get section(%lu) header.\n",
-			fl->mcount_sec_idx);
-		return -1;
-	}
-
-	/* Get address size from processed file's ELF class. */
-	addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
-
-	offset = fl->mcount_start - shdr.sh_addr;
-	count  = (fl->mcount_stop - fl->mcount_start) / addr_size;
-
-	data = elf_getdata(sec, 0);
-	if (!data) {
-		fprintf(stderr, "Failed to get section(%lu) data.\n",
-			fl->mcount_sec_idx);
-		return -1;
-	}
-
-	addrs = malloc(count * sizeof(addrs[0]));
-	if (!addrs) {
-		fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
-		return -1;
-	}
-
-	if (addr_size == sizeof(__u64)) {
-		memcpy(addrs, data->d_buf + offset, count * addr_size);
-	} else {
-		for (i = 0; i < count; i++)
-			addrs[i] = (__u64) *((__u32 *) (data->d_buf + offset + i * addr_size));
-	}
-
-	*paddrs = addrs;
-	*pcount = count;
-	return 0;
-}
-
-static int
-get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
-{
-	__u64 *addrs, count;
-	unsigned int addr_size, i;
-	GElf_Shdr shdr_mcount;
-	Elf_Data *data;
-	Elf_Scn *sec;
-
-	/* Initialize for the sake of all error paths below. */
-	*paddrs = NULL;
-	*pcount = 0;
-
-	/* get __mcount_loc */
-	sec = elf_section_by_name(btfe->elf, &btfe->ehdr, &shdr_mcount,
-				  "__mcount_loc", NULL);
-	if (!sec) {
-		if (btf_elf__verbose) {
-			printf("%s: '%s' doesn't have __mcount_loc section\n", __func__,
-			       btfe->filename);
-		}
-		return 0;
-	}
-
-	data = elf_getdata(sec, NULL);
-	if (!data) {
-		fprintf(stderr, "Failed to data for __mcount_loc section.\n");
-		return -1;
-	}
-
-	/* Get address size from processed file's ELF class. */
-	addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
-
-	count = data->d_size / addr_size;
-
-	addrs = malloc(count * sizeof(addrs[0]));
-	if (!addrs) {
-		fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
-		return -1;
-	}
-
-	if (addr_size == sizeof(__u64)) {
-		memcpy(addrs, data->d_buf, count * addr_size);
-	} else {
-		for (i = 0; i < count; i++)
-			addrs[i] = (__u64) *((__u32 *) (data->d_buf + i * addr_size));
-	}
-
-	/*
-	 * We get Elf object from dwfl_module_getelf function,
-	 * which performs all possible relocations, including
-	 * __mcount_loc section.
-	 *
-	 * So addrs array now contains relocated values, which
-	 * we need take into account when we compare them to
-	 * functions values, see comment in setup_functions
-	 * function.
-	 */
-	*paddrs = addrs;
-	*pcount = count;
-	return 0;
-}
-
-static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
-{
-	__u64 start = func->addr;
-	__u64 addr, end = func->addr + func->size;
-
-	/*
-	 * The invariant here is addr[r] that is the smallest address
-	 * that is >= than function start addr. Except the corner case
-	 * where there is no such r, but for that we have a final check
-	 * in the return.
-	 */
-	size_t l = 0, r = count - 1, m;
-
-	/* make sure we don't use invalid r */
-	if (count == 0)
-		return false;
-
-	while (l < r) {
-		m = l + (r - l) / 2;
-		addr = addrs[m];
-
-		if (addr >= start) {
-			/* we satisfy invariant, so tighten r */
-			r = m;
-		} else {
-			/* m is not good enough as l, maybe m + 1 will be */
-			l = m + 1;
-		}
-	}
-
-	return start <= addrs[r] && addrs[r] < end;
-}
-
-static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
-{
-	__u64 *addrs, count, i;
-	int functions_valid = 0;
-	bool kmod = false;
-
-	/*
-	 * Check if we are processing vmlinux image and
-	 * get mcount data if it's detected.
-	 */
-	if (get_vmlinux_addrs(btfe, fl, &addrs, &count))
-		return -1;
-
-	/*
-	 * Check if we are processing kernel module and
-	 * get mcount data if it's detected.
-	 */
-	if (!addrs) {
-		if (get_kmod_addrs(btfe, &addrs, &count))
-			return -1;
-		kmod = true;
-	}
-
-	if (!addrs) {
-		if (btf_elf__verbose)
-			printf("ftrace symbols not detected, falling back to DWARF data\n");
-		delete_functions();
-		return 0;
-	}
-
-	qsort(addrs, count, sizeof(addrs[0]), addrs_cmp);
-	qsort(functions, functions_cnt, sizeof(functions[0]), functions_cmp);
-
-	/*
-	 * Let's got through all collected functions and filter
-	 * out those that are not in ftrace.
-	 */
-	for (i = 0; i < functions_cnt; i++) {
-		struct elf_function *func = &functions[i];
-		/*
-		 * For vmlinux image both addrs[x] and functions[x]::addr
-		 * values are final address and are comparable.
-		 *
-		 * For kernel module addrs[x] is final address, but
-		 * functions[x]::addr is relative address within section
-		 * and needs to be relocated by adding sh_addr.
-		 */
-		if (kmod)
-			func->addr += func->sh_addr;
-
-		/* Make sure function is within ftrace addresses. */
-		if (is_ftrace_func(func, addrs, count)) {
-			/*
-			 * We iterate over sorted array, so we can easily skip
-			 * not valid item and move following valid field into
-			 * its place, and still keep the 'new' array sorted.
-			 */
-			if (i != functions_valid)
-				functions[functions_valid] = functions[i];
-			functions_valid++;
-		}
-	}
-
-	functions_cnt = functions_valid;
-	free(addrs);
-
-	if (btf_elf__verbose)
-		printf("Found %d functions!\n", functions_cnt);
-	return 0;
-}
-
 static struct elf_function *find_function(const struct btf_elf *btfe,
 					  const char *name)
 {
@@ -620,23 +363,8 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
 	return 0;
 }
 
-static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
-			   size_t sym_sec_idx)
-{
-	if (!fl->mcount_start &&
-	    !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
-		fl->mcount_start = sym->st_value;
-		fl->mcount_sec_idx = sym_sec_idx;
-	}
-
-	if (!fl->mcount_stop &&
-	    !strcmp("__stop_mcount_loc", elf_sym__name(sym, btfe->symtab)))
-		fl->mcount_stop = sym->st_value;
-}
-
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 {
-	struct funcs_layout fl = { };
 	Elf32_Word sym_sec_idx;
 	uint32_t core_id;
 	GElf_Sym sym;
@@ -648,9 +376,8 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
 		if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx))
 			return -1;
-		if (collect_function(btfe, &sym, sym_sec_idx))
+		if (collect_function(btfe, &sym))
 			return -1;
-		collect_symbol(&sym, &fl, sym_sec_idx);
 	}
 
 	if (collect_percpu_vars) {
@@ -661,9 +388,11 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 			printf("Found %d per-CPU variables!\n", percpu_var_cnt);
 	}
 
-	if (functions_cnt && setup_functions(btfe, &fl)) {
-		fprintf(stderr, "Failed to filter DWARF functions\n");
-		return -1;
+	if (functions_cnt) {
+		qsort(functions, functions_cnt, sizeof(functions[0]),
+		      functions_cmp);
+		if (btf_elf__verbose)
+			printf("Found %d functions!\n", functions_cnt);
 	}
 
 	return 0;
-- 
2.30.2

Re: [PATCH v2 dwarves] btf: Remove ftrace filter

[Nathan Chancellor]

On Thu, May 06, 2021 at 01:56:22PM -0700, Martin KaFai Lau wrote:
> BTF is currently generated for functions that are in ftrace list
> or extern.
> 
> A recent use case also needs BTF generated for functions included in
> allowlist.  In particular, the kernel
> commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
> allows bpf program to directly call a few tcp cc kernel functions. Those
> kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
> is set to ensure they are in the ftrace list but this kconfig dependency
> is unnecessary.
> 
> Those kernel functions are specified under an ELF section .BTF_ids.
> There was an earlier attempt [0] to add another filter for the functions in
> the .BTF_ids section.  That discussion concluded that the ftrace filter
> should be removed instead.
> 
> This patch is to remove the ftrace filter and its related functions.
> 
> Number of BTF FUNC with and without is_ftrace_func():
> My kconfig in x86: 40643 vs 46225
> Jiri reported on arm: 25022 vs 55812
> 
> [0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/
> 
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

This fixes an issue with Fedora's s390x config that CKI noticed:

https://groups.google.com/g/clang-built-linux/c/IzthpckBJvc/m/MPWGDmXiAwAJ

Tested-by: Nathan Chancellor <nathan@kernel.org> # build

> ---
> v2: Remove sym_sec_idx, last_idx, and sh. (Jiri Olsa)
> 
>  btf_encoder.c | 285 ++------------------------------------------------
>  1 file changed, 7 insertions(+), 278 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 80e896961d4e..c711f124b31e 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -27,17 +27,8 @@
>   */
>  #define KSYM_NAME_LEN 128
>  
> -struct funcs_layout {
> -	unsigned long mcount_start;
> -	unsigned long mcount_stop;
> -	unsigned long mcount_sec_idx;
> -};
> -
>  struct elf_function {
>  	const char	*name;
> -	unsigned long	 addr;
> -	unsigned long	 size;
> -	unsigned long	 sh_addr;
>  	bool		 generated;
>  };
>  
> @@ -64,12 +55,9 @@ static void delete_functions(void)
>  #define max(x, y) ((x) < (y) ? (y) : (x))
>  #endif
>  
> -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> -			    size_t sym_sec_idx)
> +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>  {
>  	struct elf_function *new;
> -	static GElf_Shdr sh;
> -	static size_t last_idx;
>  	const char *name;
>  
>  	if (elf_sym__type(sym) != STT_FUNC)
> @@ -91,257 +79,12 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
>  		functions = new;
>  	}
>  
> -	if (sym_sec_idx != last_idx) {
> -		if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
> -			return 0;
> -		last_idx = sym_sec_idx;
> -	}
> -
>  	functions[functions_cnt].name = name;
> -	functions[functions_cnt].addr = elf_sym__value(sym);
> -	functions[functions_cnt].size = elf_sym__size(sym);
> -	functions[functions_cnt].sh_addr = sh.sh_addr;
>  	functions[functions_cnt].generated = false;
>  	functions_cnt++;
>  	return 0;
>  }
>  
> -static int addrs_cmp(const void *_a, const void *_b)
> -{
> -	const __u64 *a = _a;
> -	const __u64 *b = _b;
> -
> -	if (*a == *b)
> -		return 0;
> -	return *a < *b ? -1 : 1;
> -}
> -
> -static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
> -			     __u64 **paddrs, __u64 *pcount)
> -{
> -	__u64 *addrs, count, offset;
> -	unsigned int addr_size, i;
> -	Elf_Data *data;
> -	GElf_Shdr shdr;
> -	Elf_Scn *sec;
> -
> -	/* Initialize for the sake of all error paths below. */
> -	*paddrs = NULL;
> -	*pcount = 0;
> -
> -	if (!fl->mcount_start || !fl->mcount_stop)
> -		return 0;
> -
> -	/*
> -	 * Find mcount addressed marked by __start_mcount_loc
> -	 * and __stop_mcount_loc symbols and load them into
> -	 * sorted array.
> -	 */
> -	sec = elf_getscn(btfe->elf, fl->mcount_sec_idx);
> -	if (!sec || !gelf_getshdr(sec, &shdr)) {
> -		fprintf(stderr, "Failed to get section(%lu) header.\n",
> -			fl->mcount_sec_idx);
> -		return -1;
> -	}
> -
> -	/* Get address size from processed file's ELF class. */
> -	addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
> -
> -	offset = fl->mcount_start - shdr.sh_addr;
> -	count  = (fl->mcount_stop - fl->mcount_start) / addr_size;
> -
> -	data = elf_getdata(sec, 0);
> -	if (!data) {
> -		fprintf(stderr, "Failed to get section(%lu) data.\n",
> -			fl->mcount_sec_idx);
> -		return -1;
> -	}
> -
> -	addrs = malloc(count * sizeof(addrs[0]));
> -	if (!addrs) {
> -		fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> -		return -1;
> -	}
> -
> -	if (addr_size == sizeof(__u64)) {
> -		memcpy(addrs, data->d_buf + offset, count * addr_size);
> -	} else {
> -		for (i = 0; i < count; i++)
> -			addrs[i] = (__u64) *((__u32 *) (data->d_buf + offset + i * addr_size));
> -	}
> -
> -	*paddrs = addrs;
> -	*pcount = count;
> -	return 0;
> -}
> -
> -static int
> -get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> -{
> -	__u64 *addrs, count;
> -	unsigned int addr_size, i;
> -	GElf_Shdr shdr_mcount;
> -	Elf_Data *data;
> -	Elf_Scn *sec;
> -
> -	/* Initialize for the sake of all error paths below. */
> -	*paddrs = NULL;
> -	*pcount = 0;
> -
> -	/* get __mcount_loc */
> -	sec = elf_section_by_name(btfe->elf, &btfe->ehdr, &shdr_mcount,
> -				  "__mcount_loc", NULL);
> -	if (!sec) {
> -		if (btf_elf__verbose) {
> -			printf("%s: '%s' doesn't have __mcount_loc section\n", __func__,
> -			       btfe->filename);
> -		}
> -		return 0;
> -	}
> -
> -	data = elf_getdata(sec, NULL);
> -	if (!data) {
> -		fprintf(stderr, "Failed to data for __mcount_loc section.\n");
> -		return -1;
> -	}
> -
> -	/* Get address size from processed file's ELF class. */
> -	addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
> -
> -	count = data->d_size / addr_size;
> -
> -	addrs = malloc(count * sizeof(addrs[0]));
> -	if (!addrs) {
> -		fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> -		return -1;
> -	}
> -
> -	if (addr_size == sizeof(__u64)) {
> -		memcpy(addrs, data->d_buf, count * addr_size);
> -	} else {
> -		for (i = 0; i < count; i++)
> -			addrs[i] = (__u64) *((__u32 *) (data->d_buf + i * addr_size));
> -	}
> -
> -	/*
> -	 * We get Elf object from dwfl_module_getelf function,
> -	 * which performs all possible relocations, including
> -	 * __mcount_loc section.
> -	 *
> -	 * So addrs array now contains relocated values, which
> -	 * we need take into account when we compare them to
> -	 * functions values, see comment in setup_functions
> -	 * function.
> -	 */
> -	*paddrs = addrs;
> -	*pcount = count;
> -	return 0;
> -}
> -
> -static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
> -{
> -	__u64 start = func->addr;
> -	__u64 addr, end = func->addr + func->size;
> -
> -	/*
> -	 * The invariant here is addr[r] that is the smallest address
> -	 * that is >= than function start addr. Except the corner case
> -	 * where there is no such r, but for that we have a final check
> -	 * in the return.
> -	 */
> -	size_t l = 0, r = count - 1, m;
> -
> -	/* make sure we don't use invalid r */
> -	if (count == 0)
> -		return false;
> -
> -	while (l < r) {
> -		m = l + (r - l) / 2;
> -		addr = addrs[m];
> -
> -		if (addr >= start) {
> -			/* we satisfy invariant, so tighten r */
> -			r = m;
> -		} else {
> -			/* m is not good enough as l, maybe m + 1 will be */
> -			l = m + 1;
> -		}
> -	}
> -
> -	return start <= addrs[r] && addrs[r] < end;
> -}
> -
> -static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> -{
> -	__u64 *addrs, count, i;
> -	int functions_valid = 0;
> -	bool kmod = false;
> -
> -	/*
> -	 * Check if we are processing vmlinux image and
> -	 * get mcount data if it's detected.
> -	 */
> -	if (get_vmlinux_addrs(btfe, fl, &addrs, &count))
> -		return -1;
> -
> -	/*
> -	 * Check if we are processing kernel module and
> -	 * get mcount data if it's detected.
> -	 */
> -	if (!addrs) {
> -		if (get_kmod_addrs(btfe, &addrs, &count))
> -			return -1;
> -		kmod = true;
> -	}
> -
> -	if (!addrs) {
> -		if (btf_elf__verbose)
> -			printf("ftrace symbols not detected, falling back to DWARF data\n");
> -		delete_functions();
> -		return 0;
> -	}
> -
> -	qsort(addrs, count, sizeof(addrs[0]), addrs_cmp);
> -	qsort(functions, functions_cnt, sizeof(functions[0]), functions_cmp);
> -
> -	/*
> -	 * Let's got through all collected functions and filter
> -	 * out those that are not in ftrace.
> -	 */
> -	for (i = 0; i < functions_cnt; i++) {
> -		struct elf_function *func = &functions[i];
> -		/*
> -		 * For vmlinux image both addrs[x] and functions[x]::addr
> -		 * values are final address and are comparable.
> -		 *
> -		 * For kernel module addrs[x] is final address, but
> -		 * functions[x]::addr is relative address within section
> -		 * and needs to be relocated by adding sh_addr.
> -		 */
> -		if (kmod)
> -			func->addr += func->sh_addr;
> -
> -		/* Make sure function is within ftrace addresses. */
> -		if (is_ftrace_func(func, addrs, count)) {
> -			/*
> -			 * We iterate over sorted array, so we can easily skip
> -			 * not valid item and move following valid field into
> -			 * its place, and still keep the 'new' array sorted.
> -			 */
> -			if (i != functions_valid)
> -				functions[functions_valid] = functions[i];
> -			functions_valid++;
> -		}
> -	}
> -
> -	functions_cnt = functions_valid;
> -	free(addrs);
> -
> -	if (btf_elf__verbose)
> -		printf("Found %d functions!\n", functions_cnt);
> -	return 0;
> -}
> -
>  static struct elf_function *find_function(const struct btf_elf *btfe,
>  					  const char *name)
>  {
> @@ -620,23 +363,8 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
>  	return 0;
>  }
>  
> -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
> -			   size_t sym_sec_idx)
> -{
> -	if (!fl->mcount_start &&
> -	    !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
> -		fl->mcount_start = sym->st_value;
> -		fl->mcount_sec_idx = sym_sec_idx;
> -	}
> -
> -	if (!fl->mcount_stop &&
> -	    !strcmp("__stop_mcount_loc", elf_sym__name(sym, btfe->symtab)))
> -		fl->mcount_stop = sym->st_value;
> -}
> -
>  static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  {
> -	struct funcs_layout fl = { };
>  	Elf32_Word sym_sec_idx;
>  	uint32_t core_id;
>  	GElf_Sym sym;
> @@ -648,9 +376,8 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
>  		if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx))
>  			return -1;
> -		if (collect_function(btfe, &sym, sym_sec_idx))
> +		if (collect_function(btfe, &sym))
>  			return -1;
> -		collect_symbol(&sym, &fl, sym_sec_idx);
>  	}
>  
>  	if (collect_percpu_vars) {
> @@ -661,9 +388,11 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  			printf("Found %d per-CPU variables!\n", percpu_var_cnt);
>  	}
>  
> -	if (functions_cnt && setup_functions(btfe, &fl)) {
> -		fprintf(stderr, "Failed to filter DWARF functions\n");
> -		return -1;
> +	if (functions_cnt) {
> +		qsort(functions, functions_cnt, sizeof(functions[0]),
> +		      functions_cmp);
> +		if (btf_elf__verbose)
> +			printf("Found %d functions!\n", functions_cnt);
>  	}
>  
>  	return 0;
> -- 
> 2.30.2
> 

Re: [PATCH v2 dwarves] btf: Remove ftrace filter

[Jiri Slaby]

On 06. 05. 21, 22:56, Martin KaFai Lau wrote:
> BTF is currently generated for functions that are in ftrace list
> or extern.
> 
> A recent use case also needs BTF generated for functions included in
> allowlist.  In particular, the kernel
> commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
> allows bpf program to directly call a few tcp cc kernel functions. Those
> kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
> is set to ensure they are in the ftrace list but this kconfig dependency
> is unnecessary.
> 
> Those kernel functions are specified under an ELF section .BTF_ids.
> There was an earlier attempt [0] to add another filter for the functions in
> the .BTF_ids section.  That discussion concluded that the ftrace filter
> should be removed instead.
> 
> This patch is to remove the ftrace filter and its related functions.
> 
> Number of BTF FUNC with and without is_ftrace_func():
> My kconfig in x86: 40643 vs 46225
> Jiri reported on arm: 25022 vs 55812
> 
> [0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/
> 
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Today, I missed you submission :). So:

Acked-by: Jiri Slaby <jirislaby@kernel.org>

> ---
> v2: Remove sym_sec_idx, last_idx, and sh. (Jiri Olsa)
> 
>   btf_encoder.c | 285 ++------------------------------------------------
>   1 file changed, 7 insertions(+), 278 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 80e896961d4e..c711f124b31e 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -27,17 +27,8 @@
>    */
>   #define KSYM_NAME_LEN 128
>   
> -struct funcs_layout {
> -	unsigned long mcount_start;
> -	unsigned long mcount_stop;
> -	unsigned long mcount_sec_idx;
> -};
> -
>   struct elf_function {
>   	const char	*name;
> -	unsigned long	 addr;
> -	unsigned long	 size;
> -	unsigned long	 sh_addr;
>   	bool		 generated;
>   };
>   
> @@ -64,12 +55,9 @@ static void delete_functions(void)
>   #define max(x, y) ((x) < (y) ? (y) : (x))
>   #endif
>   
> -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> -			    size_t sym_sec_idx)
> +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>   {
>   	struct elf_function *new;
> -	static GElf_Shdr sh;
> -	static size_t last_idx;
>   	const char *name;
>   
>   	if (elf_sym__type(sym) != STT_FUNC)
> @@ -91,257 +79,12 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
>   		functions = new;
>   	}
>   
> -	if (sym_sec_idx != last_idx) {
> -		if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
> -			return 0;
> -		last_idx = sym_sec_idx;
> -	}
> -
>   	functions[functions_cnt].name = name;
> -	functions[functions_cnt].addr = elf_sym__value(sym);
> -	functions[functions_cnt].size = elf_sym__size(sym);
> -	functions[functions_cnt].sh_addr = sh.sh_addr;
>   	functions[functions_cnt].generated = false;
>   	functions_cnt++;
>   	return 0;
>   }
>   
> -static int addrs_cmp(const void *_a, const void *_b)
> -{
> -	const __u64 *a = _a;
> -	const __u64 *b = _b;
> -
> -	if (*a == *b)
> -		return 0;
> -	return *a < *b ? -1 : 1;
> -}
> -
> -static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
> -			     __u64 **paddrs, __u64 *pcount)
> -{
> -	__u64 *addrs, count, offset;
> -	unsigned int addr_size, i;
> -	Elf_Data *data;
> -	GElf_Shdr shdr;
> -	Elf_Scn *sec;
> -
> -	/* Initialize for the sake of all error paths below. */
> -	*paddrs = NULL;
> -	*pcount = 0;
> -
> -	if (!fl->mcount_start || !fl->mcount_stop)
> -		return 0;
> -
> -	/*
> -	 * Find mcount addressed marked by __start_mcount_loc
> -	 * and __stop_mcount_loc symbols and load them into
> -	 * sorted array.
> -	 */
> -	sec = elf_getscn(btfe->elf, fl->mcount_sec_idx);
> -	if (!sec || !gelf_getshdr(sec, &shdr)) {
> -		fprintf(stderr, "Failed to get section(%lu) header.\n",
> -			fl->mcount_sec_idx);
> -		return -1;
> -	}
> -
> -	/* Get address size from processed file's ELF class. */
> -	addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
> -
> -	offset = fl->mcount_start - shdr.sh_addr;
> -	count  = (fl->mcount_stop - fl->mcount_start) / addr_size;
> -
> -	data = elf_getdata(sec, 0);
> -	if (!data) {
> -		fprintf(stderr, "Failed to get section(%lu) data.\n",
> -			fl->mcount_sec_idx);
> -		return -1;
> -	}
> -
> -	addrs = malloc(count * sizeof(addrs[0]));
> -	if (!addrs) {
> -		fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> -		return -1;
> -	}
> -
> -	if (addr_size == sizeof(__u64)) {
> -		memcpy(addrs, data->d_buf + offset, count * addr_size);
> -	} else {
> -		for (i = 0; i < count; i++)
> -			addrs[i] = (__u64) *((__u32 *) (data->d_buf + offset + i * addr_size));
> -	}
> -
> -	*paddrs = addrs;
> -	*pcount = count;
> -	return 0;
> -}
> -
> -static int
> -get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> -{
> -	__u64 *addrs, count;
> -	unsigned int addr_size, i;
> -	GElf_Shdr shdr_mcount;
> -	Elf_Data *data;
> -	Elf_Scn *sec;
> -
> -	/* Initialize for the sake of all error paths below. */
> -	*paddrs = NULL;
> -	*pcount = 0;
> -
> -	/* get __mcount_loc */
> -	sec = elf_section_by_name(btfe->elf, &btfe->ehdr, &shdr_mcount,
> -				  "__mcount_loc", NULL);
> -	if (!sec) {
> -		if (btf_elf__verbose) {
> -			printf("%s: '%s' doesn't have __mcount_loc section\n", __func__,
> -			       btfe->filename);
> -		}
> -		return 0;
> -	}
> -
> -	data = elf_getdata(sec, NULL);
> -	if (!data) {
> -		fprintf(stderr, "Failed to data for __mcount_loc section.\n");
> -		return -1;
> -	}
> -
> -	/* Get address size from processed file's ELF class. */
> -	addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
> -
> -	count = data->d_size / addr_size;
> -
> -	addrs = malloc(count * sizeof(addrs[0]));
> -	if (!addrs) {
> -		fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> -		return -1;
> -	}
> -
> -	if (addr_size == sizeof(__u64)) {
> -		memcpy(addrs, data->d_buf, count * addr_size);
> -	} else {
> -		for (i = 0; i < count; i++)
> -			addrs[i] = (__u64) *((__u32 *) (data->d_buf + i * addr_size));
> -	}
> -
> -	/*
> -	 * We get Elf object from dwfl_module_getelf function,
> -	 * which performs all possible relocations, including
> -	 * __mcount_loc section.
> -	 *
> -	 * So addrs array now contains relocated values, which
> -	 * we need take into account when we compare them to
> -	 * functions values, see comment in setup_functions
> -	 * function.
> -	 */
> -	*paddrs = addrs;
> -	*pcount = count;
> -	return 0;
> -}
> -
> -static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
> -{
> -	__u64 start = func->addr;
> -	__u64 addr, end = func->addr + func->size;
> -
> -	/*
> -	 * The invariant here is addr[r] that is the smallest address
> -	 * that is >= than function start addr. Except the corner case
> -	 * where there is no such r, but for that we have a final check
> -	 * in the return.
> -	 */
> -	size_t l = 0, r = count - 1, m;
> -
> -	/* make sure we don't use invalid r */
> -	if (count == 0)
> -		return false;
> -
> -	while (l < r) {
> -		m = l + (r - l) / 2;
> -		addr = addrs[m];
> -
> -		if (addr >= start) {
> -			/* we satisfy invariant, so tighten r */
> -			r = m;
> -		} else {
> -			/* m is not good enough as l, maybe m + 1 will be */
> -			l = m + 1;
> -		}
> -	}
> -
> -	return start <= addrs[r] && addrs[r] < end;
> -}
> -
> -static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> -{
> -	__u64 *addrs, count, i;
> -	int functions_valid = 0;
> -	bool kmod = false;
> -
> -	/*
> -	 * Check if we are processing vmlinux image and
> -	 * get mcount data if it's detected.
> -	 */
> -	if (get_vmlinux_addrs(btfe, fl, &addrs, &count))
> -		return -1;
> -
> -	/*
> -	 * Check if we are processing kernel module and
> -	 * get mcount data if it's detected.
> -	 */
> -	if (!addrs) {
> -		if (get_kmod_addrs(btfe, &addrs, &count))
> -			return -1;
> -		kmod = true;
> -	}
> -
> -	if (!addrs) {
> -		if (btf_elf__verbose)
> -			printf("ftrace symbols not detected, falling back to DWARF data\n");
> -		delete_functions();
> -		return 0;
> -	}
> -
> -	qsort(addrs, count, sizeof(addrs[0]), addrs_cmp);
> -	qsort(functions, functions_cnt, sizeof(functions[0]), functions_cmp);
> -
> -	/*
> -	 * Let's got through all collected functions and filter
> -	 * out those that are not in ftrace.
> -	 */
> -	for (i = 0; i < functions_cnt; i++) {
> -		struct elf_function *func = &functions[i];
> -		/*
> -		 * For vmlinux image both addrs[x] and functions[x]::addr
> -		 * values are final address and are comparable.
> -		 *
> -		 * For kernel module addrs[x] is final address, but
> -		 * functions[x]::addr is relative address within section
> -		 * and needs to be relocated by adding sh_addr.
> -		 */
> -		if (kmod)
> -			func->addr += func->sh_addr;
> -
> -		/* Make sure function is within ftrace addresses. */
> -		if (is_ftrace_func(func, addrs, count)) {
> -			/*
> -			 * We iterate over sorted array, so we can easily skip
> -			 * not valid item and move following valid field into
> -			 * its place, and still keep the 'new' array sorted.
> -			 */
> -			if (i != functions_valid)
> -				functions[functions_valid] = functions[i];
> -			functions_valid++;
> -		}
> -	}
> -
> -	functions_cnt = functions_valid;
> -	free(addrs);
> -
> -	if (btf_elf__verbose)
> -		printf("Found %d functions!\n", functions_cnt);
> -	return 0;
> -}
> -
>   static struct elf_function *find_function(const struct btf_elf *btfe,
>   					  const char *name)
>   {
> @@ -620,23 +363,8 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
>   	return 0;
>   }
>   
> -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
> -			   size_t sym_sec_idx)
> -{
> -	if (!fl->mcount_start &&
> -	    !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
> -		fl->mcount_start = sym->st_value;
> -		fl->mcount_sec_idx = sym_sec_idx;
> -	}
> -
> -	if (!fl->mcount_stop &&
> -	    !strcmp("__stop_mcount_loc", elf_sym__name(sym, btfe->symtab)))
> -		fl->mcount_stop = sym->st_value;
> -}
> -
>   static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>   {
> -	struct funcs_layout fl = { };
>   	Elf32_Word sym_sec_idx;
>   	uint32_t core_id;
>   	GElf_Sym sym;
> @@ -648,9 +376,8 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>   	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
>   		if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx))
>   			return -1;
> -		if (collect_function(btfe, &sym, sym_sec_idx))
> +		if (collect_function(btfe, &sym))
>   			return -1;
> -		collect_symbol(&sym, &fl, sym_sec_idx);
>   	}
>   
>   	if (collect_percpu_vars) {
> @@ -661,9 +388,11 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>   			printf("Found %d per-CPU variables!\n", percpu_var_cnt);
>   	}
>   
> -	if (functions_cnt && setup_functions(btfe, &fl)) {
> -		fprintf(stderr, "Failed to filter DWARF functions\n");
> -		return -1;
> +	if (functions_cnt) {
> +		qsort(functions, functions_cnt, sizeof(functions[0]),
> +		      functions_cmp);
> +		if (btf_elf__verbose)
> +			printf("Found %d functions!\n", functions_cnt);
>   	}
>   
>   	return 0;
> 


-- 
js

Re: [PATCH v2 dwarves] btf: Remove ftrace filter

[Jiri Olsa]

On Thu, May 06, 2021 at 01:56:22PM -0700, Martin KaFai Lau wrote:
> BTF is currently generated for functions that are in ftrace list
> or extern.
> 
> A recent use case also needs BTF generated for functions included in
> allowlist.  In particular, the kernel
> commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
> allows bpf program to directly call a few tcp cc kernel functions. Those
> kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
> is set to ensure they are in the ftrace list but this kconfig dependency
> is unnecessary.
> 
> Those kernel functions are specified under an ELF section .BTF_ids.
> There was an earlier attempt [0] to add another filter for the functions in
> the .BTF_ids section.  That discussion concluded that the ftrace filter
> should be removed instead.
> 
> This patch is to remove the ftrace filter and its related functions.
> 
> Number of BTF FUNC with and without is_ftrace_func():
> My kconfig in x86: 40643 vs 46225
> Jiri reported on arm: 25022 vs 55812
> 
> [0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/
> 
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> v2: Remove sym_sec_idx, last_idx, and sh. (Jiri Olsa)

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

Re: [PATCH v2 dwarves] btf: Remove ftrace filter

[Martin KaFai Lau]

On Thu, May 06, 2021 at 07:54:37PM -0700, Nathan Chancellor wrote:
> On Thu, May 06, 2021 at 01:56:22PM -0700, Martin KaFai Lau wrote:
> > BTF is currently generated for functions that are in ftrace list
> > or extern.
> > 
> > A recent use case also needs BTF generated for functions included in
> > allowlist.  In particular, the kernel
> > commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
> > allows bpf program to directly call a few tcp cc kernel functions. Those
> > kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
> > is set to ensure they are in the ftrace list but this kconfig dependency
> > is unnecessary.
> > 
> > Those kernel functions are specified under an ELF section .BTF_ids.
> > There was an earlier attempt [0] to add another filter for the functions in
> > the .BTF_ids section.  That discussion concluded that the ftrace filter
> > should be removed instead.
> > 
> > This patch is to remove the ftrace filter and its related functions.
> > 
> > Number of BTF FUNC with and without is_ftrace_func():
> > My kconfig in x86: 40643 vs 46225
> > Jiri reported on arm: 25022 vs 55812
> > 
> > [0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/
> > 
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> 
> This fixes an issue with Fedora's s390x config that CKI noticed:
> 
> https://groups.google.com/g/clang-built-linux/c/IzthpckBJvc/m/MPWGDmXiAwAJ 
> 
> Tested-by: Nathan Chancellor <nathan@kernel.org> # build
Thanks all for reviewing and testing.

In my cross compile ppc64 test, it does not solve the issue.
The problem is the tcp-cc functions (e.g. "cublictcp_*")
are not STT_FUNC in ppc64, so they are not collected in collect_function().
The ".cubictcp_*" is STT_FUNC though.

Since only the x86 (64 and 32) bpf jit can call these tcp-cc functions now
and there is no usage for adding them to .BTF_ids for other ARCHs,
I have post a patch to limit them to x86:
https://lore.kernel.org/bpf/20210508005011.3863757-1-kafai@fb.com/

Can you try the above kernel patch without pahole change?
(i.e. use pahole 1.21 as-is).  With the above kernel patch
and pahole 1.21, I have cross compiled arm64, ppc64, s390, and sparc64.

[ p.s. vfs_truncate should work as is in ppc64 since functions_cnt
       should be 0 in ppc64 and then it will rely on fn->external. ]

Re: [PATCH v2 dwarves] btf: Remove ftrace filter

[Michal Suchánek]

On Fri, May 07, 2021 at 10:03:21PM -0700, Martin KaFai Lau wrote:
> On Thu, May 06, 2021 at 07:54:37PM -0700, Nathan Chancellor wrote:
> > On Thu, May 06, 2021 at 01:56:22PM -0700, Martin KaFai Lau wrote:
> > > BTF is currently generated for functions that are in ftrace list
> > > or extern.
> > > 
> > > A recent use case also needs BTF generated for functions included in
> > > allowlist.  In particular, the kernel
> > > commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
> > > allows bpf program to directly call a few tcp cc kernel functions. Those
> > > kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
> > > is set to ensure they are in the ftrace list but this kconfig dependency
> > > is unnecessary.
> > > 
> > > Those kernel functions are specified under an ELF section .BTF_ids.
> > > There was an earlier attempt [0] to add another filter for the functions in
> > > the .BTF_ids section.  That discussion concluded that the ftrace filter
> > > should be removed instead.
> > > 
> > > This patch is to remove the ftrace filter and its related functions.
> > > 
> > > Number of BTF FUNC with and without is_ftrace_func():
> > > My kconfig in x86: 40643 vs 46225
> > > Jiri reported on arm: 25022 vs 55812
> > > 
> > > [0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/
> > > 
> > > Cc: Andrii Nakryiko <andrii@kernel.org>
> > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > 
> > This fixes an issue with Fedora's s390x config that CKI noticed:
> > 
> > https://groups.google.com/g/clang-built-linux/c/IzthpckBJvc/m/MPWGDmXiAwAJ 
> > 
> > Tested-by: Nathan Chancellor <nathan@kernel.org> # build
> Thanks all for reviewing and testing.
> 
> In my cross compile ppc64 test, it does not solve the issue.
> The problem is the tcp-cc functions (e.g. "cublictcp_*")
> are not STT_FUNC in ppc64, so they are not collected in collect_function().
> The ".cubictcp_*" is STT_FUNC though.
> 
> Since only the x86 (64 and 32) bpf jit can call these tcp-cc functions now
> and there is no usage for adding them to .BTF_ids for other ARCHs,
> I have post a patch to limit them to x86:
> https://lore.kernel.org/bpf/20210508005011.3863757-1-kafai@fb.com/

That's probably not the way to go. If function symbols start with a dot
in ppc64 elfv1 abi then pahole should learn to add a dot for ppc64 elfv1
abi.

Or we can build ppc64 BE using the elfv2 abi and depend on elfv2 abi for
BTF on ppc64. The patch for enabling elfv2 for BE is currently under
discussion and I have the patch that adds the dependency ready as well.

Thanks

Michal

Re: [PATCH v2 dwarves] btf: Remove ftrace filter

[Arnaldo Carvalho de Melo]

Em Fri, May 07, 2021 at 12:39:52PM +0200, Jiri Olsa escreveu:
> On Thu, May 06, 2021 at 01:56:22PM -0700, Martin KaFai Lau wrote:
> > BTF is currently generated for functions that are in ftrace list
> > or extern.
> > 
> > A recent use case also needs BTF generated for functions included in
> > allowlist.  In particular, the kernel
> > commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
> > allows bpf program to directly call a few tcp cc kernel functions. Those
> > kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
> > is set to ensure they are in the ftrace list but this kconfig dependency
> > is unnecessary.
> > 
> > Those kernel functions are specified under an ELF section .BTF_ids.
> > There was an earlier attempt [0] to add another filter for the functions in
> > the .BTF_ids section.  That discussion concluded that the ftrace filter
> > should be removed instead.
> > 
> > This patch is to remove the ftrace filter and its related functions.
> > 
> > Number of BTF FUNC with and without is_ftrace_func():
> > My kconfig in x86: 40643 vs 46225
> > Jiri reported on arm: 25022 vs 55812
> > 
> > [0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/
> > 
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
> > v2: Remove sym_sec_idx, last_idx, and sh. (Jiri Olsa)
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Thanks, applied.

- Arnaldo

Re: [PATCH v2 dwarves] btf: Remove ftrace filter

[Martin KaFai Lau]

On Sat, May 08, 2021 at 12:53:01PM +0200, Michal Suchánek wrote:
> On Fri, May 07, 2021 at 10:03:21PM -0700, Martin KaFai Lau wrote:
> > On Thu, May 06, 2021 at 07:54:37PM -0700, Nathan Chancellor wrote:
> > > On Thu, May 06, 2021 at 01:56:22PM -0700, Martin KaFai Lau wrote:
> > > > BTF is currently generated for functions that are in ftrace list
> > > > or extern.
> > > > 
> > > > A recent use case also needs BTF generated for functions included in
> > > > allowlist.  In particular, the kernel
> > > > commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
> > > > allows bpf program to directly call a few tcp cc kernel functions. Those
> > > > kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
> > > > is set to ensure they are in the ftrace list but this kconfig dependency
> > > > is unnecessary.
> > > > 
> > > > Those kernel functions are specified under an ELF section .BTF_ids.
> > > > There was an earlier attempt [0] to add another filter for the functions in
> > > > the .BTF_ids section.  That discussion concluded that the ftrace filter
> > > > should be removed instead.
> > > > 
> > > > This patch is to remove the ftrace filter and its related functions.
> > > > 
> > > > Number of BTF FUNC with and without is_ftrace_func():
> > > > My kconfig in x86: 40643 vs 46225
> > > > Jiri reported on arm: 25022 vs 55812
> > > > 
> > > > [0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/
> > > > 
> > > > Cc: Andrii Nakryiko <andrii@kernel.org>
> > > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > 
> > > This fixes an issue with Fedora's s390x config that CKI noticed:
> > > 
> > > https://groups.google.com/g/clang-built-linux/c/IzthpckBJvc/m/MPWGDmXiAwAJ  
> > > 
> > > Tested-by: Nathan Chancellor <nathan@kernel.org> # build
> > Thanks all for reviewing and testing.
> > 
> > In my cross compile ppc64 test, it does not solve the issue.
> > The problem is the tcp-cc functions (e.g. "cublictcp_*")
> > are not STT_FUNC in ppc64, so they are not collected in collect_function().
> > The ".cubictcp_*" is STT_FUNC though.
> > 
> > Since only the x86 (64 and 32) bpf jit can call these tcp-cc functions now
> > and there is no usage for adding them to .BTF_ids for other ARCHs,
> > I have post a patch to limit them to x86:
> > https://lore.kernel.org/bpf/20210508005011.3863757-1-kafai@fb.com/
> 
> That's probably not the way to go. If function symbols start with a dot
> in ppc64 elfv1 abi then pahole should learn to add a dot for ppc64 elfv1
> abi.
> 
> Or we can build ppc64 BE using the elfv2 abi and depend on elfv2 abi for
> BTF on ppc64. The patch for enabling elfv2 for BE is currently under
> discussion and I have the patch that adds the dependency ready as well.
ic.  I just learned the elfv2/LE vs elfv1/BE difference in ppc64.
elfv2 BE ppc64 support is on its way, so it is better to have DEBUG_INFO_BTF
depending on the ppc64-elfv2 related kconfig(s) instead of making an exception
handling in pahole.

If you have the	BTF ppc64-elfv2 dependency patch ready, does it make
sense to land this patch first independent of the ppc64 elfv2/BE
effort?

Could you share a pointer to the ppc64-elfv2 BE	patch?

Re: [PATCH v2 dwarves] btf: Remove ftrace filter

[Michal Suchánek]

On Mon, May 10, 2021 at 11:00:59AM -0700, Martin KaFai Lau wrote:
> On Sat, May 08, 2021 at 12:53:01PM +0200, Michal Suchánek wrote:
> > On Fri, May 07, 2021 at 10:03:21PM -0700, Martin KaFai Lau wrote:
> > > On Thu, May 06, 2021 at 07:54:37PM -0700, Nathan Chancellor wrote:
> > > > On Thu, May 06, 2021 at 01:56:22PM -0700, Martin KaFai Lau wrote:
> > > > > BTF is currently generated for functions that are in ftrace list
> > > > > or extern.
> > > > > 
> > > > > A recent use case also needs BTF generated for functions included in
> > > > > allowlist.  In particular, the kernel
> > > > > commit e78aea8b2170 ("bpf: tcp: Put some tcp cong functions in allowlist for bpf-tcp-cc")
> > > > > allows bpf program to directly call a few tcp cc kernel functions. Those
> > > > > kernel functions are currently allowed only if CONFIG_DYNAMIC_FTRACE
> > > > > is set to ensure they are in the ftrace list but this kconfig dependency
> > > > > is unnecessary.
> > > > > 
> > > > > Those kernel functions are specified under an ELF section .BTF_ids.
> > > > > There was an earlier attempt [0] to add another filter for the functions in
> > > > > the .BTF_ids section.  That discussion concluded that the ftrace filter
> > > > > should be removed instead.
> > > > > 
> > > > > This patch is to remove the ftrace filter and its related functions.
> > > > > 
> > > > > Number of BTF FUNC with and without is_ftrace_func():
> > > > > My kconfig in x86: 40643 vs 46225
> > > > > Jiri reported on arm: 25022 vs 55812
> > > > > 
> > > > > [0]: https://lore.kernel.org/dwarves/20210423213728.3538141-1-kafai@fb.com/
> > > > > 
> > > > > Cc: Andrii Nakryiko <andrii@kernel.org>
> > > > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > > 
> > > > This fixes an issue with Fedora's s390x config that CKI noticed:
> > > > 
> > > > https://groups.google.com/g/clang-built-linux/c/IzthpckBJvc/m/MPWGDmXiAwAJ  
> > > > 
> > > > Tested-by: Nathan Chancellor <nathan@kernel.org> # build
> > > Thanks all for reviewing and testing.
> > > 
> > > In my cross compile ppc64 test, it does not solve the issue.
> > > The problem is the tcp-cc functions (e.g. "cublictcp_*")
> > > are not STT_FUNC in ppc64, so they are not collected in collect_function().
> > > The ".cubictcp_*" is STT_FUNC though.
> > > 
> > > Since only the x86 (64 and 32) bpf jit can call these tcp-cc functions now
> > > and there is no usage for adding them to .BTF_ids for other ARCHs,
> > > I have post a patch to limit them to x86:
> > > https://lore.kernel.org/bpf/20210508005011.3863757-1-kafai@fb.com/
> > 
> > That's probably not the way to go. If function symbols start with a dot
> > in ppc64 elfv1 abi then pahole should learn to add a dot for ppc64 elfv1
> > abi.
> > 
> > Or we can build ppc64 BE using the elfv2 abi and depend on elfv2 abi for
> > BTF on ppc64. The patch for enabling elfv2 for BE is currently under
> > discussion and I have the patch that adds the dependency ready as well.
> ic.  I just learned the elfv2/LE vs elfv1/BE difference in ppc64.
> elfv2 BE ppc64 support is on its way, so it is better to have DEBUG_INFO_BTF
> depending on the ppc64-elfv2 related kconfig(s) instead of making an exception
> handling in pahole.
> 
> If you have the	BTF ppc64-elfv2 dependency patch ready, does it make
> sense to land this patch first independent of the ppc64 elfv2/BE
> effort?

It kind of depends on the result of the config option name bikeshedding
so it has to be merged after.

Thanks

Michal