[PATCHv2 0/2] btf_encoder: Fix functions BTF data generation

[PATCHv2 0/2] btf_encoder: Fix functions BTF data generation

[Jiri Olsa]

hi,
recent btf encoder's changes brakes BTF data for some gcc
versions. The problem is that some functions can appear
in dwarf data in some instances without arguments, while
they are defined with some.

v2 changes:
  - drop patch 3 logic and just change conditions
    based on Andrii's suggestion
  - drop patch 2
  - add ack for patch 1

Andrii,
please check if it's working with your gcc.

thanks,
jirka


---
Jiri Olsa (2):
      btf_encoder: Generate also .init functions
      btf_encoder: Fix function generation

 btf_encoder.c | 67 ++++++++++++-------------------------------------------------------
 1 file changed, 12 insertions(+), 55 deletions(-)

[PATCH 1/2] btf_encoder: Generate also .init functions

[Jiri Olsa]

Currently we skip functions under .init* sections, Removing the .init*
section check, BTF now contains also functions from .init* sections.

Andrii's explanation from email:

> ...                  I think we should just drop the __init check and
> include all the __init functions into BTF. There could be cases where
> we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
> security cases), so having BTFs for those FUNCs are necessary as well.
> Ftrace currently disallows that, but it's only because no user-space
> application has a way to attach probes early enough. This might change
> in the future, so there is no need to invent special mechanisms now
> for bpf_iter function preservation. Let's just include all __init
> functions in BTF.

It's over ~2000 functions on my .config:

   $ bpftool btf dump file ./vmlinux | grep 'FUNC ' | wc -l
   41505
   $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep 'FUNC ' | wc -l
   39256

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 43 ++-----------------------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 9b93e9963727..d531651b1e9e 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -29,10 +29,6 @@
 struct funcs_layout {
 	unsigned long mcount_start;
 	unsigned long mcount_stop;
-	unsigned long init_begin;
-	unsigned long init_end;
-	unsigned long init_bpf_begin;
-	unsigned long init_bpf_end;
 	unsigned long mcount_sec_idx;
 };
 
@@ -104,16 +100,6 @@ static int addrs_cmp(const void *_a, const void *_b)
 	return *a < *b ? -1 : 1;
 }
 
-static bool is_init(struct funcs_layout *fl, unsigned long addr)
-{
-	return addr >= fl->init_begin && addr < fl->init_end;
-}
-
-static bool is_bpf_init(struct funcs_layout *fl, unsigned long addr)
-{
-	return addr >= fl->init_bpf_begin && addr < fl->init_bpf_end;
-}
-
 static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 {
 	unsigned long *addrs, count, offset, i;
@@ -155,18 +141,11 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 
 	/*
 	 * Let's got through all collected functions and filter
-	 * out those that are not in ftrace and init code.
+	 * out those that are not in ftrace.
 	 */
 	for (i = 0; i < functions_cnt; i++) {
 		struct elf_function *func = &functions[i];
 
-		/*
-		 * Do not enable .init section functions,
-		 * but keep .init.bpf.preserve_type functions.
-		 */
-		if (is_init(fl, func->addr) && !is_bpf_init(fl, func->addr))
-			continue;
-
 		/* Make sure function is within ftrace addresses. */
 		if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
 			/*
@@ -493,29 +472,11 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
 	if (!fl->mcount_stop &&
 	    !strcmp("__stop_mcount_loc", elf_sym__name(sym, btfe->symtab)))
 		fl->mcount_stop = sym->st_value;
-
-	if (!fl->init_begin &&
-	    !strcmp("__init_begin", elf_sym__name(sym, btfe->symtab)))
-		fl->init_begin = sym->st_value;
-
-	if (!fl->init_end &&
-	    !strcmp("__init_end", elf_sym__name(sym, btfe->symtab)))
-		fl->init_end = sym->st_value;
-
-	if (!fl->init_bpf_begin &&
-	    !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
-		fl->init_bpf_begin = sym->st_value;
-
-	if (!fl->init_bpf_end &&
-	    !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
-		fl->init_bpf_end = sym->st_value;
 }
 
 static int has_all_symbols(struct funcs_layout *fl)
 {
-	return fl->mcount_start && fl->mcount_stop &&
-	       fl->init_begin && fl->init_end &&
-	       fl->init_bpf_begin && fl->init_bpf_end;
+	return fl->mcount_start && fl->mcount_stop;
 }
 
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
-- 
2.26.2

[PATCH 2/2] btf_encoder: Fix function generation

[Jiri Olsa]

Current conditions for picking up function records break
BTF data on some gcc versions.

Some function records can appear with no arguments but with
declaration tag set, so moving the 'fn->declaration' in front
of other checks.

Then checking if argument names are present and finally checking
ftrace filter if it's present. If ftrace filter is not available,
using the external tag to filter out non external functions.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index d531651b1e9e..de471bc754b1 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -612,25 +612,21 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		const char *name;
 
 		/*
-		 * The functions_cnt != 0 means we parsed all necessary
-		 * kernel symbols and we are using ftrace location filter
-		 * for functions. If it's not available keep the current
-		 * dwarf declaration check.
+		 * Skip functions that:
+		 *   - are marked as declarations
+		 *   - do not have full argument names
+		 *   - are not in ftrace list (if it's available)
+		 *   - are not external (in case ftrace filter is not available)
 		 */
+		if (fn->declaration)
+			continue;
+		if (!has_arg_names(cu, &fn->proto))
+			continue;
 		if (functions_cnt) {
-			/*
-			 * We check following conditions:
-			 *   - argument names are defined
-			 *   - there's symbol and address defined for the function
-			 *   - function address belongs to ftrace locations
-			 *   - function is generated only once
-			 */
-			if (!has_arg_names(cu, &fn->proto))
-				continue;
 			if (!should_generate_function(btfe, function__name(fn, cu)))
 				continue;
 		} else {
-			if (fn->declaration || !fn->external)
+			if (!fn->external)
 				continue;
 		}
 
-- 
2.26.2

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Arnaldo Carvalho de Melo]

Em Fri, Nov 13, 2020 at 04:12:22PM +0100, Jiri Olsa escreveu:
> Current conditions for picking up function records break
> BTF data on some gcc versions.
> 
> Some function records can appear with no arguments but with
> declaration tag set, so moving the 'fn->declaration' in front
> of other checks.
> 
> Then checking if argument names are present and finally checking
> ftrace filter if it's present. If ftrace filter is not available,
> using the external tag to filter out non external functions.

Humm has_arg_names() will return true for a:

void foo(void)

function, which I think is right, but can't this function appear
multiple times in different CUs and we end up with the same function
multiple times in the BTF info?

- Arnaldo
 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  btf_encoder.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index d531651b1e9e..de471bc754b1 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -612,25 +612,21 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  		const char *name;
>  
>  		/*
> -		 * The functions_cnt != 0 means we parsed all necessary
> -		 * kernel symbols and we are using ftrace location filter
> -		 * for functions. If it's not available keep the current
> -		 * dwarf declaration check.
> +		 * Skip functions that:
> +		 *   - are marked as declarations
> +		 *   - do not have full argument names
> +		 *   - are not in ftrace list (if it's available)
> +		 *   - are not external (in case ftrace filter is not available)
>  		 */
> +		if (fn->declaration)
> +			continue;
> +		if (!has_arg_names(cu, &fn->proto))
> +			continue;
>  		if (functions_cnt) {
> -			/*
> -			 * We check following conditions:
> -			 *   - argument names are defined
> -			 *   - there's symbol and address defined for the function
> -			 *   - function address belongs to ftrace locations
> -			 *   - function is generated only once
> -			 */
> -			if (!has_arg_names(cu, &fn->proto))
> -				continue;
>  			if (!should_generate_function(btfe, function__name(fn, cu)))
>  				continue;
>  		} else {
> -			if (fn->declaration || !fn->external)
> +			if (!fn->external)
>  				continue;
>  		}
>  
> -- 
> 2.26.2
> 

-- 

- Arnaldo

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Jiri Olsa]

On Fri, Nov 13, 2020 at 02:28:32PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 13, 2020 at 04:12:22PM +0100, Jiri Olsa escreveu:
> > Current conditions for picking up function records break
> > BTF data on some gcc versions.
> > 
> > Some function records can appear with no arguments but with
> > declaration tag set, so moving the 'fn->declaration' in front
> > of other checks.
> > 
> > Then checking if argument names are present and finally checking
> > ftrace filter if it's present. If ftrace filter is not available,
> > using the external tag to filter out non external functions.
> 
> Humm has_arg_names() will return true for a:
> 
> void foo(void)
> 
> function, which I think is right, but can't this function appear
> multiple times in different CUs and we end up with the same function
> multiple times in the BTF info?

so declarations should be filtered by the fn->declaration
check, if gcc has it ;-)

and if it goes through, the should_generate_function sets/checks
flag if the function was already generated

jirka

> 
> - Arnaldo
>  
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  btf_encoder.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> > 
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index d531651b1e9e..de471bc754b1 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -612,25 +612,21 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >  		const char *name;
> >  
> >  		/*
> > -		 * The functions_cnt != 0 means we parsed all necessary
> > -		 * kernel symbols and we are using ftrace location filter
> > -		 * for functions. If it's not available keep the current
> > -		 * dwarf declaration check.
> > +		 * Skip functions that:
> > +		 *   - are marked as declarations
> > +		 *   - do not have full argument names
> > +		 *   - are not in ftrace list (if it's available)
> > +		 *   - are not external (in case ftrace filter is not available)
> >  		 */
> > +		if (fn->declaration)
> > +			continue;
> > +		if (!has_arg_names(cu, &fn->proto))
> > +			continue;
> >  		if (functions_cnt) {
> > -			/*
> > -			 * We check following conditions:
> > -			 *   - argument names are defined
> > -			 *   - there's symbol and address defined for the function
> > -			 *   - function address belongs to ftrace locations
> > -			 *   - function is generated only once
> > -			 */
> > -			if (!has_arg_names(cu, &fn->proto))
> > -				continue;
> >  			if (!should_generate_function(btfe, function__name(fn, cu)))
> >  				continue;
> >  		} else {
> > -			if (fn->declaration || !fn->external)
> > +			if (!fn->external)
> >  				continue;
> >  		}
> >  
> > -- 
> > 2.26.2
> > 
> 
> -- 
> 
> - Arnaldo
> 

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Andrii Nakryiko]

On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Current conditions for picking up function records break
> BTF data on some gcc versions.
>
> Some function records can appear with no arguments but with
> declaration tag set, so moving the 'fn->declaration' in front
> of other checks.
>
> Then checking if argument names are present and finally checking
> ftrace filter if it's present. If ftrace filter is not available,
> using the external tag to filter out non external functions.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

I tested locally, all seems to work fine. Left few suggestions below,
but those could be done in follow ups (or argued to not be done).

Acked-by: Andrii Nakryiko <andrii@kernel.org>

BTW, for some stats.

BEFORE allowing static funcs:

.BTF ELF section
=======================================
Data size:      4101624
Header size:    24
Types size:     2472836
Strings size:   1628764

BTF types
=======================================
Total        2472836 bytes (83310 types)
Struct:       920436 bytes (10305 types)
FuncProto:    638668 bytes (18869 types)
Func:         308304 bytes (25692 types)
Enum:         184308 bytes (2293 types)
Ptr:          173484 bytes (14457 types)
Array:         89064 bytes (3711 types)
Union:         81552 bytes (1961 types)
Const:         34368 bytes (2864 types)
Typedef:       32124 bytes (2677 types)
Var:            4688 bytes (293 types)
Datasec:        3528 bytes (1 types)
Fwd:            1656 bytes (138 types)
Volatile:        360 bytes (30 types)
Int:             272 bytes (17 types)
Restrict:         24 bytes (2 types)


AFTER allowing static funcs:

.BTF ELF section
=======================================
Data size:      4930558
Header size:    24
Types size:     2914016
Strings size:   2016518

BTF types
=======================================
Total        2914016 bytes (108282 types)
Struct:       920436 bytes (10305 types)
FuncProto:    851528 bytes (24814 types)
Func:         536664 bytes (44722 types)
Enum:         184308 bytes (2293 types)
Ptr:          173484 bytes (14457 types)
Array:         89064 bytes (3711 types)
Union:         81552 bytes (1961 types)
Const:         34368 bytes (2864 types)
Typedef:       32124 bytes (2677 types)
Var:            4688 bytes (293 types)
Datasec:        3528 bytes (1 types)
Fwd:            1656 bytes (138 types)
Volatile:        360 bytes (30 types)
Int:             256 bytes (16 types)

So 25692 vs 44722 functions, but the increase in func_proto is smaller
due to dedup. Good chunk is strings data for all those function and
parameter names.


>  btf_encoder.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index d531651b1e9e..de471bc754b1 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -612,25 +612,21 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>                 const char *name;
>
>                 /*
> -                * The functions_cnt != 0 means we parsed all necessary
> -                * kernel symbols and we are using ftrace location filter
> -                * for functions. If it's not available keep the current
> -                * dwarf declaration check.
> +                * Skip functions that:
> +                *   - are marked as declarations
> +                *   - do not have full argument names
> +                *   - are not in ftrace list (if it's available)
> +                *   - are not external (in case ftrace filter is not available)
>                  */
> +               if (fn->declaration)
> +                       continue;
> +               if (!has_arg_names(cu, &fn->proto))
> +                       continue;
>                 if (functions_cnt) {
> -                       /*
> -                        * We check following conditions:
> -                        *   - argument names are defined
> -                        *   - there's symbol and address defined for the function
> -                        *   - function address belongs to ftrace locations
> -                        *   - function is generated only once
> -                        */
> -                       if (!has_arg_names(cu, &fn->proto))
> -                               continue;
>                         if (!should_generate_function(btfe, function__name(fn, cu)))

Seeing Arnaldo's confusion, I remember initially I was similarly
confused. I think this p->generated = true should be moved out of
should_generate_function() and done here explicitly. Let's turn
should_generate_function() into find_allowed_function() or something,
to encapsulate bsearch. Checking !p || p->generated could be done here
explicitly.

>                                 continue;
>                 } else {
> -                       if (fn->declaration || !fn->external)
> +                       if (!fn->external)

Hm.. why didn't you drop this fallback? For non-vmlinux, do you think
it's a problem to generate all FUNCs? Mostly theoretical question,
though.

>                                 continue;
>                 }
>
> --
> 2.26.2
>

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Jiri Olsa]

On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Current conditions for picking up function records break
> > BTF data on some gcc versions.
> >
> > Some function records can appear with no arguments but with
> > declaration tag set, so moving the 'fn->declaration' in front
> > of other checks.
> >
> > Then checking if argument names are present and finally checking
> > ftrace filter if it's present. If ftrace filter is not available,
> > using the external tag to filter out non external functions.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> I tested locally, all seems to work fine. Left few suggestions below,
> but those could be done in follow ups (or argued to not be done).
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> BTW, for some stats.
> 
> BEFORE allowing static funcs:
> 
> .BTF ELF section
> =======================================
> Data size:      4101624
> Header size:    24
> Types size:     2472836
> Strings size:   1628764
> 
> BTF types
> =======================================
> Total        2472836 bytes (83310 types)
> Struct:       920436 bytes (10305 types)
> FuncProto:    638668 bytes (18869 types)
> Func:         308304 bytes (25692 types)
> Enum:         184308 bytes (2293 types)
> Ptr:          173484 bytes (14457 types)
> Array:         89064 bytes (3711 types)
> Union:         81552 bytes (1961 types)
> Const:         34368 bytes (2864 types)
> Typedef:       32124 bytes (2677 types)
> Var:            4688 bytes (293 types)
> Datasec:        3528 bytes (1 types)
> Fwd:            1656 bytes (138 types)
> Volatile:        360 bytes (30 types)
> Int:             272 bytes (17 types)
> Restrict:         24 bytes (2 types)
> 
> 
> AFTER allowing static funcs:
> 
> .BTF ELF section
> =======================================
> Data size:      4930558
> Header size:    24
> Types size:     2914016
> Strings size:   2016518
> 
> BTF types
> =======================================
> Total        2914016 bytes (108282 types)
> Struct:       920436 bytes (10305 types)
> FuncProto:    851528 bytes (24814 types)
> Func:         536664 bytes (44722 types)
> Enum:         184308 bytes (2293 types)
> Ptr:          173484 bytes (14457 types)
> Array:         89064 bytes (3711 types)
> Union:         81552 bytes (1961 types)
> Const:         34368 bytes (2864 types)
> Typedef:       32124 bytes (2677 types)
> Var:            4688 bytes (293 types)
> Datasec:        3528 bytes (1 types)
> Fwd:            1656 bytes (138 types)
> Volatile:        360 bytes (30 types)
> Int:             256 bytes (16 types)

nice, is this tool somewhere in the tree?

> 
> So 25692 vs 44722 functions, but the increase in func_proto is smaller
> due to dedup. Good chunk is strings data for all those function and
> parameter names.
> 
> 
> >  btf_encoder.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index d531651b1e9e..de471bc754b1 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -612,25 +612,21 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >                 const char *name;
> >
> >                 /*
> > -                * The functions_cnt != 0 means we parsed all necessary
> > -                * kernel symbols and we are using ftrace location filter
> > -                * for functions. If it's not available keep the current
> > -                * dwarf declaration check.
> > +                * Skip functions that:
> > +                *   - are marked as declarations
> > +                *   - do not have full argument names
> > +                *   - are not in ftrace list (if it's available)
> > +                *   - are not external (in case ftrace filter is not available)
> >                  */
> > +               if (fn->declaration)
> > +                       continue;
> > +               if (!has_arg_names(cu, &fn->proto))
> > +                       continue;
> >                 if (functions_cnt) {
> > -                       /*
> > -                        * We check following conditions:
> > -                        *   - argument names are defined
> > -                        *   - there's symbol and address defined for the function
> > -                        *   - function address belongs to ftrace locations
> > -                        *   - function is generated only once
> > -                        */
> > -                       if (!has_arg_names(cu, &fn->proto))
> > -                               continue;
> >                         if (!should_generate_function(btfe, function__name(fn, cu)))
> 
> Seeing Arnaldo's confusion, I remember initially I was similarly
> confused. I think this p->generated = true should be moved out of
> should_generate_function() and done here explicitly. Let's turn
> should_generate_function() into find_allowed_function() or something,
> to encapsulate bsearch. Checking !p || p->generated could be done here
> explicitly.

ok, that should be more obvious, I'll send new version

> 
> >                                 continue;
> >                 } else {
> > -                       if (fn->declaration || !fn->external)
> > +                       if (!fn->external)
> 
> Hm.. why didn't you drop this fallback? For non-vmlinux, do you think
> it's a problem to generate all FUNCs? Mostly theoretical question,
> though.

because it would probably allowed all static functions,
(ftrace data has only static functions that are traceable)
and who knows what a can of worms we'd open here ;-)

jirka

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Andrii Nakryiko]

On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> > On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Current conditions for picking up function records break
> > > BTF data on some gcc versions.
> > >
> > > Some function records can appear with no arguments but with
> > > declaration tag set, so moving the 'fn->declaration' in front
> > > of other checks.
> > >
> > > Then checking if argument names are present and finally checking
> > > ftrace filter if it's present. If ftrace filter is not available,
> > > using the external tag to filter out non external functions.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> >
> > I tested locally, all seems to work fine. Left few suggestions below,
> > but those could be done in follow ups (or argued to not be done).
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >
> > BTW, for some stats.
> >
> > BEFORE allowing static funcs:
> >
> > .BTF ELF section
> > =======================================
> > Data size:      4101624
> > Header size:    24
> > Types size:     2472836
> > Strings size:   1628764
> >
> > BTF types
> > =======================================
> > Total        2472836 bytes (83310 types)
> > Struct:       920436 bytes (10305 types)
> > FuncProto:    638668 bytes (18869 types)
> > Func:         308304 bytes (25692 types)
> > Enum:         184308 bytes (2293 types)
> > Ptr:          173484 bytes (14457 types)
> > Array:         89064 bytes (3711 types)
> > Union:         81552 bytes (1961 types)
> > Const:         34368 bytes (2864 types)
> > Typedef:       32124 bytes (2677 types)
> > Var:            4688 bytes (293 types)
> > Datasec:        3528 bytes (1 types)
> > Fwd:            1656 bytes (138 types)
> > Volatile:        360 bytes (30 types)
> > Int:             272 bytes (17 types)
> > Restrict:         24 bytes (2 types)
> >
> >
> > AFTER allowing static funcs:
> >
> > .BTF ELF section
> > =======================================
> > Data size:      4930558
> > Header size:    24
> > Types size:     2914016
> > Strings size:   2016518
> >
> > BTF types
> > =======================================
> > Total        2914016 bytes (108282 types)
> > Struct:       920436 bytes (10305 types)
> > FuncProto:    851528 bytes (24814 types)
> > Func:         536664 bytes (44722 types)
> > Enum:         184308 bytes (2293 types)
> > Ptr:          173484 bytes (14457 types)
> > Array:         89064 bytes (3711 types)
> > Union:         81552 bytes (1961 types)
> > Const:         34368 bytes (2864 types)
> > Typedef:       32124 bytes (2677 types)
> > Var:            4688 bytes (293 types)
> > Datasec:        3528 bytes (1 types)
> > Fwd:            1656 bytes (138 types)
> > Volatile:        360 bytes (30 types)
> > Int:             256 bytes (16 types)
>
> nice, is this tool somewhere in the tree?

Nope, this is from my personal BTF inspection tool, which I never got
to open-sourcing, too low on the priority list...

>
> >
> > So 25692 vs 44722 functions, but the increase in func_proto is smaller
> > due to dedup. Good chunk is strings data for all those function and
> > parameter names.
> >
> >
> > >  btf_encoder.c | 24 ++++++++++--------------
> > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > index d531651b1e9e..de471bc754b1 100644
> > > --- a/btf_encoder.c
> > > +++ b/btf_encoder.c
> > > @@ -612,25 +612,21 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > >                 const char *name;
> > >
> > >                 /*
> > > -                * The functions_cnt != 0 means we parsed all necessary
> > > -                * kernel symbols and we are using ftrace location filter
> > > -                * for functions. If it's not available keep the current
> > > -                * dwarf declaration check.
> > > +                * Skip functions that:
> > > +                *   - are marked as declarations
> > > +                *   - do not have full argument names
> > > +                *   - are not in ftrace list (if it's available)
> > > +                *   - are not external (in case ftrace filter is not available)
> > >                  */
> > > +               if (fn->declaration)
> > > +                       continue;
> > > +               if (!has_arg_names(cu, &fn->proto))
> > > +                       continue;
> > >                 if (functions_cnt) {
> > > -                       /*
> > > -                        * We check following conditions:
> > > -                        *   - argument names are defined
> > > -                        *   - there's symbol and address defined for the function
> > > -                        *   - function address belongs to ftrace locations
> > > -                        *   - function is generated only once
> > > -                        */
> > > -                       if (!has_arg_names(cu, &fn->proto))
> > > -                               continue;
> > >                         if (!should_generate_function(btfe, function__name(fn, cu)))
> >
> > Seeing Arnaldo's confusion, I remember initially I was similarly
> > confused. I think this p->generated = true should be moved out of
> > should_generate_function() and done here explicitly. Let's turn
> > should_generate_function() into find_allowed_function() or something,
> > to encapsulate bsearch. Checking !p || p->generated could be done here
> > explicitly.
>
> ok, that should be more obvious, I'll send new version
>
> >
> > >                                 continue;
> > >                 } else {
> > > -                       if (fn->declaration || !fn->external)
> > > +                       if (!fn->external)
> >
> > Hm.. why didn't you drop this fallback? For non-vmlinux, do you think
> > it's a problem to generate all FUNCs? Mostly theoretical question,
> > though.
>
> because it would probably allowed all static functions,
> (ftrace data has only static functions that are traceable)
> and who knows what a can of worms we'd open here ;-)
>

Fair enough.

> jirka
>

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Arnaldo Carvalho de Melo]

Em Fri, Nov 13, 2020 at 01:43:47PM -0800, Andrii Nakryiko escreveu:
> On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@redhat.com> wrote:

> > On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> > > On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:

> > > > Current conditions for picking up function records break
> > > > BTF data on some gcc versions.

> > > > Some function records can appear with no arguments but with
> > > > declaration tag set, so moving the 'fn->declaration' in front
> > > > of other checks.

> > > > Then checking if argument names are present and finally checking
> > > > ftrace filter if it's present. If ftrace filter is not available,
> > > > using the external tag to filter out non external functions.

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

> > > I tested locally, all seems to work fine. Left few suggestions below,
> > > but those could be done in follow ups (or argued to not be done).

> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>

> > > BTW, for some stats.

> > > BEFORE allowing static funcs:


Nowhere in the last patchkit comments is some explanation for the
inclusion of static functions :-\ After the first patch in the last
series I get:

  $ llvm-objcopy --remove-section=.BTF vmlinux
  $ readelf -SW vmlinux  | grep BTF
  $ pahole -J vmlinux
  $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > before.bpftool
  $ cp vmlinux vmlinux.before.all
  $ wc -l before.bpftool
  28829 before.bpftool
  $
  $ llvm-objcopy --remove-section=.BTF vmlinux
  $ readelf -SW vmlinux  | grep BTF
  $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > after-1st-patch.bpftool
  Error: failed to load BTF from ./vmlinux: No such file or directory
  $ pahole -J vmlinux
  $
  $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > after-1st-patch.bpftool
  $ wc -l after-1st-patch.bpftool
  41030 after-1st-patch.bpftool
  $ diff -u before.bpftool after-1st-patch.bpftool | less
  $ diff -u before.bpftool after-1st-patch.bpftool | less
  $

BTF: I built this kernel without CONFIG_DEBUG_INFO_BTF=y, so that I could use: llvm-objcopy --remove-section=.BTF

That matches what you see here, i.e.

BEFORE:

> > > Func:         308304 bytes (25692 types)

