diff mbox series

[bpf-next] bpf: Minor improvements for bpf_cmp.

Message ID 20240112220134.71209-1-alexei.starovoitov@gmail.com (mailing list archive)
State Accepted
Commit 7f0aa0dd07e49a1c0fff09e98756d5ef5a23cd3c
Delegated to: BPF
Headers show
Series [bpf-next] bpf: Minor improvements for bpf_cmp. | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: Avoid unnecessary line continuations WARNING: Macros with flow control statements should be avoided WARNING: line length of 105 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-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-10 success Logs for aarch64-gcc / veristat
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-12 success Logs for s390x-gcc / build-release
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-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 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
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
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-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
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-34 success Logs for x86_64-llvm-17 / veristat
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-42 success Logs for x86_64-llvm-18 / veristat
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-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
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-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-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-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-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-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 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-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-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-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-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-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
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-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-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-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-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-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-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Alexei Starovoitov Jan. 12, 2024, 10:01 p.m. UTC 
From: Alexei Starovoitov <ast@kernel.org>

Few minor improvements for bpf_cmp() macro:
. reduce number of args in __bpf_cmp()
. rename NOFLIP to UNLIKELY
. add a comment about 64-bit truncation in "i" constraint
. use "ri" constraint for sizeof(rhs) <= 4
. improve error message for bpf_cmp_likely()

Before:
progs/iters_task_vma.c:31:7: error: variable 'ret' is uninitialized when used here [-Werror,-Wuninitialized]
   31 |                 if (bpf_cmp_likely(seen, <==, 1000))
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../bpf/bpf_experimental.h:325:3: note: expanded from macro 'bpf_cmp_likely'
  325 |                 ret;
      |                 ^~~
progs/iters_task_vma.c:31:7: note: variable 'ret' is declared here
../bpf/bpf_experimental.h:310:3: note: expanded from macro 'bpf_cmp_likely'
  310 |                 bool ret;
      |                 ^

After:
progs/iters_task_vma.c:31:7: error: invalid operand for instruction
   31 |                 if (bpf_cmp_likely(seen, <==, 1000))
      |                     ^
