| Message ID | 1676675433-10583-1-git-send-email-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | dwarves: change BTF encoding skip logic for functions | expand |
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. 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 >
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
On Fri, Feb 17, 2023 at 11:10:29PM +0000, Alan Maguire wrote: > 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. > > 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 changes look good, but I don't have that much insight into dwarf part of the code anyway I tested on x86 with gcc and clang bpf selftests and it looks good, and duplicate functions are gone Tested-by: Jiri Olsa <jolsa@kernel.org> thanks, jirka > > 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 >
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. 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(-)