AFTER:

> > > Func:         536664 bytes (44722 types)

The number of functions, that is. I'm scratching my head to figure out
why:

https://lore.kernel.org/bpf/20201114223853.1010900-2-jolsa@kernel.org/

[PATCH 1/2] btf_encoder: Generate also .init functions

Causes this, can you guys please explain it so that we have this in the
change log?

As the diff shows that the increase in the number of functions is due to
static functions being added:

[acme@five pahole]$ diff -u before.bpftool after-1st-patch.bpftool | tail
+__zswap_pool_release
+zswap_writeback_entry
+zswap_zpool_param_set
+zs_zpool_create
+zs_zpool_destroy
+zs_zpool_free
+zs_zpool_malloc
+zs_zpool_map
+zs_zpool_total_size
+zs_zpool_unmap
[acme@five pahole]$

static void zs_zpool_unmap(void *pool, unsigned long handle)
{
        zs_unmap_object(pool, handle);
}

Reading below Jiri says:

> > > >                                 continue;
> > > >                 } else {
> > > > -                       if (fn->declaration || !fn->external)
> > > > +                       if (!fn->external)
> > >
> > > Hm.. why didn't you drop this fallback? For non-vmlinux, do you think
> > > it's a problem to generate all FUNCs? Mostly theoretical question,
> > > though.
> >
> > because it would probably allowed all static functions,
> > (ftrace data has only static functions that are traceable)
> > and who knows what a can of worms we'd open here ;-)
> >
> 
> Fair enough.

But I'm getting these results (the addition of static variables) after
applying:

btf_encoder: Generate also .init functions

:-\

- Arnaldo

> > > .BTF ELF section
> > > =======================================
> > > Data size:      4101624
> > > Header size:    24
> > > Types size:     2472836
> > > Strings size:   1628764
> > >
> > > BTF types
> > > =======================================
> > > Total        2472836 bytes (83310 types)
> > > Struct:       920436 bytes (10305 types)
> > > FuncProto:    638668 bytes (18869 types)
> > > Func:         308304 bytes (25692 types)
> > > Enum:         184308 bytes (2293 types)
> > > Ptr:          173484 bytes (14457 types)
> > > Array:         89064 bytes (3711 types)
> > > Union:         81552 bytes (1961 types)
> > > Const:         34368 bytes (2864 types)
> > > Typedef:       32124 bytes (2677 types)
> > > Var:            4688 bytes (293 types)
> > > Datasec:        3528 bytes (1 types)
> > > Fwd:            1656 bytes (138 types)
> > > Volatile:        360 bytes (30 types)
> > > Int:             272 bytes (17 types)
> > > Restrict:         24 bytes (2 types)
> > >
> > >
> > > AFTER allowing static funcs:
> > >
> > > .BTF ELF section
> > > =======================================
> > > Data size:      4930558
> > > Header size:    24
> > > Types size:     2914016
> > > Strings size:   2016518
> > >
> > > BTF types
> > > =======================================
> > > Total        2914016 bytes (108282 types)
> > > Struct:       920436 bytes (10305 types)
> > > FuncProto:    851528 bytes (24814 types)
> > > Func:         536664 bytes (44722 types)
> > > Enum:         184308 bytes (2293 types)
> > > Ptr:          173484 bytes (14457 types)
> > > Array:         89064 bytes (3711 types)
> > > Union:         81552 bytes (1961 types)
> > > Const:         34368 bytes (2864 types)
> > > Typedef:       32124 bytes (2677 types)
> > > Var:            4688 bytes (293 types)
> > > Datasec:        3528 bytes (1 types)
> > > Fwd:            1656 bytes (138 types)
> > > Volatile:        360 bytes (30 types)
> > > Int:             256 bytes (16 types)
> >
> > nice, is this tool somewhere in the tree?
> 
> Nope, this is from my personal BTF inspection tool, which I never got
> to open-sourcing, too low on the priority list...
> 
> >
> > >
> > > So 25692 vs 44722 functions, but the increase in func_proto is smaller
> > > due to dedup. Good chunk is strings data for all those function and
> > > parameter names.
> > >
> > >
> > > >  btf_encoder.c | 24 ++++++++++--------------
> > > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > index d531651b1e9e..de471bc754b1 100644
> > > > --- a/btf_encoder.c
> > > > +++ b/btf_encoder.c
> > > > @@ -612,25 +612,21 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > >                 const char *name;
> > > >
> > > >                 /*
> > > > -                * The functions_cnt != 0 means we parsed all necessary
> > > > -                * kernel symbols and we are using ftrace location filter
> > > > -                * for functions. If it's not available keep the current
> > > > -                * dwarf declaration check.
> > > > +                * Skip functions that:
> > > > +                *   - are marked as declarations
> > > > +                *   - do not have full argument names
> > > > +                *   - are not in ftrace list (if it's available)
> > > > +                *   - are not external (in case ftrace filter is not available)
> > > >                  */
> > > > +               if (fn->declaration)
> > > > +                       continue;
> > > > +               if (!has_arg_names(cu, &fn->proto))
> > > > +                       continue;
> > > >                 if (functions_cnt) {
> > > > -                       /*
> > > > -                        * We check following conditions:
> > > > -                        *   - argument names are defined
> > > > -                        *   - there's symbol and address defined for the function
> > > > -                        *   - function address belongs to ftrace locations
> > > > -                        *   - function is generated only once
> > > > -                        */
> > > > -                       if (!has_arg_names(cu, &fn->proto))
> > > > -                               continue;
> > > >                         if (!should_generate_function(btfe, function__name(fn, cu)))
> > >
> > > Seeing Arnaldo's confusion, I remember initially I was similarly
> > > confused. I think this p->generated = true should be moved out of
> > > should_generate_function() and done here explicitly. Let's turn
> > > should_generate_function() into find_allowed_function() or something,
> > > to encapsulate bsearch. Checking !p || p->generated could be done here
> > > explicitly.
> >
> > ok, that should be more obvious, I'll send new version
> >
> > >
> > > >                                 continue;
> > > >                 } else {
> > > > -                       if (fn->declaration || !fn->external)
> > > > +                       if (!fn->external)
> > >
> > > Hm.. why didn't you drop this fallback? For non-vmlinux, do you think
> > > it's a problem to generate all FUNCs? Mostly theoretical question,
> > > though.
> >
> > because it would probably allowed all static functions,
> > (ftrace data has only static functions that are traceable)
> > and who knows what a can of worms we'd open here ;-)
> >
> 
> Fair enough.
> 
> > jirka
> >

-- 

- Arnaldo

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Jiri Olsa]

On Mon, Nov 16, 2020 at 10:50:16AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 13, 2020 at 01:43:47PM -0800, Andrii Nakryiko escreveu:
> > On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
> 
> > > On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> > > > On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> 
> > > > > Current conditions for picking up function records break
> > > > > BTF data on some gcc versions.
> 
> > > > > Some function records can appear with no arguments but with
> > > > > declaration tag set, so moving the 'fn->declaration' in front
> > > > > of other checks.
> 
> > > > > Then checking if argument names are present and finally checking
> > > > > ftrace filter if it's present. If ftrace filter is not available,
> > > > > using the external tag to filter out non external functions.
> 
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> > > > I tested locally, all seems to work fine. Left few suggestions below,
> > > > but those could be done in follow ups (or argued to not be done).
> 
> > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> > > > BTW, for some stats.
> 
> > > > BEFORE allowing static funcs:
> 
> 
> Nowhere in the last patchkit comments is some explanation for the
> inclusion of static functions :-\ After the first patch in the last
> series I get:
> 
>   $ llvm-objcopy --remove-section=.BTF vmlinux
>   $ readelf -SW vmlinux  | grep BTF
>   $ pahole -J vmlinux
>   $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > before.bpftool
>   $ cp vmlinux vmlinux.before.all
>   $ wc -l before.bpftool
>   28829 before.bpftool

I think you see the original number of functions, because without
the 'not merged' kernel patch, that added the special init section,
pahole will fail to detect vmlinux and fall back to checking dwarf
declarations

there's a verbose message for the fall back, but it is not displayed
at the moment ;-) with the fix below you should see it:

  $ LLVM_OBJCOPY=objcopy ./pahole -V -J vmlinux >out
  $ cat out | grep 'vmlinux not detected'
  vmlinux not detected, falling back to dwarf data

I'll check on the verbose setup and send full patch,
I did not expect it would not get printed, sry

so the new numebr ~41k functions is together static functions
and init functions

jirka


---
diff --git a/btf_encoder.c b/btf_encoder.c
index 9b93e9963727..7efd26de5815 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -584,6 +584,8 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 	struct tag *pos;
 	int err = 0;
 
+	btf_elf__verbose = verbose;
+
 	if (btfe && strcmp(btfe->filename, cu->filename)) {
 		err = btf_encoder__encode();
 		if (err)
@@ -623,7 +625,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		}
 	}
 
-	btf_elf__verbose = verbose;
 	btf_elf__force = force;
 	type_id_off = btf__get_nr_types(btfe->btf);
 
diff --git a/lib/bpf b/lib/bpf
--- a/lib/bpf
+++ b/lib/bpf
@@ -1 +1 @@
-Subproject commit ff797cc905d9c5fe9acab92d2da127342b20f80f
+Subproject commit ff797cc905d9c5fe9acab92d2da127342b20f80f-dirty

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Arnaldo Carvalho de Melo]

Em Mon, Nov 16, 2020 at 07:21:45PM +0100, Jiri Olsa escreveu:
> On Mon, Nov 16, 2020 at 10:50:16AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Nov 13, 2020 at 01:43:47PM -0800, Andrii Nakryiko escreveu:
> > > On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > 
> > > > On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> > > > > On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > 
> > > > > > Current conditions for picking up function records break
> > > > > > BTF data on some gcc versions.
> > 
> > > > > > Some function records can appear with no arguments but with
> > > > > > declaration tag set, so moving the 'fn->declaration' in front
> > > > > > of other checks.
> > 
> > > > > > Then checking if argument names are present and finally checking
> > > > > > ftrace filter if it's present. If ftrace filter is not available,
> > > > > > using the external tag to filter out non external functions.
> > 
> > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > 
> > > > > I tested locally, all seems to work fine. Left few suggestions below,
> > > > > but those could be done in follow ups (or argued to not be done).
> > 
> > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > 
> > > > > BTW, for some stats.
> > 
> > > > > BEFORE allowing static funcs:
> > 
> > 
> > Nowhere in the last patchkit comments is some explanation for the
> > inclusion of static functions :-\ After the first patch in the last
> > series I get:
> > 
> >   $ llvm-objcopy --remove-section=.BTF vmlinux
> >   $ readelf -SW vmlinux  | grep BTF
> >   $ pahole -J vmlinux
> >   $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > before.bpftool
> >   $ cp vmlinux vmlinux.before.all
> >   $ wc -l before.bpftool
> >   28829 before.bpftool
> 
> I think you see the original number of functions, because without
> the 'not merged' kernel patch, that added the special init section,
> pahole will fail to detect vmlinux and fall back to checking dwarf
> declarations

Indeed, I moved the verbose/force setting to the beggining of the
encoder and:

------------
Found 352 per-CPU variables!
vmlinux not detected, falling back to dwarf data
File vmlinux:
search cu '/home/acme/git/linux/arch/x86/kernel/head_64.S' for percpu global variables.
-----------------

Now I have to read that code to figure out what that "vmlinux not
detected, falling back to dwarf data" message means, as vmlinux is where
DWARF data is, so what is that isn't being "detected", /me checks...

- Arnaldo
 
> there's a verbose message for the fall back, but it is not displayed
> at the moment ;-) with the fix below you should see it:
> 
>   $ LLVM_OBJCOPY=objcopy ./pahole -V -J vmlinux >out
>   $ cat out | grep 'vmlinux not detected'
>   vmlinux not detected, falling back to dwarf data
> 
> I'll check on the verbose setup and send full patch,
> I did not expect it would not get printed, sry
> 
> so the new numebr ~41k functions is together static functions
> and init functions
> 
> jirka
> 
> 
> ---
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 9b93e9963727..7efd26de5815 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -584,6 +584,8 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  	struct tag *pos;
>  	int err = 0;
>  
> +	btf_elf__verbose = verbose;
> +
>  	if (btfe && strcmp(btfe->filename, cu->filename)) {
>  		err = btf_encoder__encode();
>  		if (err)
> @@ -623,7 +625,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  		}
>  	}
>  
> -	btf_elf__verbose = verbose;
>  	btf_elf__force = force;
>  	type_id_off = btf__get_nr_types(btfe->btf);
>  
> diff --git a/lib/bpf b/lib/bpf
> --- a/lib/bpf
> +++ b/lib/bpf
> @@ -1 +1 @@
> -Subproject commit ff797cc905d9c5fe9acab92d2da127342b20f80f
> +Subproject commit ff797cc905d9c5fe9acab92d2da127342b20f80f-dirty
> 

-- 

- Arnaldo

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Arnaldo Carvalho de Melo]

Em Mon, Nov 16, 2020 at 04:15:44PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Nov 16, 2020 at 07:21:45PM +0100, Jiri Olsa escreveu:
> > On Mon, Nov 16, 2020 at 10:50:16AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Nov 13, 2020 at 01:43:47PM -0800, Andrii Nakryiko escreveu:
> > > > On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > > 
> > > > > On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> > > > > > On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > 
> > > > > > > Current conditions for picking up function records break
> > > > > > > BTF data on some gcc versions.
> > > 
> > > > > > > Some function records can appear with no arguments but with
> > > > > > > declaration tag set, so moving the 'fn->declaration' in front
> > > > > > > of other checks.
> > > 
> > > > > > > Then checking if argument names are present and finally checking
> > > > > > > ftrace filter if it's present. If ftrace filter is not available,
> > > > > > > using the external tag to filter out non external functions.
> > > 
> > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > 
> > > > > > I tested locally, all seems to work fine. Left few suggestions below,
> > > > > > but those could be done in follow ups (or argued to not be done).
> > > 
> > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > 
> > > > > > BTW, for some stats.
> > > 
> > > > > > BEFORE allowing static funcs:
> > > 
> > > 
> > > Nowhere in the last patchkit comments is some explanation for the
> > > inclusion of static functions :-\ After the first patch in the last
> > > series I get:
> > > 
> > >   $ llvm-objcopy --remove-section=.BTF vmlinux
> > >   $ readelf -SW vmlinux  | grep BTF
> > >   $ pahole -J vmlinux
> > >   $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > before.bpftool
> > >   $ cp vmlinux vmlinux.before.all
> > >   $ wc -l before.bpftool
> > >   28829 before.bpftool
> > 
> > I think you see the original number of functions, because without
> > the 'not merged' kernel patch, that added the special init section,
> > pahole will fail to detect vmlinux and fall back to checking dwarf
> > declarations
> 
> Indeed, I moved the verbose/force setting to the beggining of the
> encoder and:
> 
> ------------
> Found 352 per-CPU variables!
> vmlinux not detected, falling back to dwarf data
> File vmlinux:
> search cu '/home/acme/git/linux/arch/x86/kernel/head_64.S' for percpu global variables.
> -----------------
> 
> Now I have to read that code to figure out what that "vmlinux not
> detected, falling back to dwarf data" message means, as vmlinux is where
> DWARF data is, so what is that isn't being "detected", /me checks...

So with some debugging I see, the message is just confusing:

"vmlinux not detected, falling back to dwarf data (functions_cnt=53238, has_all_symbols(&fl)=0"

It finds the ELF symtab, finds the percpu variables there, tons of
functions, matching the number after this approach of marking BPF init
functions was dropped its just that vague "has_all_symbols()" routine
that fails to find all the symbols it needs in the vmlinux file.

- Arnaldo

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Jiri Olsa]

On Mon, Nov 16, 2020 at 04:22:21PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Nov 16, 2020 at 04:15:44PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Nov 16, 2020 at 07:21:45PM +0100, Jiri Olsa escreveu:
> > > On Mon, Nov 16, 2020 at 10:50:16AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Fri, Nov 13, 2020 at 01:43:47PM -0800, Andrii Nakryiko escreveu:
> > > > > On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > 
> > > > > > On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> > > > > > > On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > 
> > > > > > > > Current conditions for picking up function records break
> > > > > > > > BTF data on some gcc versions.
> > > > 
> > > > > > > > Some function records can appear with no arguments but with
> > > > > > > > declaration tag set, so moving the 'fn->declaration' in front
> > > > > > > > of other checks.
> > > > 
> > > > > > > > Then checking if argument names are present and finally checking
> > > > > > > > ftrace filter if it's present. If ftrace filter is not available,
> > > > > > > > using the external tag to filter out non external functions.
> > > > 
> > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > 
> > > > > > > I tested locally, all seems to work fine. Left few suggestions below,
> > > > > > > but those could be done in follow ups (or argued to not be done).
> > > > 
> > > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > 
> > > > > > > BTW, for some stats.
> > > > 
> > > > > > > BEFORE allowing static funcs:
> > > > 
> > > > 
> > > > Nowhere in the last patchkit comments is some explanation for the
> > > > inclusion of static functions :-\ After the first patch in the last
> > > > series I get:
> > > > 
> > > >   $ llvm-objcopy --remove-section=.BTF vmlinux
> > > >   $ readelf -SW vmlinux  | grep BTF
> > > >   $ pahole -J vmlinux
> > > >   $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > before.bpftool
> > > >   $ cp vmlinux vmlinux.before.all
> > > >   $ wc -l before.bpftool
> > > >   28829 before.bpftool
> > > 
> > > I think you see the original number of functions, because without
> > > the 'not merged' kernel patch, that added the special init section,
> > > pahole will fail to detect vmlinux and fall back to checking dwarf
> > > declarations
> > 
> > Indeed, I moved the verbose/force setting to the beggining of the
> > encoder and:
> > 
> > ------------
> > Found 352 per-CPU variables!
> > vmlinux not detected, falling back to dwarf data
> > File vmlinux:
> > search cu '/home/acme/git/linux/arch/x86/kernel/head_64.S' for percpu global variables.
> > -----------------
> > 
> > Now I have to read that code to figure out what that "vmlinux not
> > detected, falling back to dwarf data" message means, as vmlinux is where
> > DWARF data is, so what is that isn't being "detected", /me checks...
> 
> So with some debugging I see, the message is just confusing:
> 
> "vmlinux not detected, falling back to dwarf data (functions_cnt=53238, has_all_symbols(&fl)=0"

how about:

"ftrace data not detected, falling back to dwarf data"

> 
> It finds the ELF symtab, finds the percpu variables there, tons of
> functions, matching the number after this approach of marking BPF init
> functions was dropped its just that vague "has_all_symbols()" routine
> that fails to find all the symbols it needs in the vmlinux file.

we collect functions and other symbols in one loop over the symtab,
so thats why we have all those collected and still can decide to fall back

before we needed also the init section symbols, now with this patch
we need to know only mcount section begin/end

jirka

> 
> - Arnaldo
> 

Re: [PATCH 2/2] btf_encoder: Fix function generation

[Arnaldo Carvalho de Melo]

Em Mon, Nov 16, 2020 at 08:40:08PM +0100, Jiri Olsa escreveu:
> On Mon, Nov 16, 2020 at 04:22:21PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Nov 16, 2020 at 04:15:44PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Nov 16, 2020 at 07:21:45PM +0100, Jiri Olsa escreveu:
> > > > On Mon, Nov 16, 2020 at 10:50:16AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Em Fri, Nov 13, 2020 at 01:43:47PM -0800, Andrii Nakryiko escreveu:
> > > > > > On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > > 
> > > > > > > On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> > > > > > > > On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > 
> > > > > > > > > Current conditions for picking up function records break
> > > > > > > > > BTF data on some gcc versions.
> > > > > 
> > > > > > > > > Some function records can appear with no arguments but with
> > > > > > > > > declaration tag set, so moving the 'fn->declaration' in front
> > > > > > > > > of other checks.
> > > > > 
> > > > > > > > > Then checking if argument names are present and finally checking
> > > > > > > > > ftrace filter if it's present. If ftrace filter is not available,
> > > > > > > > > using the external tag to filter out non external functions.
> > > > > 
> > > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > 
> > > > > > > > I tested locally, all seems to work fine. Left few suggestions below,
> > > > > > > > but those could be done in follow ups (or argued to not be done).
> > > > > 
> > > > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > > 
> > > > > > > > BTW, for some stats.
> > > > > 
> > > > > > > > BEFORE allowing static funcs:
> > > > > 
> > > > > 
> > > > > Nowhere in the last patchkit comments is some explanation for the
> > > > > inclusion of static functions :-\ After the first patch in the last
> > > > > series I get:
> > > > > 
> > > > >   $ llvm-objcopy --remove-section=.BTF vmlinux
> > > > >   $ readelf -SW vmlinux  | grep BTF
> > > > >   $ pahole -J vmlinux
> > > > >   $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > before.bpftool
> > > > >   $ cp vmlinux vmlinux.before.all
> > > > >   $ wc -l before.bpftool
> > > > >   28829 before.bpftool
> > > > 
> > > > I think you see the original number of functions, because without
> > > > the 'not merged' kernel patch, that added the special init section,
> > > > pahole will fail to detect vmlinux and fall back to checking dwarf
> > > > declarations
> > > 
> > > Indeed, I moved the verbose/force setting to the beggining of the
> > > encoder and:
> > > 
> > > ------------
> > > Found 352 per-CPU variables!
> > > vmlinux not detected, falling back to dwarf data
> > > File vmlinux:
> > > search cu '/home/acme/git/linux/arch/x86/kernel/head_64.S' for percpu global variables.
> > > -----------------
> > > 
> > > Now I have to read that code to figure out what that "vmlinux not
> > > detected, falling back to dwarf data" message means, as vmlinux is where
> > > DWARF data is, so what is that isn't being "detected", /me checks...
> > 
> > So with some debugging I see, the message is just confusing:
> > 
> > "vmlinux not detected, falling back to dwarf data (functions_cnt=53238, has_all_symbols(&fl)=0"
> 
> how about:
> 
> "ftrace data not detected, falling back to dwarf data"

Much better!
 
> > 
> > It finds the ELF symtab, finds the percpu variables there, tons of
> > functions, matching the number after this approach of marking BPF init
> > functions was dropped its just that vague "has_all_symbols()" routine
> > that fails to find all the symbols it needs in the vmlinux file.
> 
> we collect functions and other symbols in one loop over the symtab,
> so thats why we have all those collected and still can decide to fall back
> 
> before we needed also the init section symbols, now with this patch
> we need to know only mcount section begin/end

Thanks for the explanations, match my observations, its just that the
functions could have some more descriptive names :)

Please send the patch with the s/vmlinux/ftrace symbols/g and please
also s/dwarf/DWARF/g as its an acronym.

- Arnaldo

[PATCHv3 0/2] btf_encoder: Fix functions BTF data generation

[Jiri Olsa]

hi,
recent btf encoder's changes brakes BTF data for some gcc
versions. The problem is that some functions can appear
in dwarf data in some instances without arguments, while
they are defined with some.

v3 changes:
  - move 'generated' flag set out of should_generate_function
  - rename should_generate_function to find_function
  - added ack

v2 changes:
  - drop patch 3 logic and just change conditions
    based on Andrii's suggestion
  - drop patch 2
  - add ack for patch 1

thanks,
jirka


---
Jiri Olsa (2):
      btf_encoder: Generate also .init functions
      btf_encoder: Fix function generation

 btf_encoder.c | 86 +++++++++++++++++++++-----------------------------------------------------------------
 1 file changed, 21 insertions(+), 65 deletions(-)

[PATCH 2/2] btf_encoder: Fix function generation

[Jiri Olsa]

Current conditions for picking up function records break
BTF data on some gcc versions.

Some function records can appear with no arguments but with
declaration tag set, so moving the 'fn->declaration' in front
of other checks.

Then checking if argument names are present and finally checking
ftrace filter if it's present. If ftrace filter is not available,
using the external tag to filter out non external functions.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index d531651b1e9e..f3f6291391ee 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -164,18 +164,13 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 	return 0;
 }
 
-static bool should_generate_function(const struct btf_elf *btfe, const char *name)
+static struct elf_function *find_function(const struct btf_elf *btfe,
+					  const char *name)
 {
-	struct elf_function *p;
 	struct elf_function key = { .name = name };
 
-	p = bsearch(&key, functions, functions_cnt,
-		    sizeof(functions[0]), functions_cmp);
-	if (!p || p->generated)
-		return false;
-
-	p->generated = true;
-	return true;
+	return bsearch(&key, functions, functions_cnt, sizeof(functions[0]),
+		       functions_cmp);
 }
 
 static bool btf_name_char_ok(char c, bool first)
@@ -612,25 +607,25 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		const char *name;
 
 		/*
-		 * The functions_cnt != 0 means we parsed all necessary
-		 * kernel symbols and we are using ftrace location filter
-		 * for functions. If it's not available keep the current
-		 * dwarf declaration check.
+		 * Skip functions that:
+		 *   - are marked as declarations
+		 *   - do not have full argument names
+		 *   - are not in ftrace list (if it's available)
+		 *   - are not external (in case ftrace filter is not available)
 		 */
+		if (fn->declaration)
+			continue;
+		if (!has_arg_names(cu, &fn->proto))
+			continue;
 		if (functions_cnt) {
-			/*
-			 * We check following conditions:
-			 *   - argument names are defined
-			 *   - there's symbol and address defined for the function
-			 *   - function address belongs to ftrace locations
-			 *   - function is generated only once
-			 */
-			if (!has_arg_names(cu, &fn->proto))
-				continue;
-			if (!should_generate_function(btfe, function__name(fn, cu)))
+			struct elf_function *func;
+
+			func = find_function(btfe, function__name(fn, cu));
+			if (!func || func->generated)
 				continue;
+			func->generated = true;
 		} else {
-			if (fn->declaration || !fn->external)
+			if (!fn->external)
 				continue;
 		}
 
-- 
2.26.2