| Message ID | 20200313002128.2028680-1-andriin@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [v2,bpf-next] bpf: abstract away entire bpf_link clean up procedure | expand |
On Thu, Mar 12, 2020 at 05:21:28PM -0700, Andrii Nakryiko wrote: > Instead of requiring users to do three steps for cleaning up bpf_link, its > anon_inode file, and unused fd, abstract that away into bpf_link_cleanup() > helper. bpf_link_defunct() is removed, as it shouldn't be needed as an > individual operation anymore. > > v1->v2: > - keep bpf_link_cleanup() static for now (Daniel). Acked-by: Martin KaFai Lau <kafai@fb.com>
On Thu, Mar 12, 2020 at 05:21:28PM -0700, Andrii Nakryiko wrote: > Instead of requiring users to do three steps for cleaning up bpf_link, its > anon_inode file, and unused fd, abstract that away into bpf_link_cleanup() > helper. bpf_link_defunct() is removed, as it shouldn't be needed as an > individual operation anymore. > > v1->v2: > - keep bpf_link_cleanup() static for now (Daniel). > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> Applied. But noticed that the test is now sporadically failing: ./test_progs -n 24 test_link_pinning:PASS:skel_open 0 nsec test_link_pinning_subtest:PASS:link_attach 0 nsec test_link_pinning_subtest:PASS:res_check1 0 nsec test_link_pinning_subtest:PASS:link_pin 0 nsec test_link_pinning_subtest:PASS:pin_path1 0 nsec test_link_pinning_subtest:PASS:stat_link 0 nsec test_link_pinning_subtest:PASS:res_check2 0 nsec test_link_pinning_subtest:PASS:res_check3 0 nsec test_link_pinning_subtest:PASS:link_open 0 nsec test_link_pinning_subtest:PASS:pin_path2 0 nsec test_link_pinning_subtest:PASS:link_unpin 0 nsec test_link_pinning_subtest:PASS:res_check4 0 nsec test_link_pinning_subtest:FAIL:link_attached got to iteration #10000 #24/1 pin_raw_tp:FAIL test_link_pinning_subtest:PASS:link_attach 0 nsec test_link_pinning_subtest:PASS:res_check1 0 nsec test_link_pinning_subtest:PASS:link_pin 0 nsec test_link_pinning_subtest:PASS:pin_path1 0 nsec test_link_pinning_subtest:PASS:stat_link 0 nsec test_link_pinning_subtest:PASS:res_check2 0 nsec test_link_pinning_subtest:PASS:res_check3 0 nsec test_link_pinning_subtest:PASS:link_open 0 nsec test_link_pinning_subtest:PASS:pin_path2 0 nsec test_link_pinning_subtest:PASS:link_unpin 0 nsec test_link_pinning_subtest:PASS:res_check4 0 nsec test_link_pinning_subtest:FAIL:link_attached got to iteration #10000 #24/2 pin_tp_btf:FAIL #24 link_pinning:FAIL Summary: 0/0 PASSED, 0 SKIPPED, 3 FAILED it's failing more often than passing, actually. The #64 tcp_rtt also started to fail sporadically. But I wonder whether it's leftover from 24. shrug.
On Thu, Mar 12, 2020 at 6:50 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Mar 12, 2020 at 05:21:28PM -0700, Andrii Nakryiko wrote: > > Instead of requiring users to do three steps for cleaning up bpf_link, its > > anon_inode file, and unused fd, abstract that away into bpf_link_cleanup() > > helper. bpf_link_defunct() is removed, as it shouldn't be needed as an > > individual operation anymore. > > > > v1->v2: > > - keep bpf_link_cleanup() static for now (Daniel). > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > Applied. > > But noticed that the test is now sporadically failing: > ./test_progs -n 24 > test_link_pinning:PASS:skel_open 0 nsec > test_link_pinning_subtest:PASS:link_attach 0 nsec > test_link_pinning_subtest:PASS:res_check1 0 nsec > test_link_pinning_subtest:PASS:link_pin 0 nsec > test_link_pinning_subtest:PASS:pin_path1 0 nsec > test_link_pinning_subtest:PASS:stat_link 0 nsec > test_link_pinning_subtest:PASS:res_check2 0 nsec > test_link_pinning_subtest:PASS:res_check3 0 nsec > test_link_pinning_subtest:PASS:link_open 0 nsec > test_link_pinning_subtest:PASS:pin_path2 0 nsec > test_link_pinning_subtest:PASS:link_unpin 0 nsec > test_link_pinning_subtest:PASS:res_check4 0 nsec > test_link_pinning_subtest:FAIL:link_attached got to iteration #10000 > #24/1 pin_raw_tp:FAIL > test_link_pinning_subtest:PASS:link_attach 0 nsec > test_link_pinning_subtest:PASS:res_check1 0 nsec > test_link_pinning_subtest:PASS:link_pin 0 nsec > test_link_pinning_subtest:PASS:pin_path1 0 nsec > test_link_pinning_subtest:PASS:stat_link 0 nsec > test_link_pinning_subtest:PASS:res_check2 0 nsec > test_link_pinning_subtest:PASS:res_check3 0 nsec > test_link_pinning_subtest:PASS:link_open 0 nsec > test_link_pinning_subtest:PASS:pin_path2 0 nsec > test_link_pinning_subtest:PASS:link_unpin 0 nsec > test_link_pinning_subtest:PASS:res_check4 0 nsec > test_link_pinning_subtest:FAIL:link_attached got to iteration #10000 > #24/2 pin_tp_btf:FAIL > #24 link_pinning:FAIL > Summary: 0/0 PASSED, 0 SKIPPED, 3 FAILED > > it's failing more often than passing, actually. Can't repro this even with 2 parallel kernel builds and running this test in VM in a loop. I can bump waiting time a little bit or can drop that check, because it's inherently non-deterministic... > > The #64 tcp_rtt also started to fail sporadically. > But I wonder whether it's leftover from 24. shrug. Can you please paste log from #64 failure?
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 4fd91b7c95ea..49389ddb948f 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1075,7 +1075,6 @@ struct bpf_link_ops { void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops, struct bpf_prog *prog); -void bpf_link_defunct(struct bpf_link *link); void bpf_link_inc(struct bpf_link *link); void bpf_link_put(struct bpf_link *link); int bpf_link_new_fd(struct bpf_link *link); diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index b2f73ecacced..85567a6ea5f9 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2188,9 +2188,17 @@ void bpf_link_init(struct bpf_link *link, const struct bpf_link_ops *ops, link->prog = prog; } -void bpf_link_defunct(struct bpf_link *link) +/* Clean up bpf_link and corresponding anon_inode file and FD. After + * anon_inode is created, bpf_link can't be just kfree()'d due to deferred + * anon_inode's release() call. This helper manages marking bpf_link as + * defunct, releases anon_inode file and puts reserved FD. + */ +static void bpf_link_cleanup(struct bpf_link *link, struct file *link_file, + int link_fd) { link->prog = NULL; + fput(link_file); + put_unused_fd(link_fd); } void bpf_link_inc(struct bpf_link *link) @@ -2383,9 +2391,7 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog) err = bpf_trampoline_link_prog(prog); if (err) { - bpf_link_defunct(&link->link); - fput(link_file); - put_unused_fd(link_fd); + bpf_link_cleanup(&link->link, link_file, link_fd); goto out_put_prog; } @@ -2498,9 +2504,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) err = bpf_probe_register(link->btp, prog); if (err) { - bpf_link_defunct(&link->link); - fput(link_file); - put_unused_fd(link_fd); + bpf_link_cleanup(&link->link, link_file, link_fd); goto out_put_btp; }
Instead of requiring users to do three steps for cleaning up bpf_link, its anon_inode file, and unused fd, abstract that away into bpf_link_cleanup() helper. bpf_link_defunct() is removed, as it shouldn't be needed as an individual operation anymore. v1->v2: - keep bpf_link_cleanup() static for now (Daniel). Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- include/linux/bpf.h | 1 - kernel/bpf/syscall.c | 18 +++++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-)