* [PATCHv2 0/2] btf_encoder: Fix functions BTF data generation @ 2020-11-13 15:12 Jiri Olsa 2020-11-13 15:12 ` [PATCH 1/2] btf_encoder: Generate also .init functions Jiri Olsa 2020-11-13 15:12 ` [PATCH 2/2] btf_encoder: Fix function generation Jiri Olsa 0 siblings, 2 replies; 15+ messages in thread From: Jiri Olsa @ 2020-11-13 15:12 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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(-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] btf_encoder: Generate also .init functions 2020-11-13 15:12 [PATCHv2 0/2] btf_encoder: Fix functions BTF data generation Jiri Olsa @ 2020-11-13 15:12 ` Jiri Olsa 2020-11-13 15:12 ` [PATCH 2/2] btf_encoder: Fix function generation Jiri Olsa 1 sibling, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2020-11-13 15:12 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] btf_encoder: Fix function generation 2020-11-13 15:12 [PATCHv2 0/2] btf_encoder: Fix functions BTF data generation Jiri Olsa 2020-11-13 15:12 ` [PATCH 1/2] btf_encoder: Generate also .init functions Jiri Olsa @ 2020-11-13 15:12 ` Jiri Olsa 2020-11-13 17:28 ` Arnaldo Carvalho de Melo 2020-11-13 20:56 ` Andrii Nakryiko 1 sibling, 2 replies; 15+ messages in thread From: Jiri Olsa @ 2020-11-13 15:12 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-13 15:12 ` [PATCH 2/2] btf_encoder: Fix function generation Jiri Olsa @ 2020-11-13 17:28 ` Arnaldo Carvalho de Melo 2020-11-13 18:11 ` Jiri Olsa 2020-11-13 20:56 ` Andrii Nakryiko 1 sibling, 1 reply; 15+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-13 17:28 UTC (permalink / raw) To: Jiri Olsa Cc: dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-13 17:28 ` Arnaldo Carvalho de Melo @ 2020-11-13 18:11 ` Jiri Olsa 0 siblings, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2020-11-13 18:11 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-13 15:12 ` [PATCH 2/2] btf_encoder: Fix function generation Jiri Olsa 2020-11-13 17:28 ` Arnaldo Carvalho de Melo @ 2020-11-13 20:56 ` Andrii Nakryiko 2020-11-13 21:29 ` Jiri Olsa 1 sibling, 1 reply; 15+ messages in thread From: Andrii Nakryiko @ 2020-11-13 20:56 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-13 20:56 ` Andrii Nakryiko @ 2020-11-13 21:29 ` Jiri Olsa 2020-11-13 21:43 ` Andrii Nakryiko 0 siblings, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2020-11-13 21:29 UTC (permalink / raw) To: Andrii Nakryiko Cc: Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-13 21:29 ` Jiri Olsa @ 2020-11-13 21:43 ` Andrii Nakryiko 2020-11-16 13:50 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 15+ messages in thread From: Andrii Nakryiko @ 2020-11-13 21:43 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-13 21:43 ` Andrii Nakryiko @ 2020-11-16 13:50 ` Arnaldo Carvalho de Melo 2020-11-16 18:21 ` Jiri Olsa 0 siblings, 1 reply; 15+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-16 13:50 UTC (permalink / raw) To: Andrii Nakryiko Cc: Jiri Olsa, Jiri Olsa, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-16 13:50 ` Arnaldo Carvalho de Melo @ 2020-11-16 18:21 ` Jiri Olsa 2020-11-16 19:15 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2020-11-16 18:21 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, Jiri Olsa, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-16 18:21 ` Jiri Olsa @ 2020-11-16 19:15 ` Arnaldo Carvalho de Melo 2020-11-16 19:22 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 15+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-16 19:15 UTC (permalink / raw) To: Jiri Olsa Cc: Andrii Nakryiko, Jiri Olsa, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-16 19:15 ` Arnaldo Carvalho de Melo @ 2020-11-16 19:22 ` Arnaldo Carvalho de Melo 2020-11-16 19:40 ` Jiri Olsa 0 siblings, 1 reply; 15+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-16 19:22 UTC (permalink / raw) To: Jiri Olsa Cc: Andrii Nakryiko, Jiri Olsa, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-16 19:22 ` Arnaldo Carvalho de Melo @ 2020-11-16 19:40 ` Jiri Olsa 2020-11-16 19:44 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2020-11-16 19:40 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, Jiri Olsa, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] btf_encoder: Fix function generation 2020-11-16 19:40 ` Jiri Olsa @ 2020-11-16 19:44 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 15+ messages in thread From: Arnaldo Carvalho de Melo @ 2020-11-16 19:44 UTC (permalink / raw) To: Jiri Olsa Cc: Andrii Nakryiko, Jiri Olsa, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCHv3 0/2] btf_encoder: Fix functions BTF data generation @ 2020-11-14 22:38 Jiri Olsa 2020-11-14 22:38 ` [PATCH 2/2] btf_encoder: Fix function generation Jiri Olsa 0 siblings, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2020-11-14 22:38 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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(-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] btf_encoder: Fix function generation 2020-11-14 22:38 [PATCHv3 0/2] btf_encoder: Fix functions BTF data generation Jiri Olsa @ 2020-11-14 22:38 ` Jiri Olsa 0 siblings, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2020-11-14 22:38 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andrii Nakryiko, dwarves, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, Hao Luo 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-11-16 19:44 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-13 15:12 [PATCHv2 0/2] btf_encoder: Fix functions BTF data generation Jiri Olsa 2020-11-13 15:12 ` [PATCH 1/2] btf_encoder: Generate also .init functions Jiri Olsa 2020-11-13 15:12 ` [PATCH 2/2] btf_encoder: Fix function generation Jiri Olsa 2020-11-13 17:28 ` Arnaldo Carvalho de Melo 2020-11-13 18:11 ` Jiri Olsa 2020-11-13 20:56 ` Andrii Nakryiko 2020-11-13 21:29 ` Jiri Olsa 2020-11-13 21:43 ` Andrii Nakryiko 2020-11-16 13:50 ` Arnaldo Carvalho de Melo 2020-11-16 18:21 ` Jiri Olsa 2020-11-16 19:15 ` Arnaldo Carvalho de Melo 2020-11-16 19:22 ` Arnaldo Carvalho de Melo 2020-11-16 19:40 ` Jiri Olsa 2020-11-16 19:44 ` Arnaldo Carvalho de Melo 2020-11-14 22:38 [PATCHv3 0/2] btf_encoder: Fix functions BTF data generation Jiri Olsa 2020-11-14 22:38 ` [PATCH 2/2] btf_encoder: Fix function generation Jiri Olsa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).