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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> >
> > 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@xxxxxxxxxx>
> >
> > 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
>



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux