| Message ID | 20201113151222.852011-3-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | btf_encoder: Fix functions BTF data generation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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 >
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 >
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
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 >
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 > >
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
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 >
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
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 >
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
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; }
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(-)