../bpf/bpf_experimental.h:324:17: note: expanded from macro 'bpf_cmp_likely'
  324 |                         asm volatile("r0 " #OP " invalid compare");
      |                                      ^
<inline asm>:1:5: note: instantiated into assembly here
    1 |         r0 <== invalid compare
      |            ^

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 .../testing/selftests/bpf/bpf_experimental.h  | 21 +++++++++++--------
 1 file changed, 12 insertions(+), 9 deletions(-)

Comments

Yonghong Song Jan. 13, 2024, 1:24 a.m. UTC | #1 
On 1/12/24 2:01 PM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> Few minor improvements for bpf_cmp() macro:
> . reduce number of args in __bpf_cmp()
> . rename NOFLIP to UNLIKELY
> . add a comment about 64-bit truncation in "i" constraint
> . use "ri" constraint for sizeof(rhs) <= 4
> . improve error message for bpf_cmp_likely()
>
> Before:
> progs/iters_task_vma.c:31:7: error: variable 'ret' is uninitialized when used here [-Werror,-Wuninitialized]
>     31 |                 if (bpf_cmp_likely(seen, <==, 1000))
>        |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../bpf/bpf_experimental.h:325:3: note: expanded from macro 'bpf_cmp_likely'
>    325 |                 ret;
>        |                 ^~~
> progs/iters_task_vma.c:31:7: note: variable 'ret' is declared here
> ../bpf/bpf_experimental.h:310:3: note: expanded from macro 'bpf_cmp_likely'
>    310 |                 bool ret;
>        |                 ^
>
> After:
> progs/iters_task_vma.c:31:7: error: invalid operand for instruction
>     31 |                 if (bpf_cmp_likely(seen, <==, 1000))
>        |                     ^
> ../bpf/bpf_experimental.h:324:17: note: expanded from macro 'bpf_cmp_likely'
>    324 |                         asm volatile("r0 " #OP " invalid compare");
>        |                                      ^
> <inline asm>:1:5: note: instantiated into assembly here
>      1 |         r0 <== invalid compare
>        |            ^
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Acked-by: Yonghong Song <yonghong.song@linux.dev>
patchwork-bot+netdevbpf@kernel.org Jan. 15, 2024, 5:10 p.m. UTC | #2 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Fri, 12 Jan 2024 14:01:34 -0800 you wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> Few minor improvements for bpf_cmp() macro:
> . reduce number of args in __bpf_cmp()
> . rename NOFLIP to UNLIKELY
> . add a comment about 64-bit truncation in "i" constraint
> . use "ri" constraint for sizeof(rhs) <= 4
> . improve error message for bpf_cmp_likely()
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: Minor improvements for bpf_cmp.
    https://git.kernel.org/bpf/bpf-next/c/7f0aa0dd07e4

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index f44875f8b367..0d749006d107 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -260,11 +260,11 @@  extern void bpf_throw(u64 cookie) __ksym;
 
 #define __is_signed_type(type) (((type)(-1)) < (type)1)
 
-#define __bpf_cmp(LHS, OP, SIGN, PRED, RHS, DEFAULT)						\
+#define __bpf_cmp(LHS, OP, PRED, RHS, DEFAULT)						\
 	({											\
 		__label__ l_true;								\
 		bool ret = DEFAULT;								\
-		asm volatile goto("if %[lhs] " SIGN #OP " %[rhs] goto %l[l_true]"		\
+		asm volatile goto("if %[lhs] " OP " %[rhs] goto %l[l_true]"		\
 				  :: [lhs] "r"((short)LHS), [rhs] PRED (RHS) :: l_true);	\
 		ret = !DEFAULT;									\
 l_true:												\
@@ -276,7 +276,7 @@  l_true:												\
  * __lhs OP __rhs below will catch the mistake.
  * Be aware that we check only __lhs to figure out the sign of compare.
  */
-#define _bpf_cmp(LHS, OP, RHS, NOFLIP)								\
+#define _bpf_cmp(LHS, OP, RHS, UNLIKELY)								\
 	({											\
 		typeof(LHS) __lhs = (LHS);							\
 		typeof(RHS) __rhs = (RHS);							\
@@ -285,14 +285,17 @@  l_true:												\
 		(void)(__lhs OP __rhs);								\
 		if (__cmp_cannot_be_signed(OP) || !__is_signed_type(typeof(__lhs))) {		\
 			if (sizeof(__rhs) == 8)							\
-				ret = __bpf_cmp(__lhs, OP, "", "r", __rhs, NOFLIP);		\
+				/* "i" will truncate 64-bit constant into s32,			\
+				 * so we have to use extra register via "r".			\
+				 */								\
+				ret = __bpf_cmp(__lhs, #OP, "r", __rhs, UNLIKELY);		\
 			else									\
-				ret = __bpf_cmp(__lhs, OP, "", "i", __rhs, NOFLIP);		\
+				ret = __bpf_cmp(__lhs, #OP, "ri", __rhs, UNLIKELY);		\
 		} else {									\
 			if (sizeof(__rhs) == 8)							\
-				ret = __bpf_cmp(__lhs, OP, "s", "r", __rhs, NOFLIP);		\
+				ret = __bpf_cmp(__lhs, "s"#OP, "r", __rhs, UNLIKELY);		\
 			else									\
-				ret = __bpf_cmp(__lhs, OP, "s", "i", __rhs, NOFLIP);		\
+				ret = __bpf_cmp(__lhs, "s"#OP, "ri", __rhs, UNLIKELY);		\
 		}										\
 		ret;										\
        })
@@ -304,7 +307,7 @@  l_true:												\
 #ifndef bpf_cmp_likely
 #define bpf_cmp_likely(LHS, OP, RHS)								\
 	({											\
-		bool ret;									\
+		bool ret = 0;									\
 		if (__builtin_strcmp(#OP, "==") == 0)						\
 			ret = _bpf_cmp(LHS, !=, RHS, false);					\
 		else if (__builtin_strcmp(#OP, "!=") == 0)					\
@@ -318,7 +321,7 @@  l_true:												\
 		else if (__builtin_strcmp(#OP, ">=") == 0)					\
 			ret = _bpf_cmp(LHS, <, RHS, false);					\
 		else										\
-			(void) "bug";								\
+			asm volatile("r0 " #OP " invalid compare");				\
 		ret;										\
        })
 #endif