| Message ID | 20230826200843.2210074-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 32337c0a28242f725c2c499c15100d67a4133050 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Prevent inlining of bpf_fentry_test7() | expand |
On Sat, 2023-08-26 at 13:08 -0700, Yonghong Song wrote: > With latest clang18, I hit test_progs failures for the following test: > #13/2 bpf_cookie/multi_kprobe_link_api:FAIL > #13/3 bpf_cookie/multi_kprobe_attach_api:FAIL > #13 bpf_cookie:FAIL > #75 fentry_fexit:FAIL > #76/1 fentry_test/fentry:FAIL > #76 fentry_test:FAIL > #80/1 fexit_test/fexit:FAIL > #80 fexit_test:FAIL > #110/1 kprobe_multi_test/skel_api:FAIL > #110/2 kprobe_multi_test/link_api_addrs:FAIL > #110/3 kprobe_multi_test/link_api_syms:FAIL > #110/4 kprobe_multi_test/attach_api_pattern:FAIL > #110/5 kprobe_multi_test/attach_api_addrs:FAIL > #110/6 kprobe_multi_test/attach_api_syms:FAIL > #110 kprobe_multi_test:FAIL > > For example, for #13/2, the error messages are > ... > kprobe_multi_test_run:FAIL:kprobe_test7_result unexpected kprobe_test7_result: actual 0 != expected 1 > ... > kprobe_multi_test_run:FAIL:kretprobe_test7_result unexpected kretprobe_test7_result: actual 0 != expected 1 > > clang17 does not have this issue. > > Further investigation shows that kernel func bpf_fentry_test7(), used > in the above tests, is inlined by the compiler although it is > marked as noinline. > > int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) > { > return (long)arg; > } > > It is known that for simple functions like the above (e.g. just returning > a constant or an input argument), the clang compiler may still do inlining > for a noinline function. Adding 'asm volatile ("")' in the beginning of the > bpf_fentry_test7() can prevent inlining. Can confirm, this patch fixes listed flaky tests for me. (when using LLVM main 651e644595b7 from 6 days ago). Tested-by: Eduard Zingerman <eddyz87@gmail.com> > > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > --- > net/bpf/test_run.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index 57a7a64b84ed..0841f8d82419 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -543,6 +543,7 @@ struct bpf_fentry_test_t { > > int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) > { > + asm volatile (""); > return (long)arg; > } >
On 26/08/2023 21:08, Yonghong Song wrote: > With latest clang18, I hit test_progs failures for the following test: > #13/2 bpf_cookie/multi_kprobe_link_api:FAIL > #13/3 bpf_cookie/multi_kprobe_attach_api:FAIL > #13 bpf_cookie:FAIL > #75 fentry_fexit:FAIL > #76/1 fentry_test/fentry:FAIL > #76 fentry_test:FAIL > #80/1 fexit_test/fexit:FAIL > #80 fexit_test:FAIL > #110/1 kprobe_multi_test/skel_api:FAIL > #110/2 kprobe_multi_test/link_api_addrs:FAIL > #110/3 kprobe_multi_test/link_api_syms:FAIL > #110/4 kprobe_multi_test/attach_api_pattern:FAIL > #110/5 kprobe_multi_test/attach_api_addrs:FAIL > #110/6 kprobe_multi_test/attach_api_syms:FAIL > #110 kprobe_multi_test:FAIL > > For example, for #13/2, the error messages are > ... > kprobe_multi_test_run:FAIL:kprobe_test7_result unexpected kprobe_test7_result: actual 0 != expected 1 > ... > kprobe_multi_test_run:FAIL:kretprobe_test7_result unexpected kretprobe_test7_result: actual 0 != expected 1 > > clang17 does not have this issue. > > Further investigation shows that kernel func bpf_fentry_test7(), used > in the above tests, is inlined by the compiler although it is > marked as noinline. > > int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) > { > return (long)arg; > } > > It is known that for simple functions like the above (e.g. just returning > a constant or an input argument), the clang compiler may still do inlining > for a noinline function. Adding 'asm volatile ("")' in the beginning of the > bpf_fentry_test7() can prevent inlining. > > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> Fixes: d923021c2ce12 ("bpf: Add tests for PTR_TO_BTF_ID vs. null comparison") ...might help this land in stable trees too. Thanks! > --- > net/bpf/test_run.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index 57a7a64b84ed..0841f8d82419 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -543,6 +543,7 @@ struct bpf_fentry_test_t { > > int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) > { > + asm volatile (""); > return (long)arg; > } > Is there a risk bpf_fentry_test8/9 might get inlined too?
On 8/28/23 12:26 AM, Alan Maguire wrote: > On 26/08/2023 21:08, Yonghong Song wrote: >> With latest clang18, I hit test_progs failures for the following test: >> #13/2 bpf_cookie/multi_kprobe_link_api:FAIL >> #13/3 bpf_cookie/multi_kprobe_attach_api:FAIL >> #13 bpf_cookie:FAIL >> #75 fentry_fexit:FAIL >> #76/1 fentry_test/fentry:FAIL >> #76 fentry_test:FAIL >> #80/1 fexit_test/fexit:FAIL >> #80 fexit_test:FAIL >> #110/1 kprobe_multi_test/skel_api:FAIL >> #110/2 kprobe_multi_test/link_api_addrs:FAIL >> #110/3 kprobe_multi_test/link_api_syms:FAIL >> #110/4 kprobe_multi_test/attach_api_pattern:FAIL >> #110/5 kprobe_multi_test/attach_api_addrs:FAIL >> #110/6 kprobe_multi_test/attach_api_syms:FAIL >> #110 kprobe_multi_test:FAIL >> >> For example, for #13/2, the error messages are >> ... >> kprobe_multi_test_run:FAIL:kprobe_test7_result unexpected kprobe_test7_result: actual 0 != expected 1 >> ... >> kprobe_multi_test_run:FAIL:kretprobe_test7_result unexpected kretprobe_test7_result: actual 0 != expected 1 >> >> clang17 does not have this issue. >> >> Further investigation shows that kernel func bpf_fentry_test7(), used >> in the above tests, is inlined by the compiler although it is >> marked as noinline. >> >> int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) >> { >> return (long)arg; >> } >> >> It is known that for simple functions like the above (e.g. just returning >> a constant or an input argument), the clang compiler may still do inlining >> for a noinline function. Adding 'asm volatile ("")' in the beginning of the >> bpf_fentry_test7() can prevent inlining. >> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > > Fixes: d923021c2ce12 ("bpf: Add tests for PTR_TO_BTF_ID vs. null > comparison") > > ...might help this land in stable trees too. Thanks! I didn't put a Fixes tag since the issue is caused by upgrading clang compiler. The commit d923021c2ce12 does not have any issues at its patch-applying time. Since this is not a core kernel functionality bug and it is due to upgrading clang compiler, I think backport is optional. But if maintainers think I should add Fixes tag, I can do it in the next revision. > >> --- >> net/bpf/test_run.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c >> index 57a7a64b84ed..0841f8d82419 100644 >> --- a/net/bpf/test_run.c >> +++ b/net/bpf/test_run.c >> @@ -543,6 +543,7 @@ struct bpf_fentry_test_t { >> >> int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) >> { >> + asm volatile (""); >> return (long)arg; >> } >> > Is there a risk bpf_fentry_test8/9 might get inlined too? So far, not. I cannot predict in the future. But if this happens with more complicated expressions got inlined for a non-inline function, I can discuss with upstream.
Hello: This patch was applied to bpf/bpf.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Sat, 26 Aug 2023 13:08:43 -0700 you wrote: > With latest clang18, I hit test_progs failures for the following test: > #13/2 bpf_cookie/multi_kprobe_link_api:FAIL > #13/3 bpf_cookie/multi_kprobe_attach_api:FAIL > #13 bpf_cookie:FAIL > #75 fentry_fexit:FAIL > #76/1 fentry_test/fentry:FAIL > #76 fentry_test:FAIL > #80/1 fexit_test/fexit:FAIL > #80 fexit_test:FAIL > #110/1 kprobe_multi_test/skel_api:FAIL > #110/2 kprobe_multi_test/link_api_addrs:FAIL > #110/3 kprobe_multi_test/link_api_syms:FAIL > #110/4 kprobe_multi_test/attach_api_pattern:FAIL > #110/5 kprobe_multi_test/attach_api_addrs:FAIL > #110/6 kprobe_multi_test/attach_api_syms:FAIL > #110 kprobe_multi_test:FAIL > > [...] Here is the summary with links: - [bpf-next] bpf: Prevent inlining of bpf_fentry_test7() https://git.kernel.org/bpf/bpf/c/32337c0a2824 You are awesome, thank you!
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 57a7a64b84ed..0841f8d82419 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -543,6 +543,7 @@ struct bpf_fentry_test_t { int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) { + asm volatile (""); return (long)arg; }
With latest clang18, I hit test_progs failures for the following test: #13/2 bpf_cookie/multi_kprobe_link_api:FAIL #13/3 bpf_cookie/multi_kprobe_attach_api:FAIL #13 bpf_cookie:FAIL #75 fentry_fexit:FAIL #76/1 fentry_test/fentry:FAIL #76 fentry_test:FAIL #80/1 fexit_test/fexit:FAIL #80 fexit_test:FAIL #110/1 kprobe_multi_test/skel_api:FAIL #110/2 kprobe_multi_test/link_api_addrs:FAIL #110/3 kprobe_multi_test/link_api_syms:FAIL #110/4 kprobe_multi_test/attach_api_pattern:FAIL #110/5 kprobe_multi_test/attach_api_addrs:FAIL #110/6 kprobe_multi_test/attach_api_syms:FAIL #110 kprobe_multi_test:FAIL For example, for #13/2, the error messages are ... kprobe_multi_test_run:FAIL:kprobe_test7_result unexpected kprobe_test7_result: actual 0 != expected 1 ... kprobe_multi_test_run:FAIL:kretprobe_test7_result unexpected kretprobe_test7_result: actual 0 != expected 1 clang17 does not have this issue. Further investigation shows that kernel func bpf_fentry_test7(), used in the above tests, is inlined by the compiler although it is marked as noinline. int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg) { return (long)arg; } It is known that for simple functions like the above (e.g. just returning a constant or an input argument), the clang compiler may still do inlining for a noinline function. Adding 'asm volatile ("")' in the beginning of the bpf_fentry_test7() can prevent inlining. Signed-off-by: Yonghong Song <yonghong.song@linux.dev> --- net/bpf/test_run.c | 1 + 1 file changed, 1 insertion(+)