[bpf-next] bpf: Introduce "volatile compare" macro

Commit Message

Alexei Starovoitov Dec. 17, 2023, 1:06 a.m. UTC

From: Alexei Starovoitov <[e-mail]>

Compilers optimize conditional operators at will, but often bpf programmers
want to force compilers to keep the same operator in asm as it's written in C.
Introduce CMP(var1, conditional_op, var2) macro that can be used as:

-               if (seen >= 1000)
+               if (CMP(seen, >=, 1000))

The macro takes advantage of BPF assembly that is C like.

The macro checks the sign of variable 'seen' and emits either
signed or unsigned compare.

For example:
int a;
CMP(a, >, 0) will be translted to 'if rX s> 0 goto' in BPF assembly.

unsigned int a;
CMP(a, >, 0) will be translted to 'if rX > 0 goto' in BPF assembly.

The right hand side isn't checked yet. The macro needs more safety checks.

Signed-off-by: Alexei Starovoitov <[e-mail]>
---

As next step we can remove all of bpf_assert_ne|eq|... macros.
Also I'd like to remove bpf_assert_with. Open coding it is imo cleaner.

 .../testing/selftests/bpf/bpf_experimental.h  | 28 +++++++++++++++++++
 .../testing/selftests/bpf/progs/exceptions.c  | 20 ++++++-------
 .../selftests/bpf/progs/iters_task_vma.c      |  3 +-
 3 files changed, 39 insertions(+), 12 deletions(-)

Comments

Daniel Borkmann Dec. 18, 2023, 2:42 p.m. UTC | #1

On 12/17/23 2:06 AM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <[e-mail]>
> 
> Compilers optimize conditional operators at will, but often bpf programmers
> want to force compilers to keep the same operator in asm as it's written in C.
> Introduce CMP(var1, conditional_op, var2) macro that can be used as:
> 
> -               if (seen >= 1000)
> +               if (CMP(seen, >=, 1000))
> 
> The macro takes advantage of BPF assembly that is C like.
> 
> The macro checks the sign of variable 'seen' and emits either
> signed or unsigned compare.
> 
> For example:
> int a;
> CMP(a, >, 0) will be translted to 'if rX s> 0 goto' in BPF assembly.

nit: translted

> unsigned int a;
> CMP(a, >, 0) will be translted to 'if rX > 0 goto' in BPF assembly.

ditto

> The right hand side isn't checked yet. The macro needs more safety checks.
> 
> Signed-off-by: Alexei Starovoitov <[e-mail]>
> ---
> 
> As next step we can remove all of bpf_assert_ne|eq|... macros.
> Also I'd like to remove bpf_assert_with. Open coding it is imo cleaner.
> 
>   .../testing/selftests/bpf/bpf_experimental.h  | 28 +++++++++++++++++++
>   .../testing/selftests/bpf/progs/exceptions.c  | 20 ++++++-------
>   .../selftests/bpf/progs/iters_task_vma.c      |  3 +-
>   3 files changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
> index 1386baf9ae4a..a3248b086e4b 100644
> --- a/tools/testing/selftests/bpf/bpf_experimental.h
> +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> @@ -254,6 +254,34 @@ extern void bpf_throw(u64 cookie) __ksym;
>   		}									\
>   	 })
>   
> +#define __eauality(x) \
> +	__builtin_strcmp(#x, "==") == 0 || __builtin_strcmp(#x, "!=") == 0

nit: s/__eauality/__equality/ ?

Otherwise, looks good:

Acked-by: Daniel Borkmann <[e-mail]>

Patch

diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 1386baf9ae4a..a3248b086e4b 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -254,6 +254,34 @@  extern void bpf_throw(u64 cookie) __ksym;
 		}									\
 	 })
 
+#define __eauality(x) \
+	__builtin_strcmp(#x, "==") == 0 || __builtin_strcmp(#x, "!=") == 0
+
+#define is_signed_type(type) (((type)(-1)) < (type)1)
+
+#define __CMP(LHS, OP, SIGN, RHS) \
+	({ \
+		__label__ l_true; \
+		bool ret = true; \
+		asm volatile goto("if %[lhs] " SIGN #OP " %[rhs] goto %l[l_true]" \
+				  :: [lhs] "r"(LHS), [rhs] "ri"(RHS) :: l_true); \
+		ret = false; \
+l_true:\
+		ret;\
+       })
+
+#define CMP(LHS, OP, RHS) \
+	({ \
+		bool ret; \
+		if (__eauality(OP)) \
+			ret = __CMP(LHS, OP, "", RHS); \
+		else if (is_signed_type(typeof(LHS))) \
+			ret = __CMP(LHS, OP, "s", RHS); \
+		else \
+			ret = __CMP(LHS, OP, "", RHS); \
+		ret; \
+       })
+
 /* Description
  *	Assert that a conditional expression is true.
  * Returns
diff --git a/tools/testing/selftests/bpf/progs/exceptions.c b/tools/testing/selftests/bpf/progs/exceptions.c
index 2811ee842b01..4b55757eadbd 100644
--- a/tools/testing/selftests/bpf/progs/exceptions.c
+++ b/tools/testing/selftests/bpf/progs/exceptions.c
@@ -210,7 +210,7 @@  __noinline int assert_zero_gfunc(u64 c)
 {
 	volatile u64 cookie = c;
 
-	bpf_assert_eq(cookie, 0);
+	bpf_assert(CMP(cookie, ==, 0));
 	return 0;
 }
 
@@ -218,7 +218,7 @@  __noinline int assert_neg_gfunc(s64 c)
 {
 	volatile s64 cookie = c;
 
-	bpf_assert_lt(cookie, 0);
+	bpf_assert(CMP(cookie, <, 0));
 	return 0;
 }
 
@@ -226,7 +226,7 @@  __noinline int assert_pos_gfunc(s64 c)
 {
 	volatile s64 cookie = c;
 
-	bpf_assert_gt(cookie, 0);
+	bpf_assert(CMP(cookie, >, 0));
 	return 0;
 }
 
@@ -234,7 +234,7 @@  __noinline int assert_negeq_gfunc(s64 c)
 {
 	volatile s64 cookie = c;
 
-	bpf_assert_le(cookie, -1);
+	bpf_assert(CMP(cookie, <=, -1));
 	return 0;
 }
 
@@ -242,7 +242,7 @@  __noinline int assert_poseq_gfunc(s64 c)
 {
 	volatile s64 cookie = c;
 
-	bpf_assert_ge(cookie, 1);
+	bpf_assert(CMP(cookie, >=, 1));
 	return 0;
 }
 
@@ -258,7 +258,7 @@  __noinline int assert_zero_gfunc_with(u64 c)
 {
 	volatile u64 cookie = c;
 
-	bpf_assert_eq_with(cookie, 0, cookie + 100);
+	bpf_assert_with(CMP(cookie, ==, 0), cookie + 100);
 	return 0;
 }
 
@@ -266,7 +266,7 @@  __noinline int assert_neg_gfunc_with(s64 c)
 {
 	volatile s64 cookie = c;
 
-	bpf_assert_lt_with(cookie, 0, cookie + 100);
+	bpf_assert_with(CMP(cookie, <, 0), cookie + 100);
 	return 0;
 }
 
@@ -274,7 +274,7 @@  __noinline int assert_pos_gfunc_with(s64 c)
 {
 	volatile s64 cookie = c;
 
-	bpf_assert_gt_with(cookie, 0, cookie + 100);
+	bpf_assert_with(CMP(cookie, >, 0), cookie + 100);
 	return 0;
 }
 
@@ -282,7 +282,7 @@  __noinline int assert_negeq_gfunc_with(s64 c)
 {
 	volatile s64 cookie = c;
 
-	bpf_assert_le_with(cookie, -1, cookie + 100);
+	bpf_assert_with(CMP(cookie, <=, -1), cookie + 100);
 	return 0;
 }
 
@@ -290,7 +290,7 @@  __noinline int assert_poseq_gfunc_with(s64 c)
 {
 	volatile s64 cookie = c;
 
-	bpf_assert_ge_with(cookie, 1, cookie + 100);
+	bpf_assert_with(CMP(cookie, >=, 1), cookie + 100);
 	return 0;
 }
 
diff --git a/tools/testing/selftests/bpf/progs/iters_task_vma.c b/tools/testing/selftests/bpf/progs/iters_task_vma.c
index e085a51d153e..d85e29e979b7 100644
--- a/tools/testing/selftests/bpf/progs/iters_task_vma.c
+++ b/tools/testing/selftests/bpf/progs/iters_task_vma.c
@@ -28,9 +28,8 @@  int iter_task_vma_for_each(const void *ctx)
 		return 0;
 
 	bpf_for_each(task_vma, vma, task, 0) {
-		if (seen >= 1000)
+		if (CMP(seen, >=, 1000))
 			break;
-		barrier_var(seen);
 
 		vm_ranges[seen].vm_start = vma->vm_start;
 		vm_ranges[seen].vm_end = vma->vm_end;