Re: [RFC dwarves 0/4] dwarves: change BTF encoding skip logic for functions

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Sat, Feb 18, 2023 at 11:12:50AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Feb 17, 2023 at 11:10:29PM +0000, Alan Maguire escreveu:
> > It has been observed [1] that the recent dwarves changes
> > that skip BTF encoding for functions that have optimized-out
> > parameters are too aggressive, leading to missing kfuncs
> > which generate warnings and a BPF selftest failure.
> > 
> > Here a different approach is used; we observe that
> > just because a function does not _use_ a parameter,
> > it does not mean it was not passed to it.  What we
> > are really keen to detect are cases where the calling
> > conventions are violated such that a function will
> > not have parameter values in the expected registers.
> > In such cases, tracing and kfunc behaviour will be
> > unstable.  We are not worried about parameters being
> > optimized out, provided that optimization does not
> > lead to other parameters being passed via
> > unexpected registers.
> > 
> > So this series attempts to detect such cases by
> > examining register (DW_OP_regX) values for
> > parameters where available; if these match
> > expectations, the function is deemed safe to add to
> > BTF, even if parameters are optimized out.
> > 
> > Using this approach, the only functions that
> > BTF generation is suppressed for are
> > 
> > 1. those with parameters that violate calling
> >    conventions where present; and
> > 2. those which have multiple inconsistent prototypes.
> 
> This sounds sensible at first sight, I've applied the patches so that it
> can go thru further testing on the libbpf CI, for now its just on the
> 'next' branch, the testing I did so far:
> 
> make allmodconfig + enablig BTF on bpf-next reverting that revert so
> that we use the new options,
> 
> Now I'm building and booting a kernel with a fedora-ish config without
> using the new options and then with them (reverting the revert) to
> compare the tools/testing/selftests/bpf/ ./test-progs output
> before/after.

-Summary: 274/1609 PASSED, 24 SKIPPED, 17 FAILED
+Summary: 276/1612 PASSED, 24 SKIPPED, 15 FAILED

Well, we're passing _more_ tests :-)

And no messages about that kernel module, etc. Will redo with clang as
time permits.

- Arnaldo
 
> Its an extended holiday down here, so I'll be spotty but want to get
> this moving forward,
> 
> Thanks!
> 
> - Arnaldo
>  
> > With these changes, running pahole on a gcc-built
> > vmlinux skips
> > 
> > - 1164 functions due to multiple inconsistent function
> >   prototypes.  Most of these are "."-suffixed optimized
> >   fuctions.
> > - 331 functions due to unexpected register usage
> > 
> > For a clang-built kernel, the numbers are
> > 
> > - 539 functions with inconsistent prototypes are skipped
> > - 209 functions with unexpected register usage are skipped
> > 
> > One complication is that functions that are passed
> > structs (or typedef structs) can use multiple registers
> > to pass those structures.  Examples include
> > bpf_lsm_socket_getpeersec_stream() (passing a typedef
> > struct sockptr_t) and the bpf_testmod_test_struct_arg_1
> > function in bpf_testmod.  Because multiple registers
> > are used to represent the structure, this throws
> > off expectations for any subsequent parameter->register
> > mappings.  To handle this, simply exempt functions
> > that have struct (or typedef struct) parameters from
> > our register checks.
> > 
> > Note to test this series on bpf-next, the following
> > commit should be reverted (reverting the revert
> > so that the flags are added to BTF encoding when
> > using pahole v1.25):
> > 
> > commit 1f5dfcc78ab4 ("Revert "bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25"")
> > 
> > With these changes we also see tracing_struct now pass:
> > 
> > $ sudo ./test_progs -t tracing_struct
> > #233     tracing_struct:OK
> > Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> > 
> > Further testing is needed - along with support for additional
> > parameter index -> DWARF reg for more platforms.
> > 
> > Future work could also add annotations for optimized-out
> > parameters via BTF tags to help guide tracing.
> > 
> > [1] https://lore.kernel.org/bpf/CAADnVQ+hfQ9LEmEFXneB7hm17NvRniXSShrHLaM-1BrguLjLQw@mail.gmail.com/
> > 
> > Alan Maguire (4):
> >   dwarf_loader: mark functions that do not use expected registers for
> >     params
> >   btf_encoder: exclude functions with unexpected param register use not
> >     optimizations
> >   pahole: update descriptions for btf_gen_optimized,
> >     skip_encoding_btf_inconsistent_proto
> >   pahole: update man page for options also
> > 
> >  btf_encoder.c      |  24 +++++++---
> >  dwarf_loader.c     | 109 ++++++++++++++++++++++++++++++++++++++++++---
> >  dwarves.h          |   5 +++
> >  man-pages/pahole.1 |   4 +-
> >  pahole.c           |   4 +-
> >  5 files changed, 129 insertions(+), 17 deletions(-)
> > 
> > -- 
> > 2.31.1
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo