* [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25
@ 2023-05-10 13:02 Alan Maguire
2023-05-10 17:15 ` Jiri Olsa
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Alan Maguire @ 2023-05-10 13:02 UTC (permalink / raw)
To: ast, daniel, andrii, acme
Cc: jolsa, laoar.shao, martin.lau, song, yhs, john.fastabend, kpsingh,
sdf, haoluo, bpf, Alan Maguire
v1.25 of pahole supports filtering out functions with multiple inconsistent
function prototypes or optimized-out parameters from the BTF representation.
These present problems because there is no additional info in BTF saying which
inconsistent prototype matches which function instance to help guide attachment,
and functions with optimized-out parameters can lead to incorrect assumptions
about register contents.
So for now, filter out such functions while adding BTF representations for
functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters.
This patch assumes that below linked changes land in pahole for v1.25.
Issues with pahole filtering being too aggressive in removing functions
appear to be resolved now, but CI and further testing will confirm.
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
scripts/pahole-flags.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
index 1f1f1d397c39..728d55190d97 100755
--- a/scripts/pahole-flags.sh
+++ b/scripts/pahole-flags.sh
@@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
# see PAHOLE_HAS_LANG_EXCLUDE
extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
fi
+if [ "${pahole_ver}" -ge "125" ]; then
+ extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
+fi
echo ${extra_paholeopt}
--
2.31.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-10 13:02 [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 Alan Maguire @ 2023-05-10 17:15 ` Jiri Olsa 2023-05-12 2:51 ` Yafang Shao 2023-05-12 19:00 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 28+ messages in thread From: Jiri Olsa @ 2023-05-10 17:15 UTC (permalink / raw) To: Alan Maguire Cc: ast, daniel, andrii, acme, laoar.shao, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, bpf On Wed, May 10, 2023 at 02:02:41PM +0100, Alan Maguire wrote: > v1.25 of pahole supports filtering out functions with multiple inconsistent > function prototypes or optimized-out parameters from the BTF representation. > These present problems because there is no additional info in BTF saying which > inconsistent prototype matches which function instance to help guide attachment, > and functions with optimized-out parameters can lead to incorrect assumptions > about register contents. > > So for now, filter out such functions while adding BTF representations for > functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters. > This patch assumes that below linked changes land in pahole for v1.25. > > Issues with pahole filtering being too aggressive in removing functions > appear to be resolved now, but CI and further testing will confirm. > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> Acked-by: Jiri Olsa <jolsa@kernel.org> jirka > --- > scripts/pahole-flags.sh | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > index 1f1f1d397c39..728d55190d97 100755 > --- a/scripts/pahole-flags.sh > +++ b/scripts/pahole-flags.sh > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > # see PAHOLE_HAS_LANG_EXCLUDE > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > fi > +if [ "${pahole_ver}" -ge "125" ]; then > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > +fi > > echo ${extra_paholeopt} > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-10 13:02 [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 Alan Maguire 2023-05-10 17:15 ` Jiri Olsa @ 2023-05-12 2:51 ` Yafang Shao 2023-05-12 16:03 ` Alan Maguire 2023-05-12 19:00 ` patchwork-bot+netdevbpf 2 siblings, 1 reply; 28+ messages in thread From: Yafang Shao @ 2023-05-12 2:51 UTC (permalink / raw) To: Alan Maguire Cc: ast, daniel, andrii, acme, jolsa, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, bpf On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote: > > v1.25 of pahole supports filtering out functions with multiple inconsistent > function prototypes or optimized-out parameters from the BTF representation. > These present problems because there is no additional info in BTF saying which > inconsistent prototype matches which function instance to help guide attachment, > and functions with optimized-out parameters can lead to incorrect assumptions > about register contents. > > So for now, filter out such functions while adding BTF representations for > functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters. > This patch assumes that below linked changes land in pahole for v1.25. > > Issues with pahole filtering being too aggressive in removing functions > appear to be resolved now, but CI and further testing will confirm. > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > --- > scripts/pahole-flags.sh | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > index 1f1f1d397c39..728d55190d97 100755 > --- a/scripts/pahole-flags.sh > +++ b/scripts/pahole-flags.sh > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > # see PAHOLE_HAS_LANG_EXCLUDE > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > fi > +if [ "${pahole_ver}" -ge "125" ]; then > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > +fi > > echo ${extra_paholeopt} > -- > 2.31.1 > That change looks like a workaround to me. There may be multiple functions that have the same proto, e.g.: $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" kernel/bpf/ net/core/ kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct bpf_iter_aux_info *aux) net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct bpf_iter_aux_info *aux) $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 bpf_iter_detach_map [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 'aux' type_id=2638 [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static We don't know which one it is in the BTF. However, I'm not against this change, as it can avoid some issues. -- Regards Yafang ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-12 2:51 ` Yafang Shao @ 2023-05-12 16:03 ` Alan Maguire 2023-05-12 18:59 ` Alexei Starovoitov 0 siblings, 1 reply; 28+ messages in thread From: Alan Maguire @ 2023-05-12 16:03 UTC (permalink / raw) To: Yafang Shao Cc: ast, daniel, andrii, acme, jolsa, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, bpf On 12/05/2023 03:51, Yafang Shao wrote: > On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote: >> >> v1.25 of pahole supports filtering out functions with multiple inconsistent >> function prototypes or optimized-out parameters from the BTF representation. >> These present problems because there is no additional info in BTF saying which >> inconsistent prototype matches which function instance to help guide attachment, >> and functions with optimized-out parameters can lead to incorrect assumptions >> about register contents. >> >> So for now, filter out such functions while adding BTF representations for >> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters. >> This patch assumes that below linked changes land in pahole for v1.25. >> >> Issues with pahole filtering being too aggressive in removing functions >> appear to be resolved now, but CI and further testing will confirm. >> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >> --- >> scripts/pahole-flags.sh | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh >> index 1f1f1d397c39..728d55190d97 100755 >> --- a/scripts/pahole-flags.sh >> +++ b/scripts/pahole-flags.sh >> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >> # see PAHOLE_HAS_LANG_EXCLUDE >> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >> fi >> +if [ "${pahole_ver}" -ge "125" ]; then >> + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" >> +fi >> >> echo ${extra_paholeopt} >> -- >> 2.31.1 >> > > That change looks like a workaround to me. > There may be multiple functions that have the same proto, e.g.: > > $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" > kernel/bpf/ net/core/ > kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct > bpf_iter_aux_info *aux) > net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct > bpf_iter_aux_info *aux) > > $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 > bpf_iter_detach_map > [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 > 'aux' type_id=2638 > [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static > > We don't know which one it is in the BTF. > However, I'm not against this change, as it can avoid some issues. > In the above case, the BTF representation is consistent though. That is, if I attach fentry progs to either of these functions based on that BTF representation, nothing will crash. That's ultimately what those changes are about; ensuring consistency in BTF representation, so when a function is in BTF we can know the signature of the function can be safely used by fentry for example. The question of being able to identify functions (as opposed to having a consistent representation) is the next step. Finding a way to link between kallsyms and BTF would allow us to have multiple inconsistent functions in BTF, since we could map from BTF -> kallsyms safely. So two functions called "foo" with different function signatures would be okay, because we'd know which was which in kallsyms and could attach safely. Something like a BTF tag for the function that could clarify that mapping - but just for cases where it would otherwise be ambiguous - is probably the way forward longer term. Jiri's talking about this topic at LSF/MM/BPF this week I believe. Thanks! Alan ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-12 16:03 ` Alan Maguire @ 2023-05-12 18:59 ` Alexei Starovoitov 2023-05-12 21:54 ` Jiri Olsa 0 siblings, 1 reply; 28+ messages in thread From: Alexei Starovoitov @ 2023-05-12 18:59 UTC (permalink / raw) To: Alan Maguire Cc: Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Arnaldo Carvalho de Melo, Jiri Olsa, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf On Fri, May 12, 2023 at 9:04 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > On 12/05/2023 03:51, Yafang Shao wrote: > > On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote: > >> > >> v1.25 of pahole supports filtering out functions with multiple inconsistent > >> function prototypes or optimized-out parameters from the BTF representation. > >> These present problems because there is no additional info in BTF saying which > >> inconsistent prototype matches which function instance to help guide attachment, > >> and functions with optimized-out parameters can lead to incorrect assumptions > >> about register contents. > >> > >> So for now, filter out such functions while adding BTF representations for > >> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters. > >> This patch assumes that below linked changes land in pahole for v1.25. > >> > >> Issues with pahole filtering being too aggressive in removing functions > >> appear to be resolved now, but CI and further testing will confirm. > >> > >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > >> --- > >> scripts/pahole-flags.sh | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > >> index 1f1f1d397c39..728d55190d97 100755 > >> --- a/scripts/pahole-flags.sh > >> +++ b/scripts/pahole-flags.sh > >> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > >> # see PAHOLE_HAS_LANG_EXCLUDE > >> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > >> fi > >> +if [ "${pahole_ver}" -ge "125" ]; then > >> + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > >> +fi > >> > >> echo ${extra_paholeopt} > >> -- > >> 2.31.1 > >> > > > > That change looks like a workaround to me. > > There may be multiple functions that have the same proto, e.g.: > > > > $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" > > kernel/bpf/ net/core/ > > kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct > > bpf_iter_aux_info *aux) > > net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct > > bpf_iter_aux_info *aux) > > > > $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 > > bpf_iter_detach_map > > [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 > > 'aux' type_id=2638 > > [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static > > > > We don't know which one it is in the BTF. > > However, I'm not against this change, as it can avoid some issues. > > > > In the above case, the BTF representation is consistent though. > That is, if I attach fentry progs to either of these functions > based on that BTF representation, nothing will crash. > > That's ultimately what those changes are about; ensuring > consistency in BTF representation, so when a function is in > BTF we can know the signature of the function can be safely > used by fentry for example. > > The question of being able to identify functions (as opposed > to having a consistent representation) is the next step. > Finding a way to link between kallsyms and BTF would allow us to > have multiple inconsistent functions in BTF, since we could map > from BTF -> kallsyms safely. So two functions called "foo" > with different function signatures would be okay, because > we'd know which was which in kallsyms and could attach > safely. Something like a BTF tag for the function that could > clarify that mapping - but just for cases where it would > otherwise be ambiguous - is probably the way forward > longer term. > > Jiri's talking about this topic at LSF/MM/BPF this week I believe. Jiri presented a few ideas during LSFMMBPF. I feel the best approach is to add a set of addr-s to BTF via a special decl_tag. We can also consider extending KIND_FUNC. The advantage that every BTF func will have one or more addrs associated with it and bpf prog loading logic wouldn't need to do fragile name comparison between btf and kallsyms. pahole can take addrs from dwarf and optionally double check with kallsyms. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-12 18:59 ` Alexei Starovoitov @ 2023-05-12 21:54 ` Jiri Olsa 2023-05-13 2:59 ` Yonghong Song 0 siblings, 1 reply; 28+ messages in thread From: Jiri Olsa @ 2023-05-12 21:54 UTC (permalink / raw) To: Alexei Starovoitov Cc: Alan Maguire, Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Arnaldo Carvalho de Melo, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Eduard Zingerman, Lorenz Bauer, Timo Beckers On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote: > On Fri, May 12, 2023 at 9:04 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > On 12/05/2023 03:51, Yafang Shao wrote: > > > On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote: > > >> > > >> v1.25 of pahole supports filtering out functions with multiple inconsistent > > >> function prototypes or optimized-out parameters from the BTF representation. > > >> These present problems because there is no additional info in BTF saying which > > >> inconsistent prototype matches which function instance to help guide attachment, > > >> and functions with optimized-out parameters can lead to incorrect assumptions > > >> about register contents. > > >> > > >> So for now, filter out such functions while adding BTF representations for > > >> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters. > > >> This patch assumes that below linked changes land in pahole for v1.25. > > >> > > >> Issues with pahole filtering being too aggressive in removing functions > > >> appear to be resolved now, but CI and further testing will confirm. > > >> > > >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > >> --- > > >> scripts/pahole-flags.sh | 3 +++ > > >> 1 file changed, 3 insertions(+) > > >> > > >> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > > >> index 1f1f1d397c39..728d55190d97 100755 > > >> --- a/scripts/pahole-flags.sh > > >> +++ b/scripts/pahole-flags.sh > > >> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > >> # see PAHOLE_HAS_LANG_EXCLUDE > > >> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > >> fi > > >> +if [ "${pahole_ver}" -ge "125" ]; then > > >> + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > >> +fi > > >> > > >> echo ${extra_paholeopt} > > >> -- > > >> 2.31.1 > > >> > > > > > > That change looks like a workaround to me. > > > There may be multiple functions that have the same proto, e.g.: > > > > > > $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" > > > kernel/bpf/ net/core/ > > > kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct > > > bpf_iter_aux_info *aux) > > > net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct > > > bpf_iter_aux_info *aux) > > > > > > $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 > > > bpf_iter_detach_map > > > [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 > > > 'aux' type_id=2638 > > > [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static > > > > > > We don't know which one it is in the BTF. > > > However, I'm not against this change, as it can avoid some issues. > > > > > > > In the above case, the BTF representation is consistent though. > > That is, if I attach fentry progs to either of these functions > > based on that BTF representation, nothing will crash. > > > > That's ultimately what those changes are about; ensuring > > consistency in BTF representation, so when a function is in > > BTF we can know the signature of the function can be safely > > used by fentry for example. > > > > The question of being able to identify functions (as opposed > > to having a consistent representation) is the next step. > > Finding a way to link between kallsyms and BTF would allow us to > > have multiple inconsistent functions in BTF, since we could map > > from BTF -> kallsyms safely. So two functions called "foo" > > with different function signatures would be okay, because > > we'd know which was which in kallsyms and could attach > > safely. Something like a BTF tag for the function that could > > clarify that mapping - but just for cases where it would > > otherwise be ambiguous - is probably the way forward > > longer term. > > > > Jiri's talking about this topic at LSF/MM/BPF this week I believe. > > Jiri presented a few ideas during LSFMMBPF. > > I feel the best approach is to add a set of addr-s to BTF > via a special decl_tag. > We can also consider extending KIND_FUNC. > The advantage that every BTF func will have one or more addrs > associated with it and bpf prog loading logic wouldn't need to do > fragile name comparison between btf and kallsyms. > pahole can take addrs from dwarf and optionally double check with kallsyms. Yonghong summed it up in another email discussion, pasting it in here: So overall we have three options as kallsyms representation now: (a) "addr module:foo:dir_a/dir_b/core.c" (b) "addr module:foo" (c) "addr module:foo:btf_id" option (a): 'dir_a/dir_b/core.c' needs to be encoded in BTF. user space either check file path or func signature to find attach_btf_id and pass to the kernel. kernel can find file path in BTF and then lookup kallsyms to find addr. option (b): "addr" needs to be encoded in BTF. user space checks func signature to find attach_btf_id and pass to the kernel. kernel can find addr in BTF and use it. option (c): if user can decide which function to attach, e.g., through func signature, then no BTF encoding is necessary. attach_btf_id is passed to the kernel and search kallsyms to find the matching btf_id and 'addr' will be available then. For option (b) and (c), user space needs to check func signature to find which btf_id to use. If same-name static functions having the identical signatures, then user space would have a hard time to differentiate. I think it should be very rare same-name static functions in the kernel will have identical signatures. But if we want 100% correctness, we may need file path in which case option (a) is preferable. Current option (a) kallsyms format is under review. option (c) also needs kallsyms change... my thoughts so far is that I like the idea of storing functions address in BTF (option b), because it's the easiest way on the other hand, user would need debug info to find address for the function to trace.. but still just for small subset of functions that share same name also I like Lorenz's idea of storing BTF ID in kalsyms and verifier being able to lookup address based on BTF ID.. seems like easier kallsyms change, but it would still require storing objects paths in BTF to pick up the correct one cc-ing other folks jirka ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-12 21:54 ` Jiri Olsa @ 2023-05-13 2:59 ` Yonghong Song 2023-05-14 17:37 ` Yonghong Song 0 siblings, 1 reply; 28+ messages in thread From: Yonghong Song @ 2023-05-13 2:59 UTC (permalink / raw) To: Jiri Olsa, Alexei Starovoitov Cc: Alan Maguire, Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Arnaldo Carvalho de Melo, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Eduard Zingerman, Lorenz Bauer, Timo Beckers On 5/12/23 2:54 PM, Jiri Olsa wrote: > On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote: >> On Fri, May 12, 2023 at 9:04 AM Alan Maguire <alan.maguire@oracle.com> wrote: >>> >>> On 12/05/2023 03:51, Yafang Shao wrote: >>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@oracle.com> wrote: >>>>> >>>>> v1.25 of pahole supports filtering out functions with multiple inconsistent >>>>> function prototypes or optimized-out parameters from the BTF representation. >>>>> These present problems because there is no additional info in BTF saying which >>>>> inconsistent prototype matches which function instance to help guide attachment, >>>>> and functions with optimized-out parameters can lead to incorrect assumptions >>>>> about register contents. >>>>> >>>>> So for now, filter out such functions while adding BTF representations for >>>>> functions that have "."-suffixes (foo.isra.0) but not optimized-out parameters. >>>>> This patch assumes that below linked changes land in pahole for v1.25. >>>>> >>>>> Issues with pahole filtering being too aggressive in removing functions >>>>> appear to be resolved now, but CI and further testing will confirm. >>>>> >>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >>>>> --- >>>>> scripts/pahole-flags.sh | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh >>>>> index 1f1f1d397c39..728d55190d97 100755 >>>>> --- a/scripts/pahole-flags.sh >>>>> +++ b/scripts/pahole-flags.sh >>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >>>>> # see PAHOLE_HAS_LANG_EXCLUDE >>>>> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >>>>> fi >>>>> +if [ "${pahole_ver}" -ge "125" ]; then >>>>> + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" >>>>> +fi >>>>> >>>>> echo ${extra_paholeopt} >>>>> -- >>>>> 2.31.1 >>>>> >>>> >>>> That change looks like a workaround to me. >>>> There may be multiple functions that have the same proto, e.g.: >>>> >>>> $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" >>>> kernel/bpf/ net/core/ >>>> kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct >>>> bpf_iter_aux_info *aux) >>>> net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct >>>> bpf_iter_aux_info *aux) >>>> >>>> $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 >>>> bpf_iter_detach_map >>>> [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 >>>> 'aux' type_id=2638 >>>> [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static >>>> >>>> We don't know which one it is in the BTF. >>>> However, I'm not against this change, as it can avoid some issues. >>>> >>> >>> In the above case, the BTF representation is consistent though. >>> That is, if I attach fentry progs to either of these functions >>> based on that BTF representation, nothing will crash. >>> >>> That's ultimately what those changes are about; ensuring >>> consistency in BTF representation, so when a function is in >>> BTF we can know the signature of the function can be safely >>> used by fentry for example. >>> >>> The question of being able to identify functions (as opposed >>> to having a consistent representation) is the next step. >>> Finding a way to link between kallsyms and BTF would allow us to >>> have multiple inconsistent functions in BTF, since we could map >>> from BTF -> kallsyms safely. So two functions called "foo" >>> with different function signatures would be okay, because >>> we'd know which was which in kallsyms and could attach >>> safely. Something like a BTF tag for the function that could >>> clarify that mapping - but just for cases where it would >>> otherwise be ambiguous - is probably the way forward >>> longer term. >>> >>> Jiri's talking about this topic at LSF/MM/BPF this week I believe. >> >> Jiri presented a few ideas during LSFMMBPF. >> >> I feel the best approach is to add a set of addr-s to BTF >> via a special decl_tag. >> We can also consider extending KIND_FUNC. >> The advantage that every BTF func will have one or more addrs >> associated with it and bpf prog loading logic wouldn't need to do >> fragile name comparison between btf and kallsyms. >> pahole can take addrs from dwarf and optionally double check with kallsyms. > > Yonghong summed it up in another email discussion, pasting it in here: > > So overall we have three options as kallsyms representation now: > (a) "addr module:foo:dir_a/dir_b/core.c" > (b) "addr module:foo" > (c) "addr module:foo:btf_id" > > option (a): > 'dir_a/dir_b/core.c' needs to be encoded in BTF. > user space either check file path or func signature > to find attach_btf_id and pass to the kernel. > kernel can find file path in BTF and then lookup > kallsyms to find addr. > > option (b): > "addr" needs to be encoded in BTF. > user space checks func signature to find > attach_btf_id and pass to the kernel. > kernel can find addr in BTF and use it. > > option (c): > if user can decide which function to attach, e.g., > through func signature, then no BTF encoding > is necessary. attach_btf_id is passed to the > kernel and search kallsyms to find the matching > btf_id and 'addr' will be available then. > > For option (b) and (c), user space needs to check > func signature to find which btf_id to use. If > same-name static functions having the identical > signatures, then user space would have a hard time > to differentiate. I think it should be very > rare same-name static functions in the kernel will have > identical signatures. But if we want 100% correctness, > we may need file path in which case option (a) > is preferable. As Alexei mentioned in previous email, for such a extreme case, if user is willing to go through extra step to check dwarf to find and match file path, then (b) and (c) should work perfectly as well. > > Current option (a) kallsyms format is under review. > option (c) also needs kallsyms change... > > > my thoughts so far is that I like the idea of storing functions address > in BTF (option b), because it's the easiest way > > on the other hand, user would need debug info to find address for the function > to trace.. but still just for small subset of functions that share same name > > also I like Lorenz's idea of storing BTF ID in kalsyms and verifier being > able to lookup address based on BTF ID.. seems like easier kallsyms change, > but it would still require storing objects paths in BTF to pick up the > correct one > > cc-ing other folks > > jirka ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-13 2:59 ` Yonghong Song @ 2023-05-14 17:37 ` Yonghong Song 2023-05-14 21:49 ` Jiri Olsa 0 siblings, 1 reply; 28+ messages in thread From: Yonghong Song @ 2023-05-14 17:37 UTC (permalink / raw) To: Jiri Olsa, Alexei Starovoitov Cc: Alan Maguire, Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Arnaldo Carvalho de Melo, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Eduard Zingerman, Lorenz Bauer, Timo Beckers On 5/12/23 7:59 PM, Yonghong Song wrote: > > > On 5/12/23 2:54 PM, Jiri Olsa wrote: >> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote: >>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire >>> <alan.maguire@oracle.com> wrote: >>>> >>>> On 12/05/2023 03:51, Yafang Shao wrote: >>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire >>>>> <alan.maguire@oracle.com> wrote: >>>>>> >>>>>> v1.25 of pahole supports filtering out functions with multiple >>>>>> inconsistent >>>>>> function prototypes or optimized-out parameters from the BTF >>>>>> representation. >>>>>> These present problems because there is no additional info in BTF >>>>>> saying which >>>>>> inconsistent prototype matches which function instance to help >>>>>> guide attachment, >>>>>> and functions with optimized-out parameters can lead to incorrect >>>>>> assumptions >>>>>> about register contents. >>>>>> >>>>>> So for now, filter out such functions while adding BTF >>>>>> representations for >>>>>> functions that have "."-suffixes (foo.isra.0) but not >>>>>> optimized-out parameters. >>>>>> This patch assumes that below linked changes land in pahole for >>>>>> v1.25. >>>>>> >>>>>> Issues with pahole filtering being too aggressive in removing >>>>>> functions >>>>>> appear to be resolved now, but CI and further testing will confirm. >>>>>> >>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >>>>>> --- >>>>>> scripts/pahole-flags.sh | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh >>>>>> index 1f1f1d397c39..728d55190d97 100755 >>>>>> --- a/scripts/pahole-flags.sh >>>>>> +++ b/scripts/pahole-flags.sh >>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >>>>>> # see PAHOLE_HAS_LANG_EXCLUDE >>>>>> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >>>>>> fi >>>>>> +if [ "${pahole_ver}" -ge "125" ]; then >>>>>> + extra_paholeopt="${extra_paholeopt} >>>>>> --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" >>>>>> +fi >>>>>> >>>>>> echo ${extra_paholeopt} >>>>>> -- >>>>>> 2.31.1 >>>>>> >>>>> >>>>> That change looks like a workaround to me. >>>>> There may be multiple functions that have the same proto, e.g.: >>>>> >>>>> $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" >>>>> kernel/bpf/ net/core/ >>>>> kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct >>>>> bpf_iter_aux_info *aux) >>>>> net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct >>>>> bpf_iter_aux_info *aux) >>>>> >>>>> $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 >>>>> bpf_iter_detach_map >>>>> [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 >>>>> 'aux' type_id=2638 >>>>> [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static >>>>> >>>>> We don't know which one it is in the BTF. >>>>> However, I'm not against this change, as it can avoid some issues. >>>>> >>>> >>>> In the above case, the BTF representation is consistent though. >>>> That is, if I attach fentry progs to either of these functions >>>> based on that BTF representation, nothing will crash. >>>> >>>> That's ultimately what those changes are about; ensuring >>>> consistency in BTF representation, so when a function is in >>>> BTF we can know the signature of the function can be safely >>>> used by fentry for example. >>>> >>>> The question of being able to identify functions (as opposed >>>> to having a consistent representation) is the next step. >>>> Finding a way to link between kallsyms and BTF would allow us to >>>> have multiple inconsistent functions in BTF, since we could map >>>> from BTF -> kallsyms safely. So two functions called "foo" >>>> with different function signatures would be okay, because >>>> we'd know which was which in kallsyms and could attach >>>> safely. Something like a BTF tag for the function that could >>>> clarify that mapping - but just for cases where it would >>>> otherwise be ambiguous - is probably the way forward >>>> longer term. >>>> >>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe. >>> >>> Jiri presented a few ideas during LSFMMBPF. >>> >>> I feel the best approach is to add a set of addr-s to BTF >>> via a special decl_tag. >>> We can also consider extending KIND_FUNC. >>> The advantage that every BTF func will have one or more addrs >>> associated with it and bpf prog loading logic wouldn't need to do >>> fragile name comparison between btf and kallsyms. >>> pahole can take addrs from dwarf and optionally double check with >>> kallsyms. >> >> Yonghong summed it up in another email discussion, pasting it in here: >> >> So overall we have three options as kallsyms representation now: >> (a) "addr module:foo:dir_a/dir_b/core.c" >> (b) "addr module:foo" >> (c) "addr module:foo:btf_id" >> >> option (a): >> 'dir_a/dir_b/core.c' needs to be encoded in BTF. >> user space either check file path or func signature >> to find attach_btf_id and pass to the kernel. >> kernel can find file path in BTF and then lookup >> kallsyms to find addr. >> >> option (b): >> "addr" needs to be encoded in BTF. >> user space checks func signature to find >> attach_btf_id and pass to the kernel. >> kernel can find addr in BTF and use it. >> >> option (c): >> if user can decide which function to attach, e.g., >> through func signature, then no BTF encoding >> is necessary. attach_btf_id is passed to the >> kernel and search kallsyms to find the matching >> btf_id and 'addr' will be available then. >> >> For option (b) and (c), user space needs to check >> func signature to find which btf_id to use. If >> same-name static functions having the identical >> signatures, then user space would have a hard time >> to differentiate. I think it should be very >> rare same-name static functions in the kernel will have >> identical signatures. But if we want 100% correctness, >> we may need file path in which case option (a) >> is preferable. > > As Alexei mentioned in previous email, for such a extreme case, > if user is willing to go through extra step to check dwarf > to find and match file path, then (b) and (c) should work > perfectly as well. Okay, it looks like this is more complex if the function signature is the same. In such cases, current BTF dedup will merge these functions as a single BTF func. In such cases, we could have: decl_tag_1 ----> dedup'ed static_func ^ | decl_tag_2 --------- For such cases, just passing btf_id of static func to kernel won't work since the kernel won't be able to know which decl_tag to be associated with. (I did a simple test with vmlinux, it looks we have issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func as well since only one of decl_tag survives. But this is a different issue. ) So if we intend to add decl tag (addr or file_path), we should not dedup static functions or generally any functions. > >> >> Current option (a) kallsyms format is under review. >> option (c) also needs kallsyms change... >> >> >> my thoughts so far is that I like the idea of storing functions address >> in BTF (option b), because it's the easiest way >> >> on the other hand, user would need debug info to find address for the >> function >> to trace.. but still just for small subset of functions that share >> same name >> >> also I like Lorenz's idea of storing BTF ID in kalsyms and verifier being >> able to lookup address based on BTF ID.. seems like easier kallsyms >> change, >> but it would still require storing objects paths in BTF to pick up the >> correct one >> >> cc-ing other folks >> >> jirka ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-14 17:37 ` Yonghong Song @ 2023-05-14 21:49 ` Jiri Olsa 2023-05-15 4:06 ` Yonghong Song 0 siblings, 1 reply; 28+ messages in thread From: Jiri Olsa @ 2023-05-14 21:49 UTC (permalink / raw) To: Yonghong Song Cc: Jiri Olsa, Alexei Starovoitov, Alan Maguire, Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Arnaldo Carvalho de Melo, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Eduard Zingerman, Lorenz Bauer, Timo Beckers On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote: > > > On 5/12/23 7:59 PM, Yonghong Song wrote: > > > > > > On 5/12/23 2:54 PM, Jiri Olsa wrote: > > > On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote: > > > > On Fri, May 12, 2023 at 9:04 AM Alan Maguire > > > > <alan.maguire@oracle.com> wrote: > > > > > > > > > > On 12/05/2023 03:51, Yafang Shao wrote: > > > > > > On Wed, May 10, 2023 at 9:03 PM Alan Maguire > > > > > > <alan.maguire@oracle.com> wrote: > > > > > > > > > > > > > > v1.25 of pahole supports filtering out functions > > > > > > > with multiple inconsistent > > > > > > > function prototypes or optimized-out parameters from > > > > > > > the BTF representation. > > > > > > > These present problems because there is no > > > > > > > additional info in BTF saying which > > > > > > > inconsistent prototype matches which function > > > > > > > instance to help guide attachment, > > > > > > > and functions with optimized-out parameters can lead > > > > > > > to incorrect assumptions > > > > > > > about register contents. > > > > > > > > > > > > > > So for now, filter out such functions while adding > > > > > > > BTF representations for > > > > > > > functions that have "."-suffixes (foo.isra.0) but > > > > > > > not optimized-out parameters. > > > > > > > This patch assumes that below linked changes land in > > > > > > > pahole for v1.25. > > > > > > > > > > > > > > Issues with pahole filtering being too aggressive in > > > > > > > removing functions > > > > > > > appear to be resolved now, but CI and further testing will confirm. > > > > > > > > > > > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > > > > > > --- > > > > > > > scripts/pahole-flags.sh | 3 +++ > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > > > > > > > index 1f1f1d397c39..728d55190d97 100755 > > > > > > > --- a/scripts/pahole-flags.sh > > > > > > > +++ b/scripts/pahole-flags.sh > > > > > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > > > > > > # see PAHOLE_HAS_LANG_EXCLUDE > > > > > > > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > > > > > > fi > > > > > > > +if [ "${pahole_ver}" -ge "125" ]; then > > > > > > > + extra_paholeopt="${extra_paholeopt} > > > > > > > --skip_encoding_btf_inconsistent_proto > > > > > > > --btf_gen_optimized" > > > > > > > +fi > > > > > > > > > > > > > > echo ${extra_paholeopt} > > > > > > > -- > > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > That change looks like a workaround to me. > > > > > > There may be multiple functions that have the same proto, e.g.: > > > > > > > > > > > > $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" > > > > > > kernel/bpf/ net/core/ > > > > > > kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct > > > > > > bpf_iter_aux_info *aux) > > > > > > net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct > > > > > > bpf_iter_aux_info *aux) > > > > > > > > > > > > $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 > > > > > > bpf_iter_detach_map > > > > > > [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 > > > > > > 'aux' type_id=2638 > > > > > > [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static > > > > > > > > > > > > We don't know which one it is in the BTF. > > > > > > However, I'm not against this change, as it can avoid some issues. > > > > > > > > > > > > > > > > In the above case, the BTF representation is consistent though. > > > > > That is, if I attach fentry progs to either of these functions > > > > > based on that BTF representation, nothing will crash. > > > > > > > > > > That's ultimately what those changes are about; ensuring > > > > > consistency in BTF representation, so when a function is in > > > > > BTF we can know the signature of the function can be safely > > > > > used by fentry for example. > > > > > > > > > > The question of being able to identify functions (as opposed > > > > > to having a consistent representation) is the next step. > > > > > Finding a way to link between kallsyms and BTF would allow us to > > > > > have multiple inconsistent functions in BTF, since we could map > > > > > from BTF -> kallsyms safely. So two functions called "foo" > > > > > with different function signatures would be okay, because > > > > > we'd know which was which in kallsyms and could attach > > > > > safely. Something like a BTF tag for the function that could > > > > > clarify that mapping - but just for cases where it would > > > > > otherwise be ambiguous - is probably the way forward > > > > > longer term. > > > > > > > > > > Jiri's talking about this topic at LSF/MM/BPF this week I believe. > > > > > > > > Jiri presented a few ideas during LSFMMBPF. > > > > > > > > I feel the best approach is to add a set of addr-s to BTF > > > > via a special decl_tag. > > > > We can also consider extending KIND_FUNC. > > > > The advantage that every BTF func will have one or more addrs > > > > associated with it and bpf prog loading logic wouldn't need to do > > > > fragile name comparison between btf and kallsyms. > > > > pahole can take addrs from dwarf and optionally double check > > > > with kallsyms. > > > > > > Yonghong summed it up in another email discussion, pasting it in here: > > > > > > So overall we have three options as kallsyms representation now: > > > (a) "addr module:foo:dir_a/dir_b/core.c" > > > (b) "addr module:foo" > > > (c) "addr module:foo:btf_id" > > > > > > option (a): > > > 'dir_a/dir_b/core.c' needs to be encoded in BTF. > > > user space either check file path or func signature > > > to find attach_btf_id and pass to the kernel. > > > kernel can find file path in BTF and then lookup > > > kallsyms to find addr. > > > > > > option (b): > > > "addr" needs to be encoded in BTF. > > > user space checks func signature to find > > > attach_btf_id and pass to the kernel. > > > kernel can find addr in BTF and use it. > > > > > > option (c): > > > if user can decide which function to attach, e.g., > > > through func signature, then no BTF encoding > > > is necessary. attach_btf_id is passed to the > > > kernel and search kallsyms to find the matching > > > btf_id and 'addr' will be available then. > > > > > > For option (b) and (c), user space needs to check > > > func signature to find which btf_id to use. If > > > same-name static functions having the identical > > > signatures, then user space would have a hard time > > > to differentiate. I think it should be very > > > rare same-name static functions in the kernel will have > > > identical signatures. But if we want 100% correctness, > > > we may need file path in which case option (a) > > > is preferable. > > > > As Alexei mentioned in previous email, for such a extreme case, > > if user is willing to go through extra step to check dwarf > > to find and match file path, then (b) and (c) should work > > perfectly as well. > > Okay, it looks like this is more complex if the function signature is > the same. In such cases, current BTF dedup will merge these > functions as a single BTF func. In such cases, we could have: > > decl_tag_1 ----> dedup'ed static_func > ^ > | > decl_tag_2 --------- > > For such cases, just passing btf_id of static func to kernel > won't work since the kernel won't be able to know which > decl_tag to be associated with. > > (I did a simple test with vmlinux, it looks we have > issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func > as well since only one of decl_tag survives. > But this is a different issue. > ) > > So if we intend to add decl tag (addr or file_path), we > should not dedup static functions or generally any functions. I did not think functions would be dedup-ed, they are ;-) with the declaration tags in place we could perhaps switch it off, right? or perhaps I can't think of all the cases we need functions dedup for, so maybe the dedup code could check also the associated decl tag when comparing functions jirka ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-14 21:49 ` Jiri Olsa @ 2023-05-15 4:06 ` Yonghong Song 2023-05-15 14:53 ` Alan Maguire 0 siblings, 1 reply; 28+ messages in thread From: Yonghong Song @ 2023-05-15 4:06 UTC (permalink / raw) To: Jiri Olsa Cc: Alexei Starovoitov, Alan Maguire, Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Arnaldo Carvalho de Melo, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Eduard Zingerman, Lorenz Bauer, Timo Beckers On 5/14/23 2:49 PM, Jiri Olsa wrote: > On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote: >> >> >> On 5/12/23 7:59 PM, Yonghong Song wrote: >>> >>> >>> On 5/12/23 2:54 PM, Jiri Olsa wrote: >>>> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote: >>>>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire >>>>> <alan.maguire@oracle.com> wrote: >>>>>> >>>>>> On 12/05/2023 03:51, Yafang Shao wrote: >>>>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire >>>>>>> <alan.maguire@oracle.com> wrote: >>>>>>>> >>>>>>>> v1.25 of pahole supports filtering out functions >>>>>>>> with multiple inconsistent >>>>>>>> function prototypes or optimized-out parameters from >>>>>>>> the BTF representation. >>>>>>>> These present problems because there is no >>>>>>>> additional info in BTF saying which >>>>>>>> inconsistent prototype matches which function >>>>>>>> instance to help guide attachment, >>>>>>>> and functions with optimized-out parameters can lead >>>>>>>> to incorrect assumptions >>>>>>>> about register contents. >>>>>>>> >>>>>>>> So for now, filter out such functions while adding >>>>>>>> BTF representations for >>>>>>>> functions that have "."-suffixes (foo.isra.0) but >>>>>>>> not optimized-out parameters. >>>>>>>> This patch assumes that below linked changes land in >>>>>>>> pahole for v1.25. >>>>>>>> >>>>>>>> Issues with pahole filtering being too aggressive in >>>>>>>> removing functions >>>>>>>> appear to be resolved now, but CI and further testing will confirm. >>>>>>>> >>>>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >>>>>>>> --- >>>>>>>> scripts/pahole-flags.sh | 3 +++ >>>>>>>> 1 file changed, 3 insertions(+) >>>>>>>> >>>>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh >>>>>>>> index 1f1f1d397c39..728d55190d97 100755 >>>>>>>> --- a/scripts/pahole-flags.sh >>>>>>>> +++ b/scripts/pahole-flags.sh >>>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >>>>>>>> # see PAHOLE_HAS_LANG_EXCLUDE >>>>>>>> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >>>>>>>> fi >>>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then >>>>>>>> + extra_paholeopt="${extra_paholeopt} >>>>>>>> --skip_encoding_btf_inconsistent_proto >>>>>>>> --btf_gen_optimized" >>>>>>>> +fi >>>>>>>> >>>>>>>> echo ${extra_paholeopt} >>>>>>>> -- >>>>>>>> 2.31.1 >>>>>>>> >>>>>>> >>>>>>> That change looks like a workaround to me. >>>>>>> There may be multiple functions that have the same proto, e.g.: >>>>>>> >>>>>>> $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" >>>>>>> kernel/bpf/ net/core/ >>>>>>> kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct >>>>>>> bpf_iter_aux_info *aux) >>>>>>> net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct >>>>>>> bpf_iter_aux_info *aux) >>>>>>> >>>>>>> $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 >>>>>>> bpf_iter_detach_map >>>>>>> [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 >>>>>>> 'aux' type_id=2638 >>>>>>> [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static >>>>>>> >>>>>>> We don't know which one it is in the BTF. >>>>>>> However, I'm not against this change, as it can avoid some issues. >>>>>>> >>>>>> >>>>>> In the above case, the BTF representation is consistent though. >>>>>> That is, if I attach fentry progs to either of these functions >>>>>> based on that BTF representation, nothing will crash. >>>>>> >>>>>> That's ultimately what those changes are about; ensuring >>>>>> consistency in BTF representation, so when a function is in >>>>>> BTF we can know the signature of the function can be safely >>>>>> used by fentry for example. >>>>>> >>>>>> The question of being able to identify functions (as opposed >>>>>> to having a consistent representation) is the next step. >>>>>> Finding a way to link between kallsyms and BTF would allow us to >>>>>> have multiple inconsistent functions in BTF, since we could map >>>>>> from BTF -> kallsyms safely. So two functions called "foo" >>>>>> with different function signatures would be okay, because >>>>>> we'd know which was which in kallsyms and could attach >>>>>> safely. Something like a BTF tag for the function that could >>>>>> clarify that mapping - but just for cases where it would >>>>>> otherwise be ambiguous - is probably the way forward >>>>>> longer term. >>>>>> >>>>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe. >>>>> >>>>> Jiri presented a few ideas during LSFMMBPF. >>>>> >>>>> I feel the best approach is to add a set of addr-s to BTF >>>>> via a special decl_tag. >>>>> We can also consider extending KIND_FUNC. >>>>> The advantage that every BTF func will have one or more addrs >>>>> associated with it and bpf prog loading logic wouldn't need to do >>>>> fragile name comparison between btf and kallsyms. >>>>> pahole can take addrs from dwarf and optionally double check >>>>> with kallsyms. >>>> >>>> Yonghong summed it up in another email discussion, pasting it in here: >>>> >>>> So overall we have three options as kallsyms representation now: >>>> (a) "addr module:foo:dir_a/dir_b/core.c" >>>> (b) "addr module:foo" >>>> (c) "addr module:foo:btf_id" >>>> >>>> option (a): >>>> 'dir_a/dir_b/core.c' needs to be encoded in BTF. >>>> user space either check file path or func signature >>>> to find attach_btf_id and pass to the kernel. >>>> kernel can find file path in BTF and then lookup >>>> kallsyms to find addr. >>>> >>>> option (b): >>>> "addr" needs to be encoded in BTF. >>>> user space checks func signature to find >>>> attach_btf_id and pass to the kernel. >>>> kernel can find addr in BTF and use it. >>>> >>>> option (c): >>>> if user can decide which function to attach, e.g., >>>> through func signature, then no BTF encoding >>>> is necessary. attach_btf_id is passed to the >>>> kernel and search kallsyms to find the matching >>>> btf_id and 'addr' will be available then. >>>> >>>> For option (b) and (c), user space needs to check >>>> func signature to find which btf_id to use. If >>>> same-name static functions having the identical >>>> signatures, then user space would have a hard time >>>> to differentiate. I think it should be very >>>> rare same-name static functions in the kernel will have >>>> identical signatures. But if we want 100% correctness, >>>> we may need file path in which case option (a) >>>> is preferable. >>> >>> As Alexei mentioned in previous email, for such a extreme case, >>> if user is willing to go through extra step to check dwarf >>> to find and match file path, then (b) and (c) should work >>> perfectly as well. >> >> Okay, it looks like this is more complex if the function signature is >> the same. In such cases, current BTF dedup will merge these >> functions as a single BTF func. In such cases, we could have: >> >> decl_tag_1 ----> dedup'ed static_func >> ^ >> | >> decl_tag_2 --------- >> >> For such cases, just passing btf_id of static func to kernel >> won't work since the kernel won't be able to know which >> decl_tag to be associated with. >> >> (I did a simple test with vmlinux, it looks we have >> issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func >> as well since only one of decl_tag survives. >> But this is a different issue. >> ) >> >> So if we intend to add decl tag (addr or file_path), we >> should not dedup static functions or generally any functions. > > I did not think functions would be dedup-ed, they are ;-) with the > declaration tags in place we could perhaps switch it off, right? That is my hope too. If with decl tag func won't be dedup'ed, then we should be okay. In the kernel, based on attach_btf_id, through btf, kernel can find the corresponding decl tag (file path or addr) or through attach_btf_id itself if the btf id is encoded in kallsym entries. > > or perhaps I can't think of all the cases we need functions dedup for, > so maybe the dedup code could check also the associated decl tag when > comparing functions > > jirka ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-15 4:06 ` Yonghong Song @ 2023-05-15 14:53 ` Alan Maguire 2023-05-15 19:33 ` Yonghong Song 0 siblings, 1 reply; 28+ messages in thread From: Alan Maguire @ 2023-05-15 14:53 UTC (permalink / raw) To: Yonghong Song, Jiri Olsa Cc: Alexei Starovoitov, Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Arnaldo Carvalho de Melo, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Eduard Zingerman, Lorenz Bauer, Timo Beckers On 15/05/2023 05:06, Yonghong Song wrote: > > > On 5/14/23 2:49 PM, Jiri Olsa wrote: >> On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote: >>> >>> >>> On 5/12/23 7:59 PM, Yonghong Song wrote: >>>> >>>> >>>> On 5/12/23 2:54 PM, Jiri Olsa wrote: >>>>> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote: >>>>>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire >>>>>> <alan.maguire@oracle.com> wrote: >>>>>>> >>>>>>> On 12/05/2023 03:51, Yafang Shao wrote: >>>>>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire >>>>>>>> <alan.maguire@oracle.com> wrote: >>>>>>>>> >>>>>>>>> v1.25 of pahole supports filtering out functions >>>>>>>>> with multiple inconsistent >>>>>>>>> function prototypes or optimized-out parameters from >>>>>>>>> the BTF representation. >>>>>>>>> These present problems because there is no >>>>>>>>> additional info in BTF saying which >>>>>>>>> inconsistent prototype matches which function >>>>>>>>> instance to help guide attachment, >>>>>>>>> and functions with optimized-out parameters can lead >>>>>>>>> to incorrect assumptions >>>>>>>>> about register contents. >>>>>>>>> >>>>>>>>> So for now, filter out such functions while adding >>>>>>>>> BTF representations for >>>>>>>>> functions that have "."-suffixes (foo.isra.0) but >>>>>>>>> not optimized-out parameters. >>>>>>>>> This patch assumes that below linked changes land in >>>>>>>>> pahole for v1.25. >>>>>>>>> >>>>>>>>> Issues with pahole filtering being too aggressive in >>>>>>>>> removing functions >>>>>>>>> appear to be resolved now, but CI and further testing will >>>>>>>>> confirm. >>>>>>>>> >>>>>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >>>>>>>>> --- >>>>>>>>> scripts/pahole-flags.sh | 3 +++ >>>>>>>>> 1 file changed, 3 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh >>>>>>>>> index 1f1f1d397c39..728d55190d97 100755 >>>>>>>>> --- a/scripts/pahole-flags.sh >>>>>>>>> +++ b/scripts/pahole-flags.sh >>>>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >>>>>>>>> # see PAHOLE_HAS_LANG_EXCLUDE >>>>>>>>> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >>>>>>>>> fi >>>>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then >>>>>>>>> + extra_paholeopt="${extra_paholeopt} >>>>>>>>> --skip_encoding_btf_inconsistent_proto >>>>>>>>> --btf_gen_optimized" >>>>>>>>> +fi >>>>>>>>> >>>>>>>>> echo ${extra_paholeopt} >>>>>>>>> -- >>>>>>>>> 2.31.1 >>>>>>>>> >>>>>>>> >>>>>>>> That change looks like a workaround to me. >>>>>>>> There may be multiple functions that have the same proto, e.g.: >>>>>>>> >>>>>>>> $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" >>>>>>>> kernel/bpf/ net/core/ >>>>>>>> kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct >>>>>>>> bpf_iter_aux_info *aux) >>>>>>>> net/core/bpf_sk_storage.c:static void >>>>>>>> bpf_iter_detach_map(struct >>>>>>>> bpf_iter_aux_info *aux) >>>>>>>> >>>>>>>> $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 >>>>>>>> bpf_iter_detach_map >>>>>>>> [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 >>>>>>>> 'aux' type_id=2638 >>>>>>>> [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static >>>>>>>> >>>>>>>> We don't know which one it is in the BTF. >>>>>>>> However, I'm not against this change, as it can avoid some issues. >>>>>>>> >>>>>>> >>>>>>> In the above case, the BTF representation is consistent though. >>>>>>> That is, if I attach fentry progs to either of these functions >>>>>>> based on that BTF representation, nothing will crash. >>>>>>> >>>>>>> That's ultimately what those changes are about; ensuring >>>>>>> consistency in BTF representation, so when a function is in >>>>>>> BTF we can know the signature of the function can be safely >>>>>>> used by fentry for example. >>>>>>> >>>>>>> The question of being able to identify functions (as opposed >>>>>>> to having a consistent representation) is the next step. >>>>>>> Finding a way to link between kallsyms and BTF would allow us to >>>>>>> have multiple inconsistent functions in BTF, since we could map >>>>>>> from BTF -> kallsyms safely. So two functions called "foo" >>>>>>> with different function signatures would be okay, because >>>>>>> we'd know which was which in kallsyms and could attach >>>>>>> safely. Something like a BTF tag for the function that could >>>>>>> clarify that mapping - but just for cases where it would >>>>>>> otherwise be ambiguous - is probably the way forward >>>>>>> longer term. >>>>>>> >>>>>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe. >>>>>> >>>>>> Jiri presented a few ideas during LSFMMBPF. >>>>>> >>>>>> I feel the best approach is to add a set of addr-s to BTF >>>>>> via a special decl_tag. >>>>>> We can also consider extending KIND_FUNC. >>>>>> The advantage that every BTF func will have one or more addrs >>>>>> associated with it and bpf prog loading logic wouldn't need to do >>>>>> fragile name comparison between btf and kallsyms. >>>>>> pahole can take addrs from dwarf and optionally double check >>>>>> with kallsyms. >>>>> >>>>> Yonghong summed it up in another email discussion, pasting it in here: >>>>> >>>>> So overall we have three options as kallsyms representation now: >>>>> (a) "addr module:foo:dir_a/dir_b/core.c" >>>>> (b) "addr module:foo" >>>>> (c) "addr module:foo:btf_id" >>>>> >>>>> option (a): >>>>> 'dir_a/dir_b/core.c' needs to be encoded in BTF. >>>>> user space either check file path or func signature >>>>> to find attach_btf_id and pass to the kernel. >>>>> kernel can find file path in BTF and then lookup >>>>> kallsyms to find addr. >>>>> I like the source-centric nature of this, but the only danger here is we might not get a 1:1 mapping between source file location and address; consider the case of a static inline function in a .h file which doesn't get inlined. It could have multiple addresses associated with the same source. For example: static inline void __list_del_entry(struct list_head *entry) { if (!__list_del_entry_valid(entry)) return; __list_del(entry->prev, entry->next); } $ grep __list_del_entry /proc/kallsyms ffffffff982cc5c0 t __pfx___list_del_entry ffffffff982cc5d0 t __list_del_entry ffffffff985b0860 t __pfx___list_del_entry ffffffff985b0870 t __list_del_entry ffffffff9862d800 t __pfx___list_del_entry ffffffff9862d810 t __list_del_entry ffffffff987d3dd0 t __pfx___list_del_entry ffffffff987d3de0 t __list_del_entry ffffffff987f37a0 T __pfx___list_del_entry_valid ffffffff987f37b0 T __list_del_entry_valid ffffffff989fdd10 t __pfx___list_del_entry ffffffff989fdd20 t __list_del_entry ffffffff99baf08c r __ksymtab___list_del_entry_valid ffffffffc12da2e0 t __list_del_entry [bnep] ffffffffc12da2d0 t __pfx___list_del_entry [bnep] ffffffffc092d6b0 t __list_del_entry [videobuf2_common] ffffffffc092d6a0 t __pfx___list_del_entry [videobuf2_common] >>>>> option (b): >>>>> "addr" needs to be encoded in BTF. >>>>> user space checks func signature to find >>>>> attach_btf_id and pass to the kernel. >>>>> kernel can find addr in BTF and use it. >>>>> This seems like the safest option to me. Ideally we wouldn't need such information for every function - only ones with multiple sites and ambiguous signatures - but the problem is a function could have the same name in a kernel and a module too. So it seems like we're stuck with providing additional info to clarify which signature goes with which function for each static function. >>>>> option (c): >>>>> if user can decide which function to attach, e.g., >>>>> through func signature, then no BTF encoding >>>>> is necessary. attach_btf_id is passed to the >>>>> kernel and search kallsyms to find the matching >>>>> btf_id and 'addr' will be available then. >>>>> >>>>> For option (b) and (c), user space needs to check >>>>> func signature to find which btf_id to use. If >>>>> same-name static functions having the identical >>>>> signatures, then user space would have a hard time >>>>> to differentiate. I think it should be very >>>>> rare same-name static functions in the kernel will have >>>>> identical signatures. But if we want 100% correctness, >>>>> we may need file path in which case option (a) >>>>> is preferable. >>>> >>>> As Alexei mentioned in previous email, for such a extreme case, >>>> if user is willing to go through extra step to check dwarf >>>> to find and match file path, then (b) and (c) should work >>>> perfectly as well. >>> >>> Okay, it looks like this is more complex if the function signature is >>> the same. In such cases, current BTF dedup will merge these >>> functions as a single BTF func. In such cases, we could have: >>> >>> decl_tag_1 ----> dedup'ed static_func >>> ^ >>> | >>> decl_tag_2 --------- >>> >>> For such cases, just passing btf_id of static func to kernel >>> won't work since the kernel won't be able to know which >>> decl_tag to be associated with. >>> >>> (I did a simple test with vmlinux, it looks we have >>> issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func >>> as well since only one of decl_tag survives. >>> But this is a different issue. >>> ) >>> >>> So if we intend to add decl tag (addr or file_path), we >>> should not dedup static functions or generally any functions. >> >> I did not think functions would be dedup-ed, they are ;-) with the >> declaration tags in place we could perhaps switch it off, right? > > That is my hope too. If with decl tag func won't be dedup'ed, > then we should be okay. In the kernel, based on attach_btf_id, > through btf, kernel can find the corresponding decl tag (file path > or addr) or through attach_btf_id itself if the btf id is > encoded in kallsym entries. > >> >> or perhaps I can't think of all the cases we need functions dedup for, >> so maybe the dedup code could check also the associated decl tag when >> comparing functions >> Would using the BTF decl tag id instead of the function BTF id to guide attachment be an option? That way if multiple functions shared the same signature, we could still get the info to figure out which to attach to from the decl tag, and we wouldn't need to mess with dedup. I'm probably missing something which makes that unworkable, but just a thought. Thanks! Alan ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-15 14:53 ` Alan Maguire @ 2023-05-15 19:33 ` Yonghong Song 0 siblings, 0 replies; 28+ messages in thread From: Yonghong Song @ 2023-05-15 19:33 UTC (permalink / raw) To: Alan Maguire, Jiri Olsa Cc: Alexei Starovoitov, Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Arnaldo Carvalho de Melo, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Eduard Zingerman, Lorenz Bauer, Timo Beckers On 5/15/23 7:53 AM, Alan Maguire wrote: > On 15/05/2023 05:06, Yonghong Song wrote: >> >> >> On 5/14/23 2:49 PM, Jiri Olsa wrote: >>> On Sun, May 14, 2023 at 10:37:08AM -0700, Yonghong Song wrote: >>>> >>>> >>>> On 5/12/23 7:59 PM, Yonghong Song wrote: >>>>> >>>>> >>>>> On 5/12/23 2:54 PM, Jiri Olsa wrote: >>>>>> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote: >>>>>>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire >>>>>>> <alan.maguire@oracle.com> wrote: >>>>>>>> >>>>>>>> On 12/05/2023 03:51, Yafang Shao wrote: >>>>>>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire >>>>>>>>> <alan.maguire@oracle.com> wrote: >>>>>>>>>> >>>>>>>>>> v1.25 of pahole supports filtering out functions >>>>>>>>>> with multiple inconsistent >>>>>>>>>> function prototypes or optimized-out parameters from >>>>>>>>>> the BTF representation. >>>>>>>>>> These present problems because there is no >>>>>>>>>> additional info in BTF saying which >>>>>>>>>> inconsistent prototype matches which function >>>>>>>>>> instance to help guide attachment, >>>>>>>>>> and functions with optimized-out parameters can lead >>>>>>>>>> to incorrect assumptions >>>>>>>>>> about register contents. >>>>>>>>>> >>>>>>>>>> So for now, filter out such functions while adding >>>>>>>>>> BTF representations for >>>>>>>>>> functions that have "."-suffixes (foo.isra.0) but >>>>>>>>>> not optimized-out parameters. >>>>>>>>>> This patch assumes that below linked changes land in >>>>>>>>>> pahole for v1.25. >>>>>>>>>> >>>>>>>>>> Issues with pahole filtering being too aggressive in >>>>>>>>>> removing functions >>>>>>>>>> appear to be resolved now, but CI and further testing will >>>>>>>>>> confirm. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >>>>>>>>>> --- >>>>>>>>>> scripts/pahole-flags.sh | 3 +++ >>>>>>>>>> 1 file changed, 3 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh >>>>>>>>>> index 1f1f1d397c39..728d55190d97 100755 >>>>>>>>>> --- a/scripts/pahole-flags.sh >>>>>>>>>> +++ b/scripts/pahole-flags.sh >>>>>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >>>>>>>>>> # see PAHOLE_HAS_LANG_EXCLUDE >>>>>>>>>> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >>>>>>>>>> fi >>>>>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then >>>>>>>>>> + extra_paholeopt="${extra_paholeopt} >>>>>>>>>> --skip_encoding_btf_inconsistent_proto >>>>>>>>>> --btf_gen_optimized" >>>>>>>>>> +fi >>>>>>>>>> >>>>>>>>>> echo ${extra_paholeopt} >>>>>>>>>> -- >>>>>>>>>> 2.31.1 >>>>>>>>>> >>>>>>>>> >>>>>>>>> That change looks like a workaround to me. >>>>>>>>> There may be multiple functions that have the same proto, e.g.: >>>>>>>>> >>>>>>>>> $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)" >>>>>>>>> kernel/bpf/ net/core/ >>>>>>>>> kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct >>>>>>>>> bpf_iter_aux_info *aux) >>>>>>>>> net/core/bpf_sk_storage.c:static void >>>>>>>>> bpf_iter_detach_map(struct >>>>>>>>> bpf_iter_aux_info *aux) >>>>>>>>> >>>>>>>>> $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep -B 2 >>>>>>>>> bpf_iter_detach_map >>>>>>>>> [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1 >>>>>>>>> 'aux' type_id=2638 >>>>>>>>> [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static >>>>>>>>> >>>>>>>>> We don't know which one it is in the BTF. >>>>>>>>> However, I'm not against this change, as it can avoid some issues. >>>>>>>>> >>>>>>>> >>>>>>>> In the above case, the BTF representation is consistent though. >>>>>>>> That is, if I attach fentry progs to either of these functions >>>>>>>> based on that BTF representation, nothing will crash. >>>>>>>> >>>>>>>> That's ultimately what those changes are about; ensuring >>>>>>>> consistency in BTF representation, so when a function is in >>>>>>>> BTF we can know the signature of the function can be safely >>>>>>>> used by fentry for example. >>>>>>>> >>>>>>>> The question of being able to identify functions (as opposed >>>>>>>> to having a consistent representation) is the next step. >>>>>>>> Finding a way to link between kallsyms and BTF would allow us to >>>>>>>> have multiple inconsistent functions in BTF, since we could map >>>>>>>> from BTF -> kallsyms safely. So two functions called "foo" >>>>>>>> with different function signatures would be okay, because >>>>>>>> we'd know which was which in kallsyms and could attach >>>>>>>> safely. Something like a BTF tag for the function that could >>>>>>>> clarify that mapping - but just for cases where it would >>>>>>>> otherwise be ambiguous - is probably the way forward >>>>>>>> longer term. >>>>>>>> >>>>>>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe. >>>>>>> >>>>>>> Jiri presented a few ideas during LSFMMBPF. >>>>>>> >>>>>>> I feel the best approach is to add a set of addr-s to BTF >>>>>>> via a special decl_tag. >>>>>>> We can also consider extending KIND_FUNC. >>>>>>> The advantage that every BTF func will have one or more addrs >>>>>>> associated with it and bpf prog loading logic wouldn't need to do >>>>>>> fragile name comparison between btf and kallsyms. >>>>>>> pahole can take addrs from dwarf and optionally double check >>>>>>> with kallsyms. >>>>>> >>>>>> Yonghong summed it up in another email discussion, pasting it in here: >>>>>> >>>>>> So overall we have three options as kallsyms representation now: >>>>>> (a) "addr module:foo:dir_a/dir_b/core.c" >>>>>> (b) "addr module:foo" >>>>>> (c) "addr module:foo:btf_id" >>>>>> >>>>>> option (a): >>>>>> 'dir_a/dir_b/core.c' needs to be encoded in BTF. >>>>>> user space either check file path or func signature >>>>>> to find attach_btf_id and pass to the kernel. >>>>>> kernel can find file path in BTF and then lookup >>>>>> kallsyms to find addr. >>>>>> > > I like the source-centric nature of this, but the only > danger here is we might not get a 1:1 mapping between > source file location and address; consider the case > of a static inline function in a .h file which doesn't > get inlined. It could have multiple addresses associated > with the same source. For example: > > static inline void __list_del_entry(struct list_head *entry) > { > if (!__list_del_entry_valid(entry)) > return; > > __list_del(entry->prev, entry->next); > } > > $ grep __list_del_entry /proc/kallsyms > ffffffff982cc5c0 t __pfx___list_del_entry > ffffffff982cc5d0 t __list_del_entry > ffffffff985b0860 t __pfx___list_del_entry > ffffffff985b0870 t __list_del_entry > ffffffff9862d800 t __pfx___list_del_entry > ffffffff9862d810 t __list_del_entry > ffffffff987d3dd0 t __pfx___list_del_entry > ffffffff987d3de0 t __list_del_entry > ffffffff987f37a0 T __pfx___list_del_entry_valid > ffffffff987f37b0 T __list_del_entry_valid > ffffffff989fdd10 t __pfx___list_del_entry > ffffffff989fdd20 t __list_del_entry > ffffffff99baf08c r __ksymtab___list_del_entry_valid > ffffffffc12da2e0 t __list_del_entry [bnep] > ffffffffc12da2d0 t __pfx___list_del_entry [bnep] > ffffffffc092d6b0 t __list_del_entry [videobuf2_common] > ffffffffc092d6a0 t __pfx___list_del_entry [videobuf2_common] Until now, we are okay with static inline function carried into .o file since all inline functions are marked as notrace (fentry cannot attach to it.) However, now we have https://lore.kernel.org/live-patching/20230502164102.1a51cdb4@gandalf.local.home/T/#u If the patch is merged, then the above inline function in the header survived in .o file indeed a problem. Typically users won't trace inline functions in the header file. But for completeness, agree that file path may not fully reliable. > >>>>>> option (b): >>>>>> "addr" needs to be encoded in BTF. >>>>>> user space checks func signature to find >>>>>> attach_btf_id and pass to the kernel. >>>>>> kernel can find addr in BTF and use it. >>>>>> > > This seems like the safest option to me. Ideally we wouldn't > need such information for every function - only ones with > multiple sites and ambiguous signatures - but the problem > is a function could have the same name in a kernel and > a module too. So it seems like we're stuck with providing > additional info to clarify which signature goes with which > function for each static function. When passing attach_btf_id to kernel, we have attach_btf_obj_fd as well, which should differentiate whether it is kernel or which module it is. > >>>>>> option (c): >>>>>> if user can decide which function to attach, e.g., >>>>>> through func signature, then no BTF encoding >>>>>> is necessary. attach_btf_id is passed to the >>>>>> kernel and search kallsyms to find the matching >>>>>> btf_id and 'addr' will be available then. >>>>>> >>>>>> For option (b) and (c), user space needs to check >>>>>> func signature to find which btf_id to use. If >>>>>> same-name static functions having the identical >>>>>> signatures, then user space would have a hard time >>>>>> to differentiate. I think it should be very >>>>>> rare same-name static functions in the kernel will have >>>>>> identical signatures. But if we want 100% correctness, >>>>>> we may need file path in which case option (a) >>>>>> is preferable. >>>>> >>>>> As Alexei mentioned in previous email, for such a extreme case, >>>>> if user is willing to go through extra step to check dwarf >>>>> to find and match file path, then (b) and (c) should work >>>>> perfectly as well. >>>> >>>> Okay, it looks like this is more complex if the function signature is >>>> the same. In such cases, current BTF dedup will merge these >>>> functions as a single BTF func. In such cases, we could have: >>>> >>>> decl_tag_1 ----> dedup'ed static_func >>>> ^ >>>> | >>>> decl_tag_2 --------- >>>> >>>> For such cases, just passing btf_id of static func to kernel >>>> won't work since the kernel won't be able to know which >>>> decl_tag to be associated with. >>>> >>>> (I did a simple test with vmlinux, it looks we have >>>> issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func >>>> as well since only one of decl_tag survives. >>>> But this is a different issue. >>>> ) >>>> >>>> So if we intend to add decl tag (addr or file_path), we >>>> should not dedup static functions or generally any functions. >>> >>> I did not think functions would be dedup-ed, they are ;-) with the >>> declaration tags in place we could perhaps switch it off, right? >> >> That is my hope too. If with decl tag func won't be dedup'ed, >> then we should be okay. In the kernel, based on attach_btf_id, >> through btf, kernel can find the corresponding decl tag (file path >> or addr) or through attach_btf_id itself if the btf id is >> encoded in kallsym entries. >> >>> >>> or perhaps I can't think of all the cases we need functions dedup for, >>> so maybe the dedup code could check also the associated decl tag when >>> comparing functions >>> > > Would using the BTF decl tag id instead of the function BTF id to > guide attachment be an option? That way if multiple functions shared > the same signature, we could still get the info to figure out which to > attach to from the decl tag, and we wouldn't need to mess with dedup. > I'm probably missing something which makes that unworkable, but just a > thought. Thanks! The issue is due to current bpf syscall UAPI. it passed attach_btf_id and attach_btf_obj_fd to the kernel. If we change UAPI to pass decl_tag_id instead of attach_btf_id to the kernel, we should be okay, but this requires a bpf syscall UAPI change. Maybe this works too. > > Alan ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-05-10 13:02 [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 Alan Maguire 2023-05-10 17:15 ` Jiri Olsa 2023-05-12 2:51 ` Yafang Shao @ 2023-05-12 19:00 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 28+ messages in thread From: patchwork-bot+netdevbpf @ 2023-05-12 19:00 UTC (permalink / raw) To: Alan Maguire Cc: ast, daniel, andrii, acme, jolsa, laoar.shao, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, bpf Hello: This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Wed, 10 May 2023 14:02:41 +0100 you wrote: > v1.25 of pahole supports filtering out functions with multiple inconsistent > function prototypes or optimized-out parameters from the BTF representation. > These present problems because there is no additional info in BTF saying which > inconsistent prototype matches which function instance to help guide attachment, > and functions with optimized-out parameters can lead to incorrect assumptions > about register contents. > > [...] Here is the summary with links: - [bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 https://git.kernel.org/bpf/bpf-next/c/7b99f75942da You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25
@ 2023-02-09 13:28 Alan Maguire
2023-02-09 15:08 ` Jiri Olsa
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Alan Maguire @ 2023-02-09 13:28 UTC (permalink / raw)
To: ast, daniel
Cc: andrii, martin.lau, song, yhs, john.fastabend, kpsingh, sdf,
haoluo, jolsa, acme, bpf, Alan Maguire
v1.25 of pahole supports filtering out functions with multiple
inconsistent function prototypes or optimized-out parameters
from the BTF representation. These present problems because
there is no additional info in BTF saying which inconsistent
prototype matches which function instance to help guide
attachment, and functions with optimized-out parameters can
lead to incorrect assumptions about register contents.
So for now, filter out such functions while adding BTF
representations for functions that have "."-suffixes
(foo.isra.0) but not optimized-out parameters.
This patch assumes changes in [1] land and pahole is bumped
to v1.25.
[1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
scripts/pahole-flags.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
index 1f1f1d3..728d551 100755
--- a/scripts/pahole-flags.sh
+++ b/scripts/pahole-flags.sh
@@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
# see PAHOLE_HAS_LANG_EXCLUDE
extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
fi
+if [ "${pahole_ver}" -ge "125" ]; then
+ extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
+fi
echo ${extra_paholeopt}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-09 13:28 [PATCH bpf-next] bpf: add " Alan Maguire @ 2023-02-09 15:08 ` Jiri Olsa 2023-02-13 17:00 ` patchwork-bot+netdevbpf 2023-02-14 3:12 ` Alexei Starovoitov 2 siblings, 0 replies; 28+ messages in thread From: Jiri Olsa @ 2023-02-09 15:08 UTC (permalink / raw) To: Alan Maguire Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, acme, bpf On Thu, Feb 09, 2023 at 01:28:51PM +0000, Alan Maguire wrote: > v1.25 of pahole supports filtering out functions with multiple > inconsistent function prototypes or optimized-out parameters > from the BTF representation. These present problems because > there is no additional info in BTF saying which inconsistent > prototype matches which function instance to help guide > attachment, and functions with optimized-out parameters can > lead to incorrect assumptions about register contents. > > So for now, filter out such functions while adding BTF > representations for functions that have "."-suffixes > (foo.isra.0) but not optimized-out parameters. > > This patch assumes changes in [1] land and pahole is bumped > to v1.25. > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/ > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> Acked-by: Jiri Olsa <jolsa@kernel.org> jirka > > --- > scripts/pahole-flags.sh | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > index 1f1f1d3..728d551 100755 > --- a/scripts/pahole-flags.sh > +++ b/scripts/pahole-flags.sh > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > # see PAHOLE_HAS_LANG_EXCLUDE > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > fi > +if [ "${pahole_ver}" -ge "125" ]; then > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > +fi > > echo ${extra_paholeopt} > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-09 13:28 [PATCH bpf-next] bpf: add " Alan Maguire 2023-02-09 15:08 ` Jiri Olsa @ 2023-02-13 17:00 ` patchwork-bot+netdevbpf 2023-02-14 3:12 ` Alexei Starovoitov 2 siblings, 0 replies; 28+ messages in thread From: patchwork-bot+netdevbpf @ 2023-02-13 17:00 UTC (permalink / raw) To: Alan Maguire Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa, acme, bpf Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Thu, 9 Feb 2023 13:28:51 +0000 you wrote: > v1.25 of pahole supports filtering out functions with multiple > inconsistent function prototypes or optimized-out parameters > from the BTF representation. These present problems because > there is no additional info in BTF saying which inconsistent > prototype matches which function instance to help guide > attachment, and functions with optimized-out parameters can > lead to incorrect assumptions about register contents. > > [...] Here is the summary with links: - [bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 https://git.kernel.org/bpf/bpf-next/c/0243d3dfe274 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-09 13:28 [PATCH bpf-next] bpf: add " Alan Maguire 2023-02-09 15:08 ` Jiri Olsa 2023-02-13 17:00 ` patchwork-bot+netdevbpf @ 2023-02-14 3:12 ` Alexei Starovoitov 2023-02-14 6:09 ` Alexei Starovoitov 2023-02-14 12:27 ` Jiri Olsa 2 siblings, 2 replies; 28+ messages in thread From: Alexei Starovoitov @ 2023-02-14 3:12 UTC (permalink / raw) To: Alan Maguire Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Arnaldo Carvalho de Melo, bpf, Martin KaFai Lau On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > v1.25 of pahole supports filtering out functions with multiple > inconsistent function prototypes or optimized-out parameters > from the BTF representation. These present problems because > there is no additional info in BTF saying which inconsistent > prototype matches which function instance to help guide > attachment, and functions with optimized-out parameters can > lead to incorrect assumptions about register contents. > > So for now, filter out such functions while adding BTF > representations for functions that have "."-suffixes > (foo.isra.0) but not optimized-out parameters. > > This patch assumes changes in [1] land and pahole is bumped > to v1.25. > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/ > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > --- > scripts/pahole-flags.sh | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > index 1f1f1d3..728d551 100755 > --- a/scripts/pahole-flags.sh > +++ b/scripts/pahole-flags.sh > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > # see PAHOLE_HAS_LANG_EXCLUDE > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > fi > +if [ "${pahole_ver}" -ge "125" ]; then > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > +fi We landed this too soon. #229 tracing_struct:FAIL is failing now. since bpf_testmod.ko is missing a bunch of functions though they're global. I've tried a bunch of different flags and attributes, but none of them helped. The only thing that works is: diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c index 46500636d8cd..5fd0f75d5d20 100644 --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 { long b; }; +__attribute__((optimize("-O0"))) noinline int bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int b, int c) { We cannot do: --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile @@ -10,7 +10,7 @@ endif MODULES = bpf_testmod.ko obj-m += bpf_testmod.o -CFLAGS_bpf_testmod.o = -I$(src) +CFLAGS_bpf_testmod.o = -I$(src) -O0 The build fails due to asm stuff. Maybe we should make scripts/pahole-flags.sh selective and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? Thoughts? ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-14 3:12 ` Alexei Starovoitov @ 2023-02-14 6:09 ` Alexei Starovoitov 2023-03-09 1:53 ` Arnaldo Carvalho de Melo 2023-02-14 12:27 ` Jiri Olsa 1 sibling, 1 reply; 28+ messages in thread From: Alexei Starovoitov @ 2023-02-14 6:09 UTC (permalink / raw) To: Alan Maguire Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Arnaldo Carvalho de Melo, bpf, Martin KaFai Lau On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > v1.25 of pahole supports filtering out functions with multiple > > inconsistent function prototypes or optimized-out parameters > > from the BTF representation. These present problems because > > there is no additional info in BTF saying which inconsistent > > prototype matches which function instance to help guide > > attachment, and functions with optimized-out parameters can > > lead to incorrect assumptions about register contents. > > > > So for now, filter out such functions while adding BTF > > representations for functions that have "."-suffixes > > (foo.isra.0) but not optimized-out parameters. > > > > This patch assumes changes in [1] land and pahole is bumped > > to v1.25. > > > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/ > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > > > --- > > scripts/pahole-flags.sh | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > > index 1f1f1d3..728d551 100755 > > --- a/scripts/pahole-flags.sh > > +++ b/scripts/pahole-flags.sh > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > # see PAHOLE_HAS_LANG_EXCLUDE > > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > fi > > +if [ "${pahole_ver}" -ge "125" ]; then > > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > +fi > > We landed this too soon. > #229 tracing_struct:FAIL > is failing now. > since bpf_testmod.ko is missing a bunch of functions though they're global. > > I've tried a bunch of different flags and attributes, but none of them > helped. > The only thing that works is: > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > index 46500636d8cd..5fd0f75d5d20 100644 > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 { > long b; > }; > > +__attribute__((optimize("-O0"))) > noinline int > bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int > b, int c) { > > We cannot do: > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile > @@ -10,7 +10,7 @@ endif > MODULES = bpf_testmod.ko > > obj-m += bpf_testmod.o > -CFLAGS_bpf_testmod.o = -I$(src) > +CFLAGS_bpf_testmod.o = -I$(src) -O0 > > The build fails due to asm stuff. > > Maybe we should make scripts/pahole-flags.sh selective > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? > > Thoughts? It's even worse with clang compiled kernel: WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid WARN: resolve_btfids: unresolved symbol dctcp_update_alpha WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_static_unused_arg WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref so I reverted this commit for now. Looks like pahole with skip_encoding_btf_inconsistent_proto needs to be more accurate. It's way too aggressive removing valid functions. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-14 6:09 ` Alexei Starovoitov @ 2023-03-09 1:53 ` Arnaldo Carvalho de Melo 2023-03-09 8:16 ` Jiri Olsa 0 siblings, 1 reply; 28+ messages in thread From: Arnaldo Carvalho de Melo @ 2023-03-09 1:53 UTC (permalink / raw) To: Alexei Starovoitov Cc: Alan Maguire, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, Martin KaFai Lau Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu: > On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > +++ b/scripts/pahole-flags.sh > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > > # see PAHOLE_HAS_LANG_EXCLUDE > > > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > > fi > > > +if [ "${pahole_ver}" -ge "125" ]; then > > > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > > +fi > > > > We landed this too soon. > > #229 tracing_struct:FAIL > > is failing now. > > since bpf_testmod.ko is missing a bunch of functions though they're global. > > > > I've tried a bunch of different flags and attributes, but none of them > > helped. > > The only thing that works is: > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > index 46500636d8cd..5fd0f75d5d20 100644 > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 { > > long b; > > }; > > > > +__attribute__((optimize("-O0"))) > > noinline int > > bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int > > b, int c) { > > > > We cannot do: > > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile > > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile > > @@ -10,7 +10,7 @@ endif > > MODULES = bpf_testmod.ko > > > > obj-m += bpf_testmod.o > > -CFLAGS_bpf_testmod.o = -I$(src) > > +CFLAGS_bpf_testmod.o = -I$(src) -O0 > > > > The build fails due to asm stuff. > > > > Maybe we should make scripts/pahole-flags.sh selective > > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? > > > > Thoughts? > > It's even worse with clang compiled kernel: I tested what is now in the master branch with both gcc and clang, on fedora:37, Alan also tested it, Jiri, it would be great if you could check if reverting the revert works for you as well. Thanks, - Arnaldo > WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid > WARN: resolve_btfids: unresolved symbol dctcp_update_alpha > WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash > WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get > WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero > WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast > WARN: resolve_btfids: unresolved symbol > bpf_kfunc_call_test_static_unused_arg > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref > > so I reverted this commit for now. > Looks like pahole with skip_encoding_btf_inconsistent_proto needs > to be more accurate. > It's way too aggressive removing valid functions. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-03-09 1:53 ` Arnaldo Carvalho de Melo @ 2023-03-09 8:16 ` Jiri Olsa 2023-03-09 10:11 ` Jiri Olsa 0 siblings, 1 reply; 28+ messages in thread From: Jiri Olsa @ 2023-03-09 8:16 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Alexei Starovoitov, Alan Maguire, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Martin KaFai Lau On Wed, Mar 08, 2023 at 10:53:51PM -0300, Arnaldo Carvalho de Melo wrote: > Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu: > > On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > +++ b/scripts/pahole-flags.sh > > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > > > # see PAHOLE_HAS_LANG_EXCLUDE > > > > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > > > fi > > > > +if [ "${pahole_ver}" -ge "125" ]; then > > > > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > > > +fi > > > > > > We landed this too soon. > > > #229 tracing_struct:FAIL > > > is failing now. > > > since bpf_testmod.ko is missing a bunch of functions though they're global. > > > > > > I've tried a bunch of different flags and attributes, but none of them > > > helped. > > > The only thing that works is: > > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > > index 46500636d8cd..5fd0f75d5d20 100644 > > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 { > > > long b; > > > }; > > > > > > +__attribute__((optimize("-O0"))) > > > noinline int > > > bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int > > > b, int c) { > > > > > > We cannot do: > > > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile > > > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile > > > @@ -10,7 +10,7 @@ endif > > > MODULES = bpf_testmod.ko > > > > > > obj-m += bpf_testmod.o > > > -CFLAGS_bpf_testmod.o = -I$(src) > > > +CFLAGS_bpf_testmod.o = -I$(src) -O0 > > > > > > The build fails due to asm stuff. > > > > > > Maybe we should make scripts/pahole-flags.sh selective > > > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? > > > > > > Thoughts? > > > > It's even worse with clang compiled kernel: > > I tested what is now in the master branch with both gcc and clang, on > fedora:37, Alan also tested it, Jiri, it would be great if you could > check if reverting the revert works for you as well. ok, will check your master branch jirka > > Thanks, > > - Arnaldo > > > WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid > > WARN: resolve_btfids: unresolved symbol dctcp_update_alpha > > WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid > > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp > > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash > > WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get > > WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero > > WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast > > WARN: resolve_btfids: unresolved symbol > > bpf_kfunc_call_test_static_unused_arg > > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref > > > > so I reverted this commit for now. > > Looks like pahole with skip_encoding_btf_inconsistent_proto needs > > to be more accurate. > > It's way too aggressive removing valid functions. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-03-09 8:16 ` Jiri Olsa @ 2023-03-09 10:11 ` Jiri Olsa 2023-03-09 12:26 ` Arnaldo Carvalho de Melo 2023-03-09 14:58 ` Alan Maguire 0 siblings, 2 replies; 28+ messages in thread From: Jiri Olsa @ 2023-03-09 10:11 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Alexei Starovoitov, Alan Maguire, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Martin KaFai Lau On Thu, Mar 09, 2023 at 09:16:53AM +0100, Jiri Olsa wrote: > On Wed, Mar 08, 2023 at 10:53:51PM -0300, Arnaldo Carvalho de Melo wrote: > > Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu: > > > On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > > +++ b/scripts/pahole-flags.sh > > > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > > > > # see PAHOLE_HAS_LANG_EXCLUDE > > > > > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > > > > fi > > > > > +if [ "${pahole_ver}" -ge "125" ]; then > > > > > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > > > > +fi > > > > > > > > We landed this too soon. > > > > #229 tracing_struct:FAIL > > > > is failing now. > > > > since bpf_testmod.ko is missing a bunch of functions though they're global. > > > > > > > > I've tried a bunch of different flags and attributes, but none of them > > > > helped. > > > > The only thing that works is: > > > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > > > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > > > index 46500636d8cd..5fd0f75d5d20 100644 > > > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > > > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 { > > > > long b; > > > > }; > > > > > > > > +__attribute__((optimize("-O0"))) > > > > noinline int > > > > bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int > > > > b, int c) { > > > > > > > > We cannot do: > > > > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile > > > > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile > > > > @@ -10,7 +10,7 @@ endif > > > > MODULES = bpf_testmod.ko > > > > > > > > obj-m += bpf_testmod.o > > > > -CFLAGS_bpf_testmod.o = -I$(src) > > > > +CFLAGS_bpf_testmod.o = -I$(src) -O0 > > > > > > > > The build fails due to asm stuff. > > > > > > > > Maybe we should make scripts/pahole-flags.sh selective > > > > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? > > > > > > > > Thoughts? > > > > > > It's even worse with clang compiled kernel: > > > > I tested what is now in the master branch with both gcc and clang, on > > fedora:37, Alan also tested it, Jiri, it would be great if you could > > check if reverting the revert works for you as well. > > ok, will check your master branch looks good.. got no duplicates and passing bpf tests for both gcc and clang setups jirka ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-03-09 10:11 ` Jiri Olsa @ 2023-03-09 12:26 ` Arnaldo Carvalho de Melo 2023-03-09 14:58 ` Alan Maguire 1 sibling, 0 replies; 28+ messages in thread From: Arnaldo Carvalho de Melo @ 2023-03-09 12:26 UTC (permalink / raw) To: Alexei Starovoitov Cc: Jiri Olsa, Alan Maguire, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Martin KaFai Lau Em Thu, Mar 09, 2023 at 11:11:27AM +0100, Jiri Olsa escreveu: > On Thu, Mar 09, 2023 at 09:16:53AM +0100, Jiri Olsa wrote: > > On Wed, Mar 08, 2023 at 10:53:51PM -0300, Arnaldo Carvalho de Melo wrote: > > > Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu: > > > > On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > > Maybe we should make scripts/pahole-flags.sh selective > > > > > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? > > > > > Thoughts? > > > > It's even worse with clang compiled kernel: > > > I tested what is now in the master branch with both gcc and clang, on > > > fedora:37, Alan also tested it, Jiri, it would be great if you could > > > check if reverting the revert works for you as well. > > ok, will check your master branch > looks good.. got no duplicates and passing bpf tests for both > gcc and clang setups Thanks for testing! Alexei, since you hit those problems, please consider redoing those tests in your environment so that we triple check all this. Thanks, - Arnaldo ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-03-09 10:11 ` Jiri Olsa 2023-03-09 12:26 ` Arnaldo Carvalho de Melo @ 2023-03-09 14:58 ` Alan Maguire 1 sibling, 0 replies; 28+ messages in thread From: Alan Maguire @ 2023-03-09 14:58 UTC (permalink / raw) To: Jiri Olsa, Arnaldo Carvalho de Melo Cc: Alexei Starovoitov, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Martin KaFai Lau On 09/03/2023 10:11, Jiri Olsa wrote: > On Thu, Mar 09, 2023 at 09:16:53AM +0100, Jiri Olsa wrote: >> On Wed, Mar 08, 2023 at 10:53:51PM -0300, Arnaldo Carvalho de Melo wrote: >>> Em Mon, Feb 13, 2023 at 10:09:21PM -0800, Alexei Starovoitov escreveu: >>>> On Mon, Feb 13, 2023 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: >>>>> On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: >>>>>> +++ b/scripts/pahole-flags.sh >>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >>>>>> # see PAHOLE_HAS_LANG_EXCLUDE >>>>>> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >>>>>> fi >>>>>> +if [ "${pahole_ver}" -ge "125" ]; then >>>>>> + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" >>>>>> +fi >>>>> >>>>> We landed this too soon. >>>>> #229 tracing_struct:FAIL >>>>> is failing now. >>>>> since bpf_testmod.ko is missing a bunch of functions though they're global. >>>>> >>>>> I've tried a bunch of different flags and attributes, but none of them >>>>> helped. >>>>> The only thing that works is: >>>>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c >>>>> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c >>>>> index 46500636d8cd..5fd0f75d5d20 100644 >>>>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c >>>>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c >>>>> @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 { >>>>> long b; >>>>> }; >>>>> >>>>> +__attribute__((optimize("-O0"))) >>>>> noinline int >>>>> bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int >>>>> b, int c) { >>>>> >>>>> We cannot do: >>>>> --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile >>>>> +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile >>>>> @@ -10,7 +10,7 @@ endif >>>>> MODULES = bpf_testmod.ko >>>>> >>>>> obj-m += bpf_testmod.o >>>>> -CFLAGS_bpf_testmod.o = -I$(src) >>>>> +CFLAGS_bpf_testmod.o = -I$(src) -O0 >>>>> >>>>> The build fails due to asm stuff. >>>>> >>>>> Maybe we should make scripts/pahole-flags.sh selective >>>>> and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? >>>>> >>>>> Thoughts? >>>> >>>> It's even worse with clang compiled kernel: >>> >>> I tested what is now in the master branch with both gcc and clang, on >>> fedora:37, Alan also tested it, Jiri, it would be great if you could >>> check if reverting the revert works for you as well. >> >> ok, will check your master branch > > looks good.. got no duplicates and passing bpf tests for both > gcc and clang setups > thanks for re-testing! I just did the same for latest bpf-next on x86_64/aarch64; all looks good. Alan ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-14 3:12 ` Alexei Starovoitov 2023-02-14 6:09 ` Alexei Starovoitov @ 2023-02-14 12:27 ` Jiri Olsa 2023-02-14 16:17 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 28+ messages in thread From: Jiri Olsa @ 2023-02-14 12:27 UTC (permalink / raw) To: Alexei Starovoitov Cc: Alan Maguire, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Arnaldo Carvalho de Melo, bpf, Martin KaFai Lau On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote: > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > v1.25 of pahole supports filtering out functions with multiple > > inconsistent function prototypes or optimized-out parameters > > from the BTF representation. These present problems because > > there is no additional info in BTF saying which inconsistent > > prototype matches which function instance to help guide > > attachment, and functions with optimized-out parameters can > > lead to incorrect assumptions about register contents. > > > > So for now, filter out such functions while adding BTF > > representations for functions that have "."-suffixes > > (foo.isra.0) but not optimized-out parameters. > > > > This patch assumes changes in [1] land and pahole is bumped > > to v1.25. > > > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/ > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > > > --- > > scripts/pahole-flags.sh | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > > index 1f1f1d3..728d551 100755 > > --- a/scripts/pahole-flags.sh > > +++ b/scripts/pahole-flags.sh > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > # see PAHOLE_HAS_LANG_EXCLUDE > > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > fi > > +if [ "${pahole_ver}" -ge "125" ]; then > > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > +fi > > We landed this too soon. > #229 tracing_struct:FAIL > is failing now. > since bpf_testmod.ko is missing a bunch of functions though they're global. > hum, didn't see this one failing.. I'll try that again jirka > I've tried a bunch of different flags and attributes, but none of them > helped. > The only thing that works is: > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > index 46500636d8cd..5fd0f75d5d20 100644 > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 { > long b; > }; > > +__attribute__((optimize("-O0"))) > noinline int > bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int > b, int c) { > > We cannot do: > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile > @@ -10,7 +10,7 @@ endif > MODULES = bpf_testmod.ko > > obj-m += bpf_testmod.o > -CFLAGS_bpf_testmod.o = -I$(src) > +CFLAGS_bpf_testmod.o = -I$(src) -O0 > > The build fails due to asm stuff. > > Maybe we should make scripts/pahole-flags.sh selective > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? > > Thoughts? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-14 12:27 ` Jiri Olsa @ 2023-02-14 16:17 ` Arnaldo Carvalho de Melo 2023-02-14 22:12 ` Jiri Olsa 0 siblings, 1 reply; 28+ messages in thread From: Arnaldo Carvalho de Melo @ 2023-02-14 16:17 UTC (permalink / raw) To: Jiri Olsa Cc: Alexei Starovoitov, Alan Maguire, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Martin KaFai Lau Em Tue, Feb 14, 2023 at 01:27:43PM +0100, Jiri Olsa escreveu: > On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote: > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > > > v1.25 of pahole supports filtering out functions with multiple > > > inconsistent function prototypes or optimized-out parameters > > > from the BTF representation. These present problems because > > > there is no additional info in BTF saying which inconsistent > > > prototype matches which function instance to help guide > > > attachment, and functions with optimized-out parameters can > > > lead to incorrect assumptions about register contents. > > > > > > So for now, filter out such functions while adding BTF > > > representations for functions that have "."-suffixes > > > (foo.isra.0) but not optimized-out parameters. > > > > > > This patch assumes changes in [1] land and pahole is bumped > > > to v1.25. > > > > > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/ > > > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > > > > > --- > > > scripts/pahole-flags.sh | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > > > index 1f1f1d3..728d551 100755 > > > --- a/scripts/pahole-flags.sh > > > +++ b/scripts/pahole-flags.sh > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > > # see PAHOLE_HAS_LANG_EXCLUDE > > > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > > fi > > > +if [ "${pahole_ver}" -ge "125" ]; then > > > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > > +fi > > > > We landed this too soon. > > #229 tracing_struct:FAIL > > is failing now. > > since bpf_testmod.ko is missing a bunch of functions though they're global. > > > > hum, didn't see this one failing.. I'll try that again /me too, redoing tests her, with gcc and clang, running selftests on a system booted with a kernel built with pahole 1.25, etc. - Arnaldo > jirka > > > I've tried a bunch of different flags and attributes, but none of them > > helped. > > The only thing that works is: > > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > index 46500636d8cd..5fd0f75d5d20 100644 > > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c > > @@ -28,6 +28,7 @@ struct bpf_testmod_struct_arg_2 { > > long b; > > }; > > > > +__attribute__((optimize("-O0"))) > > noinline int > > bpf_testmod_test_struct_arg_1(struct bpf_testmod_struct_arg_2 a, int > > b, int c) { > > > > We cannot do: > > --- a/tools/testing/selftests/bpf/bpf_testmod/Makefile > > +++ b/tools/testing/selftests/bpf/bpf_testmod/Makefile > > @@ -10,7 +10,7 @@ endif > > MODULES = bpf_testmod.ko > > > > obj-m += bpf_testmod.o > > -CFLAGS_bpf_testmod.o = -I$(src) > > +CFLAGS_bpf_testmod.o = -I$(src) -O0 > > > > The build fails due to asm stuff. > > > > Maybe we should make scripts/pahole-flags.sh selective > > and don't apply skip_encoding_btf_inconsiste to bpf_testmod ? > > > > Thoughts? -- - Arnaldo ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-14 16:17 ` Arnaldo Carvalho de Melo @ 2023-02-14 22:12 ` Jiri Olsa 2023-02-17 13:45 ` Alan Maguire 0 siblings, 1 reply; 28+ messages in thread From: Jiri Olsa @ 2023-02-14 22:12 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, Alexei Starovoitov, Alan Maguire, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Martin KaFai Lau On Tue, Feb 14, 2023 at 01:17:56PM -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Feb 14, 2023 at 01:27:43PM +0100, Jiri Olsa escreveu: > > On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote: > > > On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > > > > > v1.25 of pahole supports filtering out functions with multiple > > > > inconsistent function prototypes or optimized-out parameters > > > > from the BTF representation. These present problems because > > > > there is no additional info in BTF saying which inconsistent > > > > prototype matches which function instance to help guide > > > > attachment, and functions with optimized-out parameters can > > > > lead to incorrect assumptions about register contents. > > > > > > > > So for now, filter out such functions while adding BTF > > > > representations for functions that have "."-suffixes > > > > (foo.isra.0) but not optimized-out parameters. > > > > > > > > This patch assumes changes in [1] land and pahole is bumped > > > > to v1.25. > > > > > > > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/ > > > > > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > > > Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > > > > > > > --- > > > > scripts/pahole-flags.sh | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > > > > index 1f1f1d3..728d551 100755 > > > > --- a/scripts/pahole-flags.sh > > > > +++ b/scripts/pahole-flags.sh > > > > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > > > > # see PAHOLE_HAS_LANG_EXCLUDE > > > > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > > > > fi > > > > +if [ "${pahole_ver}" -ge "125" ]; then > > > > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > > > > +fi > > > > > > We landed this too soon. > > > #229 tracing_struct:FAIL > > > is failing now. > > > since bpf_testmod.ko is missing a bunch of functions though they're global. > > > > > > > hum, didn't see this one failing.. I'll try that again > > /me too, redoing tests her, with gcc and clang, running selftests on a > system booted with a kernel built with pahole 1.25, etc. ok, can't see that with gcc, but reproduced with clang 16 resolve_btfids complains because those functions are not in btf BTFIDS vmlinux WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid WARN: resolve_btfids: unresolved symbol should_failslab WARN: resolve_btfids: unresolved symbol should_fail_alloc_page WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_static_unused_arg WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass_ctx WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass2 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass1 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_pass1 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail2 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail1 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_kptr_get WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail3 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail2 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail1 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_acquire WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test2 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test1 WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb_release WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb1_release WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_int_mem_release NM System.map jirka ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 2023-02-14 22:12 ` Jiri Olsa @ 2023-02-17 13:45 ` Alan Maguire 0 siblings, 0 replies; 28+ messages in thread From: Alan Maguire @ 2023-02-17 13:45 UTC (permalink / raw) To: Jiri Olsa, Arnaldo Carvalho de Melo Cc: Alexei Starovoitov, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, bpf, Martin KaFai Lau On 14/02/2023 22:12, Jiri Olsa wrote: > On Tue, Feb 14, 2023 at 01:17:56PM -0300, Arnaldo Carvalho de Melo wrote: >> Em Tue, Feb 14, 2023 at 01:27:43PM +0100, Jiri Olsa escreveu: >>> On Mon, Feb 13, 2023 at 07:12:33PM -0800, Alexei Starovoitov wrote: >>>> On Thu, Feb 9, 2023 at 5:29 AM Alan Maguire <alan.maguire@oracle.com> wrote: >>>>> >>>>> v1.25 of pahole supports filtering out functions with multiple >>>>> inconsistent function prototypes or optimized-out parameters >>>>> from the BTF representation. These present problems because >>>>> there is no additional info in BTF saying which inconsistent >>>>> prototype matches which function instance to help guide >>>>> attachment, and functions with optimized-out parameters can >>>>> lead to incorrect assumptions about register contents. >>>>> >>>>> So for now, filter out such functions while adding BTF >>>>> representations for functions that have "."-suffixes >>>>> (foo.isra.0) but not optimized-out parameters. >>>>> >>>>> This patch assumes changes in [1] land and pahole is bumped >>>>> to v1.25. >>>>> >>>>> [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/ >>>>> >>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> >>>>> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> >>>>> >>>>> --- >>>>> scripts/pahole-flags.sh | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh >>>>> index 1f1f1d3..728d551 100755 >>>>> --- a/scripts/pahole-flags.sh >>>>> +++ b/scripts/pahole-flags.sh >>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then >>>>> # see PAHOLE_HAS_LANG_EXCLUDE >>>>> extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" >>>>> fi >>>>> +if [ "${pahole_ver}" -ge "125" ]; then >>>>> + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" >>>>> +fi >>>> >>>> We landed this too soon. >>>> #229 tracing_struct:FAIL >>>> is failing now. >>>> since bpf_testmod.ko is missing a bunch of functions though they're global. >>>> >>> >>> hum, didn't see this one failing.. I'll try that again >> >> /me too, redoing tests her, with gcc and clang, running selftests on a >> system booted with a kernel built with pahole 1.25, etc. > > ok, can't see that with gcc, but reproduced with clang 16 > > resolve_btfids complains because those functions are not in btf > > BTFIDS vmlinux > WARN: resolve_btfids: unresolved symbol tcp_reno_cong_avoid > WARN: resolve_btfids: unresolved symbol should_failslab > WARN: resolve_btfids: unresolved symbol should_fail_alloc_page > WARN: resolve_btfids: unresolved symbol cubictcp_cong_avoid > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_timestamp > WARN: resolve_btfids: unresolved symbol bpf_xdp_metadata_rx_hash > WARN: resolve_btfids: unresolved symbol bpf_task_kptr_get > WARN: resolve_btfids: unresolved symbol bpf_task_acquire_not_zero > WARN: resolve_btfids: unresolved symbol bpf_rdonly_cast > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_static_unused_arg > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_ref > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass_ctx > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass2 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_pass1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_pass1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail2 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_mem_len_fail1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_kptr_get > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail3 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail2 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_fail1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test_acquire > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test2 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_test1 > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb_release > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_memb1_release > WARN: resolve_btfids: unresolved symbol bpf_kfunc_call_int_mem_release > NM System.map > Jiri kindly provided a clang-generated vmlinux, and I also managed to reproduce issues by building the kernel with clang 17. The first question is if we're detecting optimizations correctly. From an initial look, I _think_ we are, in some cases at least. For tcp_reno_cong_avoid(), the function signature is __bpf_kfunc void tcp_reno_cong_avoid(struct sock *sk, u32 ack, u32 acked) ...but our handling of the DWARF generated spots that the "ack" parameter has no location info; and looking at the source it is never used. Here is the DWARF - note no location info for the second parameter ("ack"): 0x0891dab0: DW_TAG_subprogram DW_AT_low_pc (0xffffffff82031180) DW_AT_high_pc (0xffffffff820311d8) DW_AT_frame_base (DW_OP_reg7 RSP) DW_AT_call_all_calls (true) DW_AT_name ("tcp_reno_cong_avoid") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c") DW_AT_decl_line (446) DW_AT_prototyped (true) DW_AT_external (true) 0x0891dabe: DW_TAG_formal_parameter DW_AT_location (indexed (0x7a) loclist = 0x00f50eb1: [0xffffffff82031185, 0xffffffff8203119e): DW_OP_reg5 RDI [0xffffffff8203119e, 0xffffffff820311cc): DW_OP_reg3 RBX [0xffffffff820311cc, 0xffffffff820311d1): DW_OP_reg5 RDI [0xffffffff820311d1, 0xffffffff820311d2): DW_OP_reg3 RBX [0xffffffff820311d2, 0xffffffff820311d8): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value) DW_AT_name ("sk") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c") DW_AT_decl_line (446) DW_AT_type (0x08902c4d "sock *") 0x0891dac9: DW_TAG_formal_parameter DW_AT_name ("ack") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c") DW_AT_decl_line (446) DW_AT_type (0x08902c3d "u32") 0x0891dad4: DW_TAG_formal_parameter DW_AT_location (indexed (0x7b) loclist = 0x00f50eda: [0xffffffff82031185, 0xffffffff820311bc): DW_OP_reg1 RDX [0xffffffff820311bc, 0xffffffff820311c8): DW_OP_reg0 RAX [0xffffffff820311c8, 0xffffffff820311d1): DW_OP_reg1 RDX) DW_AT_name ("acked") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/ipv4/tcp_cong.c") DW_AT_decl_line (446) DW_AT_type (0x08902c3d "u32") Disassembling we see the following: (gdb) disassemble/s tcp_reno_cong_avoid Dump of assembler code for function tcp_reno_cong_avoid: net/ipv4/tcp_cong.c: 447 { 0xffffffff82031180 <+0>: nopl 0x0(%rax,%rax,1) 0xffffffff82031185 <+5>: push %rbx 0xffffffff82031186 <+6>: mov %rdi,%rbx ./include/net/tcp.h: 1305 if (tp->is_cwnd_limited) 0xffffffff82031189 <+9>: cmpb $0x0,0x95f(%rdi) 0xffffffff82031190 <+16>: mov 0x9e4(%rdi),%eax 0xffffffff82031196 <+22>: mov 0x9e8(%rdi),%esi 0xffffffff8203119c <+28>: js 0xffffffff820311ae <tcp_reno_cong_avoid+46> 1238 return tcp_snd_cwnd(tp) < tp->snd_ssthresh; 0xffffffff8203119e <+30>: cmp %eax,%esi 1306 return true; 1307 1308 /* If in slow start, ensure cwnd grows to twice what was ACKed. */ 1309 if (tcp_in_slow_start(tp)) 0xffffffff820311a0 <+32>: jae 0xffffffff820311d1 <tcp_reno_cong_avoid+81> 1310 return tcp_snd_cwnd(tp) < 2 * tp->max_packets_out; 0xffffffff820311a2 <+34>: mov 0x9b4(%rbx),%ecx 0xffffffff820311a8 <+40>: add %ecx,%ecx 0xffffffff820311aa <+42>: cmp %ecx,%esi net/ipv4/tcp_cong.c: 450 if (!tcp_is_cwnd_limited(sk)) 0xffffffff820311ac <+44>: jae 0xffffffff820311d1 <tcp_reno_cong_avoid+81> ./include/net/tcp.h: 1238 return tcp_snd_cwnd(tp) < tp->snd_ssthresh; 0xffffffff820311ae <+46>: cmp %eax,%esi net/ipv4/tcp_cong.c: 454 if (tcp_in_slow_start(tp)) { 0xffffffff820311b0 <+48>: jae 0xffffffff820311c8 <tcp_reno_cong_avoid+72> 455 acked = tcp_slow_start(tp, acked); 0xffffffff820311b2 <+50>: mov %rbx,%rdi 0xffffffff820311b5 <+53>: mov %edx,%esi 0xffffffff820311b7 <+55>: call 0xffffffff82031080 <tcp_slow_start> --Type <RET> for more, q to quit, c to continue without paging-- 456 if (!acked) 0xffffffff820311bc <+60>: test %eax,%eax 0xffffffff820311be <+62>: je 0xffffffff820311d1 <tcp_reno_cong_avoid+81> 0xffffffff820311c0 <+64>: mov %eax,%edx ./include/net/tcp.h: 1227 return tp->snd_cwnd; 0xffffffff820311c2 <+66>: mov 0x9e8(%rbx),%esi net/ipv4/tcp_cong.c: 460 tcp_cong_avoid_ai(tp, tcp_snd_cwnd(tp), acked); 0xffffffff820311c8 <+72>: mov %rbx,%rdi 0xffffffff820311cb <+75>: pop %rbx 0xffffffff820311cc <+76>: jmp 0xffffffff820310d0 <tcp_cong_avoid_ai> 461 } 0xffffffff820311d1 <+81>: pop %rbx 0xffffffff820311d2 <+82>: cs jmp 0xffffffff8223c240 <__x86_return_thunk> End of assembler dump. From what I can see above - unless I'm misreading it - we see something interesting here. Note that in preparing the call to tcp_cong_avoid_ai(), we get the tcp_snd_cwnd() value into %esi, but nothing needs to be done with %rdx because it's already got the "acked" value in it. Now this is good news, because if we're calling this kfunc - that only uses the first and third parameters - preparing all three will not lead to any nasty surprises (we just wasted a bit of time preparing the second unused parameter). If this held true for all kfuncs it would mean that skipping representing them in BTF due to optimized-out parameters would be the wrong answer. The key thing to figure out is this - if we optimize out a parameter, will the subsequent parameters that are not optimized out still use the registers that they would be expected to if no optimization had happened? So if I optimize out the first parameter say, will the second parameter use the "right" register (%rsi on x86_64)? If that were guaranteed, we could relax the cases where we skip BTF generation to the following: 1. cases where multiple inconsistent function prototypes for the same name exist. 2. cases where a function has multiple instances with different optimization states (optimized out parameter in one CU but not another). This is another instance of 1 really, and shouldn't be an issue for kfuncs. 3. cases where an optimized-out parameter has knock-on effects for the registers used to handle other unoptimized parameters such that assumptions we would make from the function signature are violated for non-optimized out parameters. So the parameter would be flagged as using an unexpected register, and that unexpected case would instead lead to skipping BTF encoding. I can have a go at implementing the above in pahole and seeing how it effects our list of functions. Note though that what's good for kfuncs isn't necessarily good for tracing accuracy; if assumptions I make about parameter presence are violated, I will see strange trace results based upon reading the code, whereas if I am preparing kfunc parameters, a bit of extra work is done preparing an unused parameter, but no harm is done. For the tracing case, function annotations flagging optimized-out parameters via BTF tags could help. However, none of the above will help problematic cases like bpf_xdp_metadata_rx_timestamp() or bpf_xdp_metadata_rx_hash(); again we see missing location info in their DWARF representations 0x07449011: DW_TAG_subprogram DW_AT_low_pc (0xffffffff81ec8c80) DW_AT_high_pc (0xffffffff81ec8c90) DW_AT_frame_base (DW_OP_reg7 RSP) DW_AT_call_all_calls (true) DW_AT_name ("bpf_xdp_metadata_rx_timestamp") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c") DW_AT_decl_line (725) DW_AT_prototyped (true) DW_AT_type (0x0742f1ae "int") DW_AT_external (true) 0x07449023: DW_TAG_formal_parameter DW_AT_name ("ctx") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c") DW_AT_decl_line (725) DW_AT_type (0x0743dff0 "const xdp_md *") 0x0744902d: DW_TAG_formal_parameter DW_AT_name ("timestamp") DW_AT_decl_file ("/home/jolsa/kernel/linux-qemu/net/core/xdp.c") DW_AT_decl_line (725) DW_AT_type (0x0743217a "u64 *") ...due to the function just being a "return -EOPNOTSUPP;": Dump of assembler code for function bpf_xdp_metadata_rx_timestamp: 0xffffffff81ec8c80 <+0>: nopl 0x0(%rax,%rax,1) 0xffffffff81ec8c85 <+5>: mov $0xffffffa1,%eax 0xffffffff81ec8c8a <+10>: cs jmp 0xffffffff8223c240 <__x86_return_thunk> End of assembler dump. __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp) { return -EOPNOTSUPP; } ...and playing around with various attributes here does not seem to help. should_failslab() has a similar story, I suspect because __should_failslab() got optimized out due to CONFIG_FAILSLAB=n and we get (gdb) disassemble/s should_failslab Dump of assembler code for function should_failslab: mm/slab_common.c: 1462 { 0xffffffff81422490 <+0>: nopl 0x0(%rax,%rax,1) 1463 if (__should_failslab(s, gfpflags)) 1464 return -ENOMEM; 1465 return 0; 1466 } 0xffffffff81422495 <+5>: xor %eax,%eax 0xffffffff81422497 <+7>: cs jmp 0xffffffff8223c240 <__x86_return_thunk> End of assembler dump. However, the caller of this function still prepares parameters as if they were going to be used: (gdb) disassemble slab_pre_alloc_hook Dump of assembler code for function slab_pre_alloc_hook: 0xffffffff813bc5b0 <+0>: push %rbp 0xffffffff813bc5b1 <+1>: mov %rsp,%rbp 0xffffffff813bc5b4 <+4>: push %r15 0xffffffff813bc5b6 <+6>: push %r14 0xffffffff813bc5b8 <+8>: push %r13 0xffffffff813bc5ba <+10>: push %r12 0xffffffff813bc5bc <+12>: push %rbx 0xffffffff813bc5bd <+13>: sub $0x20,%rsp 0xffffffff813bc5c1 <+17>: mov %r8d,%r15d 0xffffffff813bc5c4 <+20>: mov %rcx,%r12 0xffffffff813bc5c7 <+23>: mov %rdx,-0x48(%rbp) 0xffffffff813bc5cb <+27>: mov %rsi,%rbx 0xffffffff813bc5ce <+30>: mov %rdi,%r13 0xffffffff813bc5d1 <+33>: and 0x219ff00(%rip),%r15d # 0xffffffff8355c4d8 <gfp_allowed_mask> 0xffffffff813bc5d8 <+40>: test $0x400,%r15d 0xffffffff813bc5df <+47>: je 0xffffffff813bc5e6 <slab_pre_alloc_hook+54> 0xffffffff813bc5e1 <+49>: callq 0xffffffff81d27b68 <__SCT__might_resched> 0xffffffff813bc5e6 <+54>: mov %r13,%rdi 0xffffffff813bc5e9 <+57>: mov %r15d,%esi 0xffffffff813bc5ec <+60>: callq 0xffffffff81340a70 <should_failslab> ...so the function is still being called with the right register values. This points to a conceptual flaw in the approach I was taking - we cannot equate register _state_ on entry to a function with register _use_ by that function. So from a DWARF perspective, the fact that there is no location information does not necessarily tell us the function is not _called_ with that parameter; rather it tells us it is not used within the body of the function. This all combines to suggest that the only time we should be definitive in rejecting a function for BTF encoding due to optimizations is when they interfere with the expectations we have about _used_ parameter->register mappings. So concretely, where the second parameter uses a register other than the one the calling conventions dictate should be used by the second parameter say, or indeed does not use a register at all. It is possible that we could be more definitive by examining DWARF call site info, but there is none for some of the above functions like should_failslab(), so that will not help here as far as I can see. Does this seem reasonable, or am I missing something here? I'm worried we're going to end up playing whack-a-mole with increasingly clever compiler optimizations, so my instinct is that we need to be conservative in choosing when to skip BTF encoding, only doing so when we are certain it will mislead badly. I _think_ there is some justification to that based on the above. What do you think? Alan ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions
@ 2023-02-07 17:14 Alan Maguire
2023-02-08 16:20 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 28+ messages in thread
From: Alan Maguire @ 2023-02-07 17:14 UTC (permalink / raw)
To: acme
Cc: ast, andrii, daniel, eddyz87, haoluo, jolsa, john.fastabend,
kpsingh, sinquersw, martin.lau, songliubraving, sdf, timo, yhs,
bpf, Alan Maguire
At optimization level -O2 or higher in gcc, static functions may be
optimized such that they have suffixes like .isra.0, .constprop.0 etc.
These represent
- constant propagation (.constprop.0);
- interprocedural scalar replacement of aggregates, removal of
unused parameters and replacement of parameters passed by
reference by parameters passed by value (.isra.0)
See [1] for details.
Currently BTF encoding does not handle such optimized functions
that get renamed with a "." suffix such as ".isra.0", ".constprop.0".
This is safer because such suffixes can often indicate parameters have
been optimized out. This series addresses this by matching a
function to a suffixed version ("foo" matching "foo.isra.0") while
ensuring that the function signature does not contain optimized-out
parameters. Note that if the function is found ("foo") it will
be preferred, only falling back to "foo.isra.0" if lookup of the
function fails. Addition to BTF is skipped if the function has
optimized-out parameters, since the expected function signature
will not match. BTF encoding does not include the "."-suffix to
be consistent with DWARF. In addition, the kernel currently does
not allow a "." suffix in a BTF function name.
A problem with this approach however is that BTF carries out the
encoding process in parallel across multiple CUs, and sometimes
a function has optimized-out parameters in one CU but not others;
we see this for NF_HOOK.constprop.0 for example. So in order to
determine if the function has optimized-out parameters in any
CU, its addition is not carried out until we have processed all
CUs and are about to merge BTF. At this point we know if any
such optimizations have occurred. Patches 1-5 handle the
optimized-out parameter identification and matching "."-suffixed
functions with the original function to facilitate BTF
encoding. This feature can be enabled via the
"--btf_gen_optimized" option.
Patch 6 addresses a related problem - it is entirely possible
for a static function of the same name to exist in different
CUs with different function signatures. Because BTF does not
currently encode any information that would help disambiguate
which BTF function specification matches which static function
(in the case of multiple different function signatures), it is
best to eliminate such functions from BTF for now. The same
mechanism that is used to compare static "."-suffixed functions
is re-used for the static function comparison. A superficial
comparison of number of parameters/parameter names is done to
see if such representations are consistent, and if inconsistent
prototypes are observed, the function is flagged for exclusion
from BTF.
When these methods are combined - the additive encoding of
"."-suffixed functions and the subtractive elimination of
functions with inconsistent parameters - we see an overall
drop in the number of functions in vmlinux BTF, from
51529 to 50246. Skipping inconsistent functions is enabled
via "--skip_encoding_btf_inconsistent_proto".
Changes since v2 [2]
- Arnaldo incorporated some of the suggestions in the v2 thread;
these patches are based on those; the relevant changes are
noted as committer changes.
- Patch 1 is unchanged from v2, but the rest of the patches
have been updated:
- Patch 2 separates out the changes to the struct btf_encoder
that better support later addition of functions.
- Patch 3 then is changed insofar as these changes are no
longer needed for the function addition refactoring.
- Patch 4 has a small change; we need to verify that an
encoder has actually been added to the encoders list
prior to removal
- Patch 5 changed significantly; when attempting to measure
performance the relatively good numbers attained when using
delayed function addition were not reproducible.
Further analysis revealed that the large number of lookups
caused by the presence of the separate function tree was
a major cause of performance degradation in the multi
threaded case. So instead of maintaining a separate tree,
we use the ELF function list which we already need to look
up to match ELF -> DWARF function descriptions to store
the function representation. This has 2 benefits; firstly
as mentioned, we already look up the ELF function so no
additional lookup is required to save the function.
Secondly, the ELF representation is identical for each
encoder, so we can index the same function across multiple
encoder function arrays - this greatly speeds up the
processing of comparing function representations across
encoders. There is still a performance cost in this
approach however; more details are provided in patch 6.
An option specific to adding functions with "." suffixes
is added "--btf_gen_optimized"
- Patch 6 builds on patch 5 in applying the save/merge/add
approach for all functions using the same mechanisms.
In addition the "--skip_encoding_btf_inconsistent_proto"
option is introduced.
- Patches 7/8 document the new options in the pahole manual
page.
Changes since v1 [3]
- Eduard noted that a DW_AT_const_value attribute can signal
an optimized-out parameter, and that the lack of a location
attribute signals optimization; ensure we handle those cases
also (Eduard, patch 1).
- Jiri noted we can have inconsistencies between a static
and non-static function; apply the comparison process to
all functions (Jiri, patch 5)
- segmentation fault was observed when handling functions with
> 10 parameters; needed parameter comparison loop to exit
at BTF_ENCODER_MAX_PARAMETERS (patch 5)
- Kui-Feng Lee pointed out that having a global shared function
tree would lead to a lot of contention; here a per-encoder
tree is used, and once the threads are collected the trees
are merged. Performance numbers are provided in patch 5
(Kui-Feng Lee, patches 4/5)
[1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
[2] https://lore.kernel.org/bpf/1675088985-20300-1-git-send-email-alan.maguire@oracle.com/
[3] https://lore.kernel.org/bpf/1674567931-26458-1-git-send-email-alan.maguire@oracle.com/
Alan Maguire (8):
dwarf_loader: Help spotting functions with optimized-out parameters
btf_encoder: store type_id_off, unspecified type in encoder
btf_encoder: Refactor function addition into dedicated
btf_encoder__add_func
btf_encoder: Rework btf_encoders__*() API to allow traversal of
encoders
btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF
btf_encoder: support delaying function addition to check for function
prototype inconsistencies
dwarves: document --btf_gen_optimized option
dwarves: document --skip_encoding_btf_inconsistent_proto option
btf_encoder.c | 360 +++++++++++++++++++++++++++++++++++++--------
btf_encoder.h | 6 -
dwarf_loader.c | 130 +++++++++++++++-
dwarves.h | 11 +-
man-pages/pahole.1 | 10 ++
pahole.c | 30 +++-
6 files changed, 468 insertions(+), 79 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions 2023-02-07 17:14 [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions Alan Maguire @ 2023-02-08 16:20 ` Arnaldo Carvalho de Melo 2023-02-08 16:50 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 28+ messages in thread From: Arnaldo Carvalho de Melo @ 2023-02-08 16:20 UTC (permalink / raw) To: Alan Maguire Cc: ast, andrii, daniel, eddyz87, haoluo, jolsa, john.fastabend, kpsingh, sinquersw, martin.lau, songliubraving, sdf, timo, yhs, bpf Em Tue, Feb 07, 2023 at 05:14:54PM +0000, Alan Maguire escreveu: > At optimization level -O2 or higher in gcc, static functions may be > optimized such that they have suffixes like .isra.0, .constprop.0 etc. > These represent > > - constant propagation (.constprop.0); > - interprocedural scalar replacement of aggregates, removal of > unused parameters and replacement of parameters passed by > reference by parameters passed by value (.isra.0) Initial test, without using the new options: [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux | sort | uniq -c | sort -n | tail 3 start_show 3 timeout_show 3 uuid_show 4 m_next 4 parse_options 4 sk_diag_fill 4 state_show 4 state_store 5 status_show 6 type_show [acme@pumpkin ~]$ Now I'll use --skip_encoding_btf_inconsistent_proto and --btf_gen_optimized - Arnaldo > See [1] for details. > > Currently BTF encoding does not handle such optimized functions > that get renamed with a "." suffix such as ".isra.0", ".constprop.0". > This is safer because such suffixes can often indicate parameters have > been optimized out. This series addresses this by matching a > function to a suffixed version ("foo" matching "foo.isra.0") while > ensuring that the function signature does not contain optimized-out > parameters. Note that if the function is found ("foo") it will > be preferred, only falling back to "foo.isra.0" if lookup of the > function fails. Addition to BTF is skipped if the function has > optimized-out parameters, since the expected function signature > will not match. BTF encoding does not include the "."-suffix to > be consistent with DWARF. In addition, the kernel currently does > not allow a "." suffix in a BTF function name. > > A problem with this approach however is that BTF carries out the > encoding process in parallel across multiple CUs, and sometimes > a function has optimized-out parameters in one CU but not others; > we see this for NF_HOOK.constprop.0 for example. So in order to > determine if the function has optimized-out parameters in any > CU, its addition is not carried out until we have processed all > CUs and are about to merge BTF. At this point we know if any > such optimizations have occurred. Patches 1-5 handle the > optimized-out parameter identification and matching "."-suffixed > functions with the original function to facilitate BTF > encoding. This feature can be enabled via the > "--btf_gen_optimized" option. > > Patch 6 addresses a related problem - it is entirely possible > for a static function of the same name to exist in different > CUs with different function signatures. Because BTF does not > currently encode any information that would help disambiguate > which BTF function specification matches which static function > (in the case of multiple different function signatures), it is > best to eliminate such functions from BTF for now. The same > mechanism that is used to compare static "."-suffixed functions > is re-used for the static function comparison. A superficial > comparison of number of parameters/parameter names is done to > see if such representations are consistent, and if inconsistent > prototypes are observed, the function is flagged for exclusion > from BTF. > > When these methods are combined - the additive encoding of > "."-suffixed functions and the subtractive elimination of > functions with inconsistent parameters - we see an overall > drop in the number of functions in vmlinux BTF, from > 51529 to 50246. Skipping inconsistent functions is enabled > via "--skip_encoding_btf_inconsistent_proto". > > Changes since v2 [2] > - Arnaldo incorporated some of the suggestions in the v2 thread; > these patches are based on those; the relevant changes are > noted as committer changes. > - Patch 1 is unchanged from v2, but the rest of the patches > have been updated: > - Patch 2 separates out the changes to the struct btf_encoder > that better support later addition of functions. > - Patch 3 then is changed insofar as these changes are no > longer needed for the function addition refactoring. > - Patch 4 has a small change; we need to verify that an > encoder has actually been added to the encoders list > prior to removal > - Patch 5 changed significantly; when attempting to measure > performance the relatively good numbers attained when using > delayed function addition were not reproducible. > Further analysis revealed that the large number of lookups > caused by the presence of the separate function tree was > a major cause of performance degradation in the multi > threaded case. So instead of maintaining a separate tree, > we use the ELF function list which we already need to look > up to match ELF -> DWARF function descriptions to store > the function representation. This has 2 benefits; firstly > as mentioned, we already look up the ELF function so no > additional lookup is required to save the function. > Secondly, the ELF representation is identical for each > encoder, so we can index the same function across multiple > encoder function arrays - this greatly speeds up the > processing of comparing function representations across > encoders. There is still a performance cost in this > approach however; more details are provided in patch 6. > An option specific to adding functions with "." suffixes > is added "--btf_gen_optimized" > - Patch 6 builds on patch 5 in applying the save/merge/add > approach for all functions using the same mechanisms. > In addition the "--skip_encoding_btf_inconsistent_proto" > option is introduced. > - Patches 7/8 document the new options in the pahole manual > page. > > Changes since v1 [3] > > - Eduard noted that a DW_AT_const_value attribute can signal > an optimized-out parameter, and that the lack of a location > attribute signals optimization; ensure we handle those cases > also (Eduard, patch 1). > - Jiri noted we can have inconsistencies between a static > and non-static function; apply the comparison process to > all functions (Jiri, patch 5) > - segmentation fault was observed when handling functions with > > 10 parameters; needed parameter comparison loop to exit > at BTF_ENCODER_MAX_PARAMETERS (patch 5) > - Kui-Feng Lee pointed out that having a global shared function > tree would lead to a lot of contention; here a per-encoder > tree is used, and once the threads are collected the trees > are merged. Performance numbers are provided in patch 5 > (Kui-Feng Lee, patches 4/5) > > [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > [2] https://lore.kernel.org/bpf/1675088985-20300-1-git-send-email-alan.maguire@oracle.com/ > [3] https://lore.kernel.org/bpf/1674567931-26458-1-git-send-email-alan.maguire@oracle.com/ > > Alan Maguire (8): > dwarf_loader: Help spotting functions with optimized-out parameters > btf_encoder: store type_id_off, unspecified type in encoder > btf_encoder: Refactor function addition into dedicated > btf_encoder__add_func > btf_encoder: Rework btf_encoders__*() API to allow traversal of > encoders > btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF > btf_encoder: support delaying function addition to check for function > prototype inconsistencies > dwarves: document --btf_gen_optimized option > dwarves: document --skip_encoding_btf_inconsistent_proto option > > btf_encoder.c | 360 +++++++++++++++++++++++++++++++++++++-------- > btf_encoder.h | 6 - > dwarf_loader.c | 130 +++++++++++++++- > dwarves.h | 11 +- > man-pages/pahole.1 | 10 ++ > pahole.c | 30 +++- > 6 files changed, 468 insertions(+), 79 deletions(-) > > -- > 2.31.1 > -- - Arnaldo ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions 2023-02-08 16:20 ` Arnaldo Carvalho de Melo @ 2023-02-08 16:50 ` Arnaldo Carvalho de Melo [not found] ` <3c021d56-8818-2464-f7e0-889e769c0311@oracle.com> 0 siblings, 1 reply; 28+ messages in thread From: Arnaldo Carvalho de Melo @ 2023-02-08 16:50 UTC (permalink / raw) To: Alan Maguire Cc: ast, andrii, daniel, eddyz87, haoluo, jolsa, john.fastabend, kpsingh, sinquersw, martin.lau, songliubraving, sdf, timo, yhs, bpf Em Wed, Feb 08, 2023 at 01:20:39PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Feb 07, 2023 at 05:14:54PM +0000, Alan Maguire escreveu: > > At optimization level -O2 or higher in gcc, static functions may be > > optimized such that they have suffixes like .isra.0, .constprop.0 etc. > > These represent > > > > - constant propagation (.constprop.0); > > - interprocedural scalar replacement of aggregates, removal of > > unused parameters and replacement of parameters passed by > > reference by parameters passed by value (.isra.0) > > Initial test, without using the new options: > > [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux | sort | uniq -c | sort -n | tail > 3 start_show > 3 timeout_show > 3 uuid_show > 4 m_next > 4 parse_options > 4 sk_diag_fill > 4 state_show > 4 state_store > 5 status_show > 6 type_show > [acme@pumpkin ~]$ > > Now I'll use --skip_encoding_btf_inconsistent_proto and --btf_gen_optimized With: ⬢[acme@toolbox linux]$ git diff diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh index 1f1f1d397c399afe..9dd185fb1ff1fc3b 100755 --- a/scripts/pahole-flags.sh +++ b/scripts/pahole-flags.sh @@ -21,7 +21,7 @@ if [ "${pahole_ver}" -ge "122" ]; then fi if [ "${pahole_ver}" -ge "124" ]; then # see PAHOLE_HAS_LANG_EXCLUDE - extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" + extra_paholeopt="${extra_paholeopt} --lang_exclude=rust --btf_gen_optimized --skip_encoding_btf_inconsistent_proto" fi echo ${extra_paholeopt} ⬢[acme@toolbox linux]$ I get: [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux | sort | uniq -c | sort -n | tail 1 zswap_writeback_entry 1 zswap_zpool_param_set 1 zs_zpool_create 1 zs_zpool_destroy 1 zs_zpool_free 1 zs_zpool_malloc 1 zs_zpool_map 1 zs_zpool_shrink 1 zs_zpool_total_size 1 zs_zpool_unmap [acme@pumpkin ~]$ No functions with more than one entry: [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux | sort | uniq -c | sort -n | grep -v ' 1 ' [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux | sort | uniq -c | sort -n | grep ' 1 ' | wc -l 54558 [acme@pumpkin ~]$ pfunct /sys/kernel/btf/vmlinux | wc -l 54558 [acme@pumpkin ~]$ So I'll bump the release as we did in the past when testing features that we need to test against a release on the pahole-flags.sh script so that we can do further tests. - Arnaldo > - Arnaldo > > > See [1] for details. > > > > Currently BTF encoding does not handle such optimized functions > > that get renamed with a "." suffix such as ".isra.0", ".constprop.0". > > This is safer because such suffixes can often indicate parameters have > > been optimized out. This series addresses this by matching a > > function to a suffixed version ("foo" matching "foo.isra.0") while > > ensuring that the function signature does not contain optimized-out > > parameters. Note that if the function is found ("foo") it will > > be preferred, only falling back to "foo.isra.0" if lookup of the > > function fails. Addition to BTF is skipped if the function has > > optimized-out parameters, since the expected function signature > > will not match. BTF encoding does not include the "."-suffix to > > be consistent with DWARF. In addition, the kernel currently does > > not allow a "." suffix in a BTF function name. > > > > A problem with this approach however is that BTF carries out the > > encoding process in parallel across multiple CUs, and sometimes > > a function has optimized-out parameters in one CU but not others; > > we see this for NF_HOOK.constprop.0 for example. So in order to > > determine if the function has optimized-out parameters in any > > CU, its addition is not carried out until we have processed all > > CUs and are about to merge BTF. At this point we know if any > > such optimizations have occurred. Patches 1-5 handle the > > optimized-out parameter identification and matching "."-suffixed > > functions with the original function to facilitate BTF > > encoding. This feature can be enabled via the > > "--btf_gen_optimized" option. > > > > Patch 6 addresses a related problem - it is entirely possible > > for a static function of the same name to exist in different > > CUs with different function signatures. Because BTF does not > > currently encode any information that would help disambiguate > > which BTF function specification matches which static function > > (in the case of multiple different function signatures), it is > > best to eliminate such functions from BTF for now. The same > > mechanism that is used to compare static "."-suffixed functions > > is re-used for the static function comparison. A superficial > > comparison of number of parameters/parameter names is done to > > see if such representations are consistent, and if inconsistent > > prototypes are observed, the function is flagged for exclusion > > from BTF. > > > > When these methods are combined - the additive encoding of > > "."-suffixed functions and the subtractive elimination of > > functions with inconsistent parameters - we see an overall > > drop in the number of functions in vmlinux BTF, from > > 51529 to 50246. Skipping inconsistent functions is enabled > > via "--skip_encoding_btf_inconsistent_proto". > > > > Changes since v2 [2] > > - Arnaldo incorporated some of the suggestions in the v2 thread; > > these patches are based on those; the relevant changes are > > noted as committer changes. > > - Patch 1 is unchanged from v2, but the rest of the patches > > have been updated: > > - Patch 2 separates out the changes to the struct btf_encoder > > that better support later addition of functions. > > - Patch 3 then is changed insofar as these changes are no > > longer needed for the function addition refactoring. > > - Patch 4 has a small change; we need to verify that an > > encoder has actually been added to the encoders list > > prior to removal > > - Patch 5 changed significantly; when attempting to measure > > performance the relatively good numbers attained when using > > delayed function addition were not reproducible. > > Further analysis revealed that the large number of lookups > > caused by the presence of the separate function tree was > > a major cause of performance degradation in the multi > > threaded case. So instead of maintaining a separate tree, > > we use the ELF function list which we already need to look > > up to match ELF -> DWARF function descriptions to store > > the function representation. This has 2 benefits; firstly > > as mentioned, we already look up the ELF function so no > > additional lookup is required to save the function. > > Secondly, the ELF representation is identical for each > > encoder, so we can index the same function across multiple > > encoder function arrays - this greatly speeds up the > > processing of comparing function representations across > > encoders. There is still a performance cost in this > > approach however; more details are provided in patch 6. > > An option specific to adding functions with "." suffixes > > is added "--btf_gen_optimized" > > - Patch 6 builds on patch 5 in applying the save/merge/add > > approach for all functions using the same mechanisms. > > In addition the "--skip_encoding_btf_inconsistent_proto" > > option is introduced. > > - Patches 7/8 document the new options in the pahole manual > > page. > > > > Changes since v1 [3] > > > > - Eduard noted that a DW_AT_const_value attribute can signal > > an optimized-out parameter, and that the lack of a location > > attribute signals optimization; ensure we handle those cases > > also (Eduard, patch 1). > > - Jiri noted we can have inconsistencies between a static > > and non-static function; apply the comparison process to > > all functions (Jiri, patch 5) > > - segmentation fault was observed when handling functions with > > > 10 parameters; needed parameter comparison loop to exit > > at BTF_ENCODER_MAX_PARAMETERS (patch 5) > > - Kui-Feng Lee pointed out that having a global shared function > > tree would lead to a lot of contention; here a per-encoder > > tree is used, and once the threads are collected the trees > > are merged. Performance numbers are provided in patch 5 > > (Kui-Feng Lee, patches 4/5) > > > > [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > > [2] https://lore.kernel.org/bpf/1675088985-20300-1-git-send-email-alan.maguire@oracle.com/ > > [3] https://lore.kernel.org/bpf/1674567931-26458-1-git-send-email-alan.maguire@oracle.com/ > > > > Alan Maguire (8): > > dwarf_loader: Help spotting functions with optimized-out parameters > > btf_encoder: store type_id_off, unspecified type in encoder > > btf_encoder: Refactor function addition into dedicated > > btf_encoder__add_func > > btf_encoder: Rework btf_encoders__*() API to allow traversal of > > encoders > > btf_encoder: Represent "."-suffixed functions (".isra.0") in BTF > > btf_encoder: support delaying function addition to check for function > > prototype inconsistencies > > dwarves: document --btf_gen_optimized option > > dwarves: document --skip_encoding_btf_inconsistent_proto option > > > > btf_encoder.c | 360 +++++++++++++++++++++++++++++++++++++-------- > > btf_encoder.h | 6 - > > dwarf_loader.c | 130 +++++++++++++++- > > dwarves.h | 11 +- > > man-pages/pahole.1 | 10 ++ > > pahole.c | 30 +++- > > 6 files changed, 468 insertions(+), 79 deletions(-) > > > > -- > > 2.31.1 > > > > -- > > - Arnaldo -- - Arnaldo ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <3c021d56-8818-2464-f7e0-889e769c0311@oracle.com>]
* Re: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 [not found] ` <3c021d56-8818-2464-f7e0-889e769c0311@oracle.com> @ 2023-02-09 13:09 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 28+ messages in thread From: Arnaldo Carvalho de Melo @ 2023-02-09 13:09 UTC (permalink / raw) To: Alan Maguire Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, bpf, Jiri Olsa, dwarves Em Wed, Feb 08, 2023 at 05:17:02PM +0000, Alan Maguire escreveu: > From: Alan Maguire <alan.maguire@oracle.com> > Date: Thu, 2 Feb 2023 12:26:20 +0000 > Subject: [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, > --btf_gen_optimized to pahole flags for v1.25 > > v1.25 of pahole supports filtering out functions with multiple > inconsistent function prototypes or optimized-out parameters > from the BTF representation. These present problems because > there is no additional info in BTF saying which inconsistent > prototype matches which function instance to help guide > attachment, and functions with optimized-out parameters can > lead to incorrect assumptions about register contents. > > So for now, filter out such functions while adding BTF > representations for functions that have "."-suffixes > (foo.isra.0) but not optimized-out parameters. > > This patch assumes changes in [1] land and pahole is bumped > to v1.25. > > [1] https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/ > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > --- > scripts/pahole-flags.sh | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh > index 1f1f1d3..728d551 100755 > --- a/scripts/pahole-flags.sh > +++ b/scripts/pahole-flags.sh > @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then > # see PAHOLE_HAS_LANG_EXCLUDE > extra_paholeopt="${extra_paholeopt} --lang_exclude=rust" > fi > +if [ "${pahole_ver}" -ge "125" ]; then > + extra_paholeopt="${extra_paholeopt} --skip_encoding_btf_inconsistent_proto --btf_gen_optimized" > +fi > > echo ${extra_paholeopt} > -- > 1.8.3.1 I applied the patch and it works as advertised using what is in pahole's 'next' branch, that should go to 'master' later today. Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> - Arnaldo ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2023-05-15 19:34 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-10 13:02 [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 Alan Maguire
2023-05-10 17:15 ` Jiri Olsa
2023-05-12 2:51 ` Yafang Shao
2023-05-12 16:03 ` Alan Maguire
2023-05-12 18:59 ` Alexei Starovoitov
2023-05-12 21:54 ` Jiri Olsa
2023-05-13 2:59 ` Yonghong Song
2023-05-14 17:37 ` Yonghong Song
2023-05-14 21:49 ` Jiri Olsa
2023-05-15 4:06 ` Yonghong Song
2023-05-15 14:53 ` Alan Maguire
2023-05-15 19:33 ` Yonghong Song
2023-05-12 19:00 ` patchwork-bot+netdevbpf
-- strict thread matches above, loose matches on Subject: below --
2023-02-09 13:28 [PATCH bpf-next] bpf: add " Alan Maguire
2023-02-09 15:08 ` Jiri Olsa
2023-02-13 17:00 ` patchwork-bot+netdevbpf
2023-02-14 3:12 ` Alexei Starovoitov
2023-02-14 6:09 ` Alexei Starovoitov
2023-03-09 1:53 ` Arnaldo Carvalho de Melo
2023-03-09 8:16 ` Jiri Olsa
2023-03-09 10:11 ` Jiri Olsa
2023-03-09 12:26 ` Arnaldo Carvalho de Melo
2023-03-09 14:58 ` Alan Maguire
2023-02-14 12:27 ` Jiri Olsa
2023-02-14 16:17 ` Arnaldo Carvalho de Melo
2023-02-14 22:12 ` Jiri Olsa
2023-02-17 13:45 ` Alan Maguire
2023-02-07 17:14 [PATCH v3 dwarves 0/8] dwarves: support encoding of optimized-out parameters, removal of inconsistent static functions Alan Maguire
2023-02-08 16:20 ` Arnaldo Carvalho de Melo
2023-02-08 16:50 ` Arnaldo Carvalho de Melo
[not found] ` <3c021d56-8818-2464-f7e0-889e769c0311@oracle.com>
2023-02-09 13:09 ` [PATCH bpf-next] bpf: add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25 Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox