Message ID | 32cf8376a810e2e9c719f8e4cfb97132ed2d1f9c.1716446893.git.tanggeliang@kylinos.cn (mailing list archive) |
---|---|
State | Accepted |
Commit | 3f32a115f61d31049e3e91d469bca849f712a979 |
Delegated to: | BPF |
Headers | show |
Series | fixes for test_sockmap | expand |
Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > Switch attachments to bpf_link using bpf_program__attach_sockmap() instead > of bpf_prog_attach(). Sorry it took me a few days to get to this. Is there a reason to push this to links vs just leave it as is? I had a plan to port all the test_sockmap tests into prog_tests anyways. I'll try to push some initial patch next week. The one advantage of test_sockmap is we can have it run for longer runs by pushing different options through so might be worth keeping just for that. If you really want links here I'm OK with that I guess just asking. Thanks, John > > This patch adds a new array progs[] to replace prog_fd[] array, set in > populate_progs() for each program in bpf object. > > And another new array links[] to save the attached bpf_link. It is > initalized as NULL in populate_progs, set as the return valuses of > bpf_program__attach_sockmap(), and detached by bpf_link__detach(). > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > tools/testing/selftests/bpf/test_sockmap.c | 59 ++++++++++++---------- > 1 file changed, 31 insertions(+), 28 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c > index e7dbf49a2ca6..d7581bbbc473 100644 > --- a/tools/testing/selftests/bpf/test_sockmap.c > +++ b/tools/testing/selftests/bpf/test_sockmap.c > @@ -64,6 +64,8 @@ int failed; > int map_fd[9]; > struct bpf_map *maps[9]; > int prog_fd[9]; > +struct bpf_program *progs[9]; > +struct bpf_link *links[9]; > > int txmsg_pass; > int txmsg_redir; > @@ -960,43 +962,39 @@ static int run_options(struct sockmap_options *options, int cg_fd, int test) > > /* Attach programs to sockmap */ > if (!txmsg_omit_skb_parser) { > - err = bpf_prog_attach(prog_fd[0], map_fd[0], > - BPF_SK_SKB_STREAM_PARSER, 0); > - if (err) { > + links[0] = bpf_program__attach_sockmap(progs[0], map_fd[0]); > + if (!links[0]) { > fprintf(stderr, > - "ERROR: bpf_prog_attach (sockmap %i->%i): %d (%s)\n", > - prog_fd[0], map_fd[0], err, strerror(errno)); > - return err; > + "ERROR: bpf_program__attach_sockmap (sockmap %i->%i): (%s)\n", > + bpf_program__fd(progs[0]), map_fd[0], strerror(errno)); > + return -1; > } > } > > - err = bpf_prog_attach(prog_fd[1], map_fd[0], > - BPF_SK_SKB_STREAM_VERDICT, 0); > - if (err) { > - fprintf(stderr, "ERROR: bpf_prog_attach (sockmap): %d (%s)\n", > - err, strerror(errno)); > - return err; > + links[1] = bpf_program__attach_sockmap(progs[1], map_fd[0]); > + if (!links[1]) { > + fprintf(stderr, "ERROR: bpf_program__attach_sockmap (sockmap): (%s)\n", > + strerror(errno)); > + return -1; > } > > /* Attach programs to TLS sockmap */ > if (txmsg_ktls_skb) { > if (!txmsg_omit_skb_parser) { > - err = bpf_prog_attach(prog_fd[0], map_fd[8], > - BPF_SK_SKB_STREAM_PARSER, 0); > - if (err) { > + links[2] = bpf_program__attach_sockmap(progs[0], map_fd[8]); > + if (!links[2]) { > fprintf(stderr, > - "ERROR: bpf_prog_attach (TLS sockmap %i->%i): %d (%s)\n", > - prog_fd[0], map_fd[8], err, strerror(errno)); > - return err; > + "ERROR: bpf_program__attach_sockmap (TLS sockmap %i->%i): (%s)\n", > + bpf_program__fd(progs[0]), map_fd[8], strerror(errno)); > + return -1; > } > } > > - err = bpf_prog_attach(prog_fd[2], map_fd[8], > - BPF_SK_SKB_STREAM_VERDICT, 0); > - if (err) { > - fprintf(stderr, "ERROR: bpf_prog_attach (TLS sockmap): %d (%s)\n", > - err, strerror(errno)); > - return err; > + links[3] = bpf_program__attach_sockmap(progs[2], map_fd[8]); > + if (!links[3]) { > + fprintf(stderr, "ERROR: bpf_program__attach_sockmap (TLS sockmap): (%s)\n", > + strerror(errno)); > + return -1; > } > } > > @@ -1281,10 +1279,11 @@ static int run_options(struct sockmap_options *options, int cg_fd, int test) > out: > /* Detatch and zero all the maps */ > bpf_prog_detach2(prog_fd[3], cg_fd, BPF_CGROUP_SOCK_OPS); > - bpf_prog_detach2(prog_fd[0], map_fd[0], BPF_SK_SKB_STREAM_PARSER); > - bpf_prog_detach2(prog_fd[1], map_fd[0], BPF_SK_SKB_STREAM_VERDICT); > - bpf_prog_detach2(prog_fd[0], map_fd[8], BPF_SK_SKB_STREAM_PARSER); > - bpf_prog_detach2(prog_fd[2], map_fd[8], BPF_SK_SKB_STREAM_VERDICT); > + > + for (i = 0; i < ARRAY_SIZE(links); i++) { > + if (links[i]) > + bpf_link__detach(links[i]); > + } > > if (tx_prog_fd > 0) > bpf_prog_detach2(tx_prog_fd, map_fd[1], BPF_SK_MSG_VERDICT); > @@ -1836,6 +1835,7 @@ static int populate_progs(char *bpf_file) > i = bpf_object__load(obj); > i = 0; > bpf_object__for_each_program(prog, obj) { > + progs[i] = prog; > prog_fd[i] = bpf_program__fd(prog); > i++; > } > @@ -1850,6 +1850,9 @@ static int populate_progs(char *bpf_file) > } > } > > + for (i = 0; i < ARRAY_SIZE(links); i++) > + links[i] = NULL; > + > return 0; > } > > -- > 2.43.0 >
On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote: > Geliang Tang wrote: >> From: Geliang Tang <tanggeliang@kylinos.cn> >> >> Switch attachments to bpf_link using bpf_program__attach_sockmap() instead >> of bpf_prog_attach(). > > Sorry it took me a few days to get to this. > > Is there a reason to push this to links vs just leave it as is? I had > a plan to port all the test_sockmap tests into prog_tests anyways. I'll > try to push some initial patch next week. > > The one advantage of test_sockmap is we can have it run for longer > runs by pushing different options through so might be worth keeping > just for that. > > If you really want links here I'm OK with that I guess just asking. It was me who suggested the switch to bpf_link in reaction to a series of cleanups to prog_type and prog_attach_type submitted by Geliang. Relevant threads: https://lore.kernel.org/bpf/9c10d9f974f07fcb354a43a8eca67acb2fafc587.1715926605.git.tanggeliang@kylinos.cn https://lore.kernel.org/bpf/20240522080936.2475833-1-jakub@cloudflare.com https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn I thought bpf_links added more value than cleaning up "old style" attachments.
On Mon, 2024-05-27 at 21:36 +0200, Jakub Sitnicki wrote: > On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote: > > Geliang Tang wrote: > > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > > > Switch attachments to bpf_link using > > > bpf_program__attach_sockmap() instead > > > of bpf_prog_attach(). > > > > Sorry it took me a few days to get to this. > > > > Is there a reason to push this to links vs just leave it as is? I > > had > > a plan to port all the test_sockmap tests into prog_tests anyways. > > I'll > > try to push some initial patch next week. Great, I strongly agree with porting them into prog_tests. I am also willing to participate in implementing this plan together. > > > > The one advantage of test_sockmap is we can have it run for longer > > runs by pushing different options through so might be worth keeping > > just for that. > > > > If you really want links here I'm OK with that I guess just asking. > > It was me who suggested the switch to bpf_link in reaction to a > series > of cleanups to prog_type and prog_attach_type submitted by Geliang. Yes, patches 3-5 address Jakub's suggestion: switching attachments to bpf_link. > Relevant threads: > > https://lore.kernel.org/bpf/9c10d9f974f07fcb354a43a8eca67acb2fafc587.1715926605.git.tanggeliang@kylinos.cn > https://lore.kernel.org/bpf/20240522080936.2475833-1-jakub@cloudflare.com > https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn > > I thought bpf_links added more value than cleaning up "old style" > attachments. Other patches 1-2, 6-8 are small fixes which I found while trying to solve the NONBLOCK issue [1]. Yes, I haven't given up on solving this issue yet. I think it must be solved, since there is a bug somewhere. WDYT? [1] https://patchwork.kernel.org/project/netdevbpf/patch/b67101632c6858d281f105b5d4e1bc62dd6b7d27.1712133039.git.tanggeliang@kylinos.cn/ Thanks, -Geliang
Geliang Tang wrote: > On Mon, 2024-05-27 at 21:36 +0200, Jakub Sitnicki wrote: > > On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote: > > > Geliang Tang wrote: > > > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > > > > > Switch attachments to bpf_link using > > > > bpf_program__attach_sockmap() instead > > > > of bpf_prog_attach(). > > > > > > Sorry it took me a few days to get to this. > > > > > > Is there a reason to push this to links vs just leave it as is? I > > > had > > > a plan to port all the test_sockmap tests into prog_tests anyways. > > > I'll > > > try to push some initial patch next week. > > Great, I strongly agree with porting them into prog_tests. I am also > willing to participate in implementing this plan together. I have a first patch that starts to move things I'll dig it up here. Still a bit behind on everything as you see its Thr already. > > > > > > > The one advantage of test_sockmap is we can have it run for longer > > > runs by pushing different options through so might be worth keeping > > > just for that. > > > > > > If you really want links here I'm OK with that I guess just asking. > > > > It was me who suggested the switch to bpf_link in reaction to a > > series > > of cleanups to prog_type and prog_attach_type submitted by Geliang. > > Yes, patches 3-5 address Jakub's suggestion: switching attachments to > bpf_link. OK. Lets just take them the series lgtm. Jakub any other comments? > > > Relevant threads: > > > > https://lore.kernel.org/bpf/9c10d9f974f07fcb354a43a8eca67acb2fafc587.1715926605.git.tanggeliang@kylinos.cn > > https://lore.kernel.org/bpf/20240522080936.2475833-1-jakub@cloudflare.com > > https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn > > > > I thought bpf_links added more value than cleaning up "old style" > > attachments. > > Other patches 1-2, 6-8 are small fixes which I found while trying to > solve the NONBLOCK issue [1]. Yes, I haven't given up on solving this > issue yet. I think it must be solved, since there is a bug somewhere. > WDYT? Yes I think this is an actual issue with the stream parser waking up sockets before the data is copied into the recv buffers.
On Thu, May 30, 2024 at 04:45 PM -07, John Fastabend wrote: > Geliang Tang wrote: >> On Mon, 2024-05-27 at 21:36 +0200, Jakub Sitnicki wrote: >> > On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote: >> > > Geliang Tang wrote: [...] >> > > The one advantage of test_sockmap is we can have it run for longer >> > > runs by pushing different options through so might be worth keeping >> > > just for that. >> > > >> > > If you really want links here I'm OK with that I guess just asking. >> > >> > It was me who suggested the switch to bpf_link in reaction to a >> > series >> > of cleanups to prog_type and prog_attach_type submitted by Geliang. >> >> Yes, patches 3-5 address Jakub's suggestion: switching attachments to >> bpf_link. > > OK. Lets just take them the series lgtm. Jakub any other comments? Gave it a run - all looks well. Thanks for the patches. Geliang, is there some MPTCP+sockmap use-case you're working towards?
On Fri, May 31, 2024 at 01:13:39PM +0200, Jakub Sitnicki wrote: > On Thu, May 30, 2024 at 04:45 PM -07, John Fastabend wrote: > > Geliang Tang wrote: > >> On Mon, 2024-05-27 at 21:36 +0200, Jakub Sitnicki wrote: > >> > On Mon, May 27, 2024 at 10:12 AM -07, John Fastabend wrote: > >> > > Geliang Tang wrote: > > [...] > > >> > > The one advantage of test_sockmap is we can have it run for longer > >> > > runs by pushing different options through so might be worth keeping > >> > > just for that. > >> > > > >> > > If you really want links here I'm OK with that I guess just asking. > >> > > >> > It was me who suggested the switch to bpf_link in reaction to a > >> > series > >> > of cleanups to prog_type and prog_attach_type submitted by Geliang. > >> > >> Yes, patches 3-5 address Jakub's suggestion: switching attachments to > >> bpf_link. > > > > OK. Lets just take them the series lgtm. Jakub any other comments? > > Gave it a run - all looks well. Thanks for the patches. > > Geliang, is there some MPTCP+sockmap use-case you're working towards? Yes, indeed. I have been working on a task related to MPTCP+sockmap recently, at least related to this test_sockmap.c selftest. We recently received an issue with MPTCP [1], that is TLS cannot be set on MPTCP sockets. The reason is that both MPTCP and TLS are implemented on TCP ULP. And each socket only supports one type of TCP ULP. I simply modified this test_sockmap.c selftest to support MPTCP, so that it can be used as the first version of test for MPTCP+TLS. So I spent some time reading and debugging this test. The development of MPTCP+TLS is still ongoing, and currently only setsockopt part has been successfully supported. The idea is simple, use an array of tcp_ulp_ops in a socket, instead of a single one: struct inet_connection_sock { ... ... const struct tcp_ulp_ops *icsk_ulp_ops[ULP_INDEX_MAX]; void __rcu *icsk_ulp_data[ULP_INDEX_MAX]; } The entire patch is in my commit "mptcp: tls support" [2]. It's not finish yet, but I really want to hear your opinions, especially John's. [1] https://github.com/multipath-tcp/mptcp_net-next/issues/480 [2] https://github.com/geliangtang/mptcp_net-next/commit/bba00a6cde75bab5a2c1c196d49812b4ed6addb0 Thanks, -Geliang
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c index e7dbf49a2ca6..d7581bbbc473 100644 --- a/tools/testing/selftests/bpf/test_sockmap.c +++ b/tools/testing/selftests/bpf/test_sockmap.c @@ -64,6 +64,8 @@ int failed; int map_fd[9]; struct bpf_map *maps[9]; int prog_fd[9]; +struct bpf_program *progs[9]; +struct bpf_link *links[9]; int txmsg_pass; int txmsg_redir; @@ -960,43 +962,39 @@ static int run_options(struct sockmap_options *options, int cg_fd, int test) /* Attach programs to sockmap */ if (!txmsg_omit_skb_parser) { - err = bpf_prog_attach(prog_fd[0], map_fd[0], - BPF_SK_SKB_STREAM_PARSER, 0); - if (err) { + links[0] = bpf_program__attach_sockmap(progs[0], map_fd[0]); + if (!links[0]) { fprintf(stderr, - "ERROR: bpf_prog_attach (sockmap %i->%i): %d (%s)\n", - prog_fd[0], map_fd[0], err, strerror(errno)); - return err; + "ERROR: bpf_program__attach_sockmap (sockmap %i->%i): (%s)\n", + bpf_program__fd(progs[0]), map_fd[0], strerror(errno)); + return -1; } } - err = bpf_prog_attach(prog_fd[1], map_fd[0], - BPF_SK_SKB_STREAM_VERDICT, 0); - if (err) { - fprintf(stderr, "ERROR: bpf_prog_attach (sockmap): %d (%s)\n", - err, strerror(errno)); - return err; + links[1] = bpf_program__attach_sockmap(progs[1], map_fd[0]); + if (!links[1]) { + fprintf(stderr, "ERROR: bpf_program__attach_sockmap (sockmap): (%s)\n", + strerror(errno)); + return -1; } /* Attach programs to TLS sockmap */ if (txmsg_ktls_skb) { if (!txmsg_omit_skb_parser) { - err = bpf_prog_attach(prog_fd[0], map_fd[8], - BPF_SK_SKB_STREAM_PARSER, 0); - if (err) { + links[2] = bpf_program__attach_sockmap(progs[0], map_fd[8]); + if (!links[2]) { fprintf(stderr, - "ERROR: bpf_prog_attach (TLS sockmap %i->%i): %d (%s)\n", - prog_fd[0], map_fd[8], err, strerror(errno)); - return err; + "ERROR: bpf_program__attach_sockmap (TLS sockmap %i->%i): (%s)\n", + bpf_program__fd(progs[0]), map_fd[8], strerror(errno)); + return -1; } } - err = bpf_prog_attach(prog_fd[2], map_fd[8], - BPF_SK_SKB_STREAM_VERDICT, 0); - if (err) { - fprintf(stderr, "ERROR: bpf_prog_attach (TLS sockmap): %d (%s)\n", - err, strerror(errno)); - return err; + links[3] = bpf_program__attach_sockmap(progs[2], map_fd[8]); + if (!links[3]) { + fprintf(stderr, "ERROR: bpf_program__attach_sockmap (TLS sockmap): (%s)\n", + strerror(errno)); + return -1; } } @@ -1281,10 +1279,11 @@ static int run_options(struct sockmap_options *options, int cg_fd, int test) out: /* Detatch and zero all the maps */ bpf_prog_detach2(prog_fd[3], cg_fd, BPF_CGROUP_SOCK_OPS); - bpf_prog_detach2(prog_fd[0], map_fd[0], BPF_SK_SKB_STREAM_PARSER); - bpf_prog_detach2(prog_fd[1], map_fd[0], BPF_SK_SKB_STREAM_VERDICT); - bpf_prog_detach2(prog_fd[0], map_fd[8], BPF_SK_SKB_STREAM_PARSER); - bpf_prog_detach2(prog_fd[2], map_fd[8], BPF_SK_SKB_STREAM_VERDICT); + + for (i = 0; i < ARRAY_SIZE(links); i++) { + if (links[i]) + bpf_link__detach(links[i]); + } if (tx_prog_fd > 0) bpf_prog_detach2(tx_prog_fd, map_fd[1], BPF_SK_MSG_VERDICT); @@ -1836,6 +1835,7 @@ static int populate_progs(char *bpf_file) i = bpf_object__load(obj); i = 0; bpf_object__for_each_program(prog, obj) { + progs[i] = prog; prog_fd[i] = bpf_program__fd(prog); i++; } @@ -1850,6 +1850,9 @@ static int populate_progs(char *bpf_file) } } + for (i = 0; i < ARRAY_SIZE(links); i++) + links[i] = NULL; + return 0; }