diff mbox series

[bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf

Message ID 20240522080936.2475833-1-jakub@cloudflare.com (mailing list archive)
State Accepted
Commit 46253c4ae96162a840ad65c1394de63796d7798a
Delegated to: BPF
Headers show
Series [bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
netdev/tree_selection success Clearly marked for bpf-next
netdev/apply fail Patch does not apply to bpf-next-0

Commit Message

Jakub Sitnicki May 22, 2024, 8:09 a.m. UTC 
libbpf can deduce program type and attach type from the ELF section name.
We don't need to pass it out-of-band if we switch to libbpf convention [1].

[1] https://docs.kernel.org/bpf/libbpf/program_types.html

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../selftests/bpf/progs/test_sockmap_kern.h   | 17 +++++-----
 tools/testing/selftests/bpf/test_sockmap.c    | 31 -------------------
 2 files changed, 9 insertions(+), 39 deletions(-)

Comments

Geliang Tang May 22, 2024, 10:12 a.m. UTC | #1 
Hi Jakub,

On Wed, 2024-05-22 at 10:09 +0200, Jakub Sitnicki wrote:
> libbpf can deduce program type and attach type from the ELF section
> name.
> We don't need to pass it out-of-band if we switch to libbpf
> convention [1].
> 
> [1] https://docs.kernel.org/bpf/libbpf/program_types.html
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>

Thanks for this patch, it works well. But ...

> ---
>  .../selftests/bpf/progs/test_sockmap_kern.h   | 17 +++++-----
>  tools/testing/selftests/bpf/test_sockmap.c    | 31 -----------------
> --
>  2 files changed, 9 insertions(+), 39 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> index 99d2ea9fb658..3dff0813730b 100644
> --- a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> +++ b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
> @@ -92,7 +92,7 @@ struct {
>  	__uint(value_size, sizeof(int));
>  } tls_sock_map SEC(".maps");
>  
> -SEC("sk_skb1")
> +SEC("sk_skb/stream_parser")
>  int bpf_prog1(struct __sk_buff *skb)
>  {
>  	int *f, two = 2;
> @@ -104,7 +104,7 @@ int bpf_prog1(struct __sk_buff *skb)
>  	return skb->len;
>  }
>  
> -SEC("sk_skb2")
> +SEC("sk_skb/stream_verdict")
>  int bpf_prog2(struct __sk_buff *skb)
>  {
>  	__u32 lport = skb->local_port;
> @@ -151,7 +151,7 @@ static inline void bpf_write_pass(struct
> __sk_buff *skb, int offset)
>  		memcpy(c + offset, "PASS", 4);
>  }
>  
> -SEC("sk_skb3")
> +SEC("sk_skb/stream_verdict")
>  int bpf_prog3(struct __sk_buff *skb)
>  {
>  	int err, *f, ret = SK_PASS;
> @@ -233,7 +233,7 @@ int bpf_sockmap(struct bpf_sock_ops *skops)
>  	return 0;
>  }
>  
> -SEC("sk_msg1")
> +SEC("sk_msg")
>  int bpf_prog4(struct sk_msg_md *msg)
>  {
>  	int *bytes, zero = 0, one = 1, two = 2, three = 3, four = 4,
> five = 5;
> @@ -263,7 +263,7 @@ int bpf_prog4(struct sk_msg_md *msg)
>  	return SK_PASS;
>  }
>  
> -SEC("sk_msg2")
> +SEC("sk_msg")
>  int bpf_prog6(struct sk_msg_md *msg)
>  {
>  	int zero = 0, one = 1, two = 2, three = 3, four = 4, five =
> 5, key = 0;
> @@ -308,7 +308,7 @@ int bpf_prog6(struct sk_msg_md *msg)
>  #endif
>  }
>  
> -SEC("sk_msg3")
> +SEC("sk_msg")
>  int bpf_prog8(struct sk_msg_md *msg)
>  {
>  	void *data_end = (void *)(long) msg->data_end;
> @@ -329,7 +329,8 @@ int bpf_prog8(struct sk_msg_md *msg)
>  
>  	return SK_PASS;
>  }
> -SEC("sk_msg4")
> +
> +SEC("sk_msg")
>  int bpf_prog9(struct sk_msg_md *msg)
>  {
>  	void *data_end = (void *)(long) msg->data_end;
> @@ -347,7 +348,7 @@ int bpf_prog9(struct sk_msg_md *msg)
>  	return SK_PASS;
>  }
>  
> -SEC("sk_msg5")
> +SEC("sk_msg")
>  int bpf_prog10(struct sk_msg_md *msg)
>  {
>  	int *bytes, *start, *end, *start_push, *end_push,
> *start_pop, *pop;
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c
> b/tools/testing/selftests/bpf/test_sockmap.c
> index 4499b3cfc3a6..ddc6a9cef36f 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -1783,30 +1783,6 @@ char *map_names[] = {
>  	"tls_sock_map",
>  };
>  
> -int prog_attach_type[] = {
> -	BPF_SK_SKB_STREAM_PARSER,
> -	BPF_SK_SKB_STREAM_VERDICT,
> -	BPF_SK_SKB_STREAM_VERDICT,
> -	BPF_CGROUP_SOCK_OPS,
> -	BPF_SK_MSG_VERDICT,
> -	BPF_SK_MSG_VERDICT,
> -	BPF_SK_MSG_VERDICT,
> -	BPF_SK_MSG_VERDICT,
> -	BPF_SK_MSG_VERDICT,
> -};

prog_attach_type is still used by my commit:

https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn/T/#u

Please review it for me.

If my commit is acceptable, this patch will conflict with it. It's a
bit strange to delete this prog_attach_type in your patch and then add
it back in my commit. So could you please rebase this patch on my
commit in that case. Sorry for the trouble.

Anyway please add my tag:

Acked-and-tested-by: Geliang Tang <geliang@kernel.org> 

Thanks,
-Geliang

> -
> -int prog_type[] = {
> -	BPF_PROG_TYPE_SK_SKB,
> -	BPF_PROG_TYPE_SK_SKB,
> -	BPF_PROG_TYPE_SK_SKB,
> -	BPF_PROG_TYPE_SOCK_OPS,
> -	BPF_PROG_TYPE_SK_MSG,
> -	BPF_PROG_TYPE_SK_MSG,
> -	BPF_PROG_TYPE_SK_MSG,
> -	BPF_PROG_TYPE_SK_MSG,
> -	BPF_PROG_TYPE_SK_MSG,
> -};
> -
>  static int populate_progs(char *bpf_file)
>  {
>  	struct bpf_program *prog;
> @@ -1825,13 +1801,6 @@ static int populate_progs(char *bpf_file)
>  		return -1;
>  	}
>  
> -	bpf_object__for_each_program(prog, obj) {
> -		bpf_program__set_type(prog, prog_type[i]);
> -		bpf_program__set_expected_attach_type(prog,
> -						     
> prog_attach_type[i]);
> -		i++;
> -	}
> -
>  	i = bpf_object__load(obj);
>  	i = 0;
>  	bpf_object__for_each_program(prog, obj) {
Jakub Sitnicki May 22, 2024, 10:26 a.m. UTC | #2 
On Wed, May 22, 2024 at 06:12 PM +08, Geliang Tang wrote:
> On Wed, 2024-05-22 at 10:09 +0200, Jakub Sitnicki wrote:

[...]

> prog_attach_type is still used by my commit:
>
> https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn/T/#u
>
> Please review it for me.
>
> If my commit is acceptable, this patch will conflict with it. It's a
> bit strange to delete this prog_attach_type in your patch and then add
> it back in my commit. So could you please rebase this patch on my
> commit in that case. Sorry for the trouble.

If you want to help improve and modernize this test code, I suggest
switching attachments to bpf_link instead, they are now available for
sockmap:

https://lore.kernel.org/bpf/20240408152451.4162024-1-yonghong.song@linux.dev/
Geliang Tang May 23, 2024, 6:57 a.m. UTC | #3 
Hi Jakub,

On Wed, 2024-05-22 at 12:26 +0200, Jakub Sitnicki wrote:
> On Wed, May 22, 2024 at 06:12 PM +08, Geliang Tang wrote:
> > On Wed, 2024-05-22 at 10:09 +0200, Jakub Sitnicki wrote:
> 
> [...]
> 
> > prog_attach_type is still used by my commit:
> > 
> > https://lore.kernel.org/bpf/e27d7d0c1e0e79b0acd22ac6ad5d8f9f00225303.1716372485.git.tanggeliang@kylinos.cn/T/#u

I changes my commit above as "Rejected". So this patch is ready to
apply now.

> > Please review it for me.
> > 
> > If my commit is acceptable, this patch will conflict with it. It's
> > a
> > bit strange to delete this prog_attach_type in your patch and then
> > add
> > it back in my commit. So could you please rebase this patch on my
> > commit in that case. Sorry for the trouble.
> 
> If you want to help improve and modernize this test code, I suggest
> switching attachments to bpf_link instead, they are now available for
> sockmap:

Here's the patches for switching attachments to bpf_link, together with
some small fixes and cleanups:

https://patchwork.kernel.org/project/netdevbpf/cover/cover.1716446893.git.tanggeliang@kylinos.cn/

Please review them for me.

Thanks,
-Geliang

> 
> https://lore.kernel.org/bpf/20240408152451.4162024-1-yonghong.song@linux.dev/
>
patchwork-bot+netdevbpf@kernel.org May 30, 2024, 9:50 p.m. UTC | #4 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Wed, 22 May 2024 10:09:36 +0200 you wrote:
> libbpf can deduce program type and attach type from the ELF section name.
> We don't need to pass it out-of-band if we switch to libbpf convention [1].
> 
> [1] https://docs.kernel.org/bpf/libbpf/program_types.html
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: test_sockmap, use section names understood by libbpf
    https://git.kernel.org/bpf/bpf-next/c/46253c4ae961

You are awesome, thank you!
Tony Ambardar July 4, 2024, 5:03 a.m. UTC | #5 
Hi Jakub, Daniel,

On Wed, May 22, 2024 at 10:09:36AM +0200, Jakub Sitnicki wrote:
> libbpf can deduce program type and attach type from the ELF section name.
> We don't need to pass it out-of-band if we switch to libbpf convention [1].
> 
> [1] https://docs.kernel.org/bpf/libbpf/program_types.html
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  .../selftests/bpf/progs/test_sockmap_kern.h   | 17 +++++-----
>  tools/testing/selftests/bpf/test_sockmap.c    | 31 -------------------
>  2 files changed, 9 insertions(+), 39 deletions(-)
> 
[ snip ]

I recently hit some CI errors on kernel-patches/bpf related to veristat
and the test_sockmap BPF object files:

https://github.com/kernel-patches/bpf/actions/runs/9775040227/job/26985293772?pr=7279

I hadn't seen your submission, and after some investigation my own fix
ended up reproducing your patch: <sigh...>

https://github.com/kernel-patches/bpf/pull/7279/commits/9e5bc23d4a3643e6a4733aac431ee425310e03d6

Given it fixes a CI issue for bpf/master, could we backport/apply your
patch there? 

Thanks,
Tony
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
index 99d2ea9fb658..3dff0813730b 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_kern.h
@@ -92,7 +92,7 @@  struct {
 	__uint(value_size, sizeof(int));
 } tls_sock_map SEC(".maps");
 
-SEC("sk_skb1")
+SEC("sk_skb/stream_parser")
 int bpf_prog1(struct __sk_buff *skb)
 {
 	int *f, two = 2;
@@ -104,7 +104,7 @@  int bpf_prog1(struct __sk_buff *skb)
 	return skb->len;
 }
 
-SEC("sk_skb2")
+SEC("sk_skb/stream_verdict")
 int bpf_prog2(struct __sk_buff *skb)
 {
 	__u32 lport = skb->local_port;
@@ -151,7 +151,7 @@  static inline void bpf_write_pass(struct __sk_buff *skb, int offset)
 		memcpy(c + offset, "PASS", 4);
 }
 
-SEC("sk_skb3")
+SEC("sk_skb/stream_verdict")
 int bpf_prog3(struct __sk_buff *skb)
 {
 	int err, *f, ret = SK_PASS;
@@ -233,7 +233,7 @@  int bpf_sockmap(struct bpf_sock_ops *skops)
 	return 0;
 }
 
-SEC("sk_msg1")
+SEC("sk_msg")
 int bpf_prog4(struct sk_msg_md *msg)
 {
 	int *bytes, zero = 0, one = 1, two = 2, three = 3, four = 4, five = 5;
@@ -263,7 +263,7 @@  int bpf_prog4(struct sk_msg_md *msg)
 	return SK_PASS;
 }
 
-SEC("sk_msg2")
+SEC("sk_msg")
 int bpf_prog6(struct sk_msg_md *msg)
 {
 	int zero = 0, one = 1, two = 2, three = 3, four = 4, five = 5, key = 0;
@@ -308,7 +308,7 @@  int bpf_prog6(struct sk_msg_md *msg)
 #endif
 }
 
-SEC("sk_msg3")
+SEC("sk_msg")
 int bpf_prog8(struct sk_msg_md *msg)
 {
 	void *data_end = (void *)(long) msg->data_end;
@@ -329,7 +329,8 @@  int bpf_prog8(struct sk_msg_md *msg)
 
 	return SK_PASS;
 }
-SEC("sk_msg4")
+
+SEC("sk_msg")
 int bpf_prog9(struct sk_msg_md *msg)
 {
 	void *data_end = (void *)(long) msg->data_end;
@@ -347,7 +348,7 @@  int bpf_prog9(struct sk_msg_md *msg)
 	return SK_PASS;
 }
 
-SEC("sk_msg5")
+SEC("sk_msg")
 int bpf_prog10(struct sk_msg_md *msg)
 {
 	int *bytes, *start, *end, *start_push, *end_push, *start_pop, *pop;
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 4499b3cfc3a6..ddc6a9cef36f 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1783,30 +1783,6 @@  char *map_names[] = {
 	"tls_sock_map",
 };
 
-int prog_attach_type[] = {
-	BPF_SK_SKB_STREAM_PARSER,
-	BPF_SK_SKB_STREAM_VERDICT,
-	BPF_SK_SKB_STREAM_VERDICT,
-	BPF_CGROUP_SOCK_OPS,
-	BPF_SK_MSG_VERDICT,
-	BPF_SK_MSG_VERDICT,
-	BPF_SK_MSG_VERDICT,
-	BPF_SK_MSG_VERDICT,
-	BPF_SK_MSG_VERDICT,
-};
-
-int prog_type[] = {
-	BPF_PROG_TYPE_SK_SKB,
-	BPF_PROG_TYPE_SK_SKB,
-	BPF_PROG_TYPE_SK_SKB,
-	BPF_PROG_TYPE_SOCK_OPS,
-	BPF_PROG_TYPE_SK_MSG,
-	BPF_PROG_TYPE_SK_MSG,
-	BPF_PROG_TYPE_SK_MSG,
-	BPF_PROG_TYPE_SK_MSG,
-	BPF_PROG_TYPE_SK_MSG,
-};
-
 static int populate_progs(char *bpf_file)
 {
 	struct bpf_program *prog;
@@ -1825,13 +1801,6 @@  static int populate_progs(char *bpf_file)
 		return -1;
 	}
 
-	bpf_object__for_each_program(prog, obj) {
-		bpf_program__set_type(prog, prog_type[i]);
-		bpf_program__set_expected_attach_type(prog,
-						      prog_attach_type[i]);
-		i++;
-	}
-
 	i = bpf_object__load(obj);
 	i = 0;
 	bpf_object__for_each_program(prog, obj) {