* [PATCH v3 bpf-next 0/6] bpf: volatile compare
@ 2023-12-26 19:11 Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 1/6] selftests/bpf: Attempt to build BPF programs with -Wsign-compare Alexei Starovoitov
` (6 more replies)
0 siblings, 7 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2023-12-26 19:11 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, martin.lau, dxu, memxor, john.fastabend, jolsa,
kernel-team
From: Alexei Starovoitov <ast@kernel.org>
v2->v3:
Debugged profiler.c regression. It was caused by basic block layout.
Introduce bpf_cmp_likely() and bpf_cmp_unlikely() macros.
Debugged redundant <<=32, >>=32 with u32 variables. Added cast workaround.
v1->v2:
Fixed issues pointed out by Daniel, added more tests, attempted to convert profiler.c,
but barrier_var() wins vs bpf_cmp(). To be investigated.
Patches 1-4 are good to go, but 5 needs more work.
Alexei Starovoitov (6):
selftests/bpf: Attempt to build BPF programs with -Wsign-compare
bpf: Introduce "volatile compare" macros
selftests/bpf: Convert exceptions_assert.c to bpf_cmp
selftests/bpf: Remove bpf_assert_eq-like macros.
bpf: Add bpf_nop_mov() asm macro.
selftests/bpf: Convert profiler.c to bpf_cmp.
tools/testing/selftests/bpf/Makefile | 1 +
.../testing/selftests/bpf/bpf_experimental.h | 224 ++++++------------
.../bpf/progs/bpf_iter_bpf_percpu_hash_map.c | 2 +-
.../selftests/bpf/progs/bpf_iter_task_vmas.c | 2 +-
.../selftests/bpf/progs/bpf_iter_tasks.c | 2 +-
.../selftests/bpf/progs/bpf_iter_test_kern4.c | 2 +-
.../progs/cgroup_getset_retval_setsockopt.c | 2 +-
.../selftests/bpf/progs/cgrp_ls_sleepable.c | 2 +-
.../selftests/bpf/progs/cpumask_success.c | 2 +-
.../testing/selftests/bpf/progs/exceptions.c | 20 +-
.../selftests/bpf/progs/exceptions_assert.c | 80 +++----
tools/testing/selftests/bpf/progs/iters.c | 4 +-
.../selftests/bpf/progs/iters_task_vma.c | 3 +-
.../selftests/bpf/progs/linked_funcs1.c | 2 +-
.../selftests/bpf/progs/linked_funcs2.c | 2 +-
.../testing/selftests/bpf/progs/linked_list.c | 2 +-
.../selftests/bpf/progs/local_storage.c | 2 +-
tools/testing/selftests/bpf/progs/lsm.c | 2 +-
.../selftests/bpf/progs/normal_map_btf.c | 2 +-
.../selftests/bpf/progs/profiler.inc.h | 68 ++----
.../selftests/bpf/progs/sockopt_inherit.c | 2 +-
.../selftests/bpf/progs/sockopt_multi.c | 2 +-
.../selftests/bpf/progs/sockopt_qos_to_cc.c | 2 +-
.../testing/selftests/bpf/progs/test_bpf_ma.c | 2 +-
.../bpf/progs/test_core_reloc_kernel.c | 2 +-
.../bpf/progs/test_core_reloc_module.c | 8 +-
.../selftests/bpf/progs/test_fsverity.c | 2 +-
.../bpf/progs/test_skc_to_unix_sock.c | 2 +-
.../bpf/progs/test_xdp_do_redirect.c | 2 +-
29 files changed, 173 insertions(+), 277 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 bpf-next 1/6] selftests/bpf: Attempt to build BPF programs with -Wsign-compare
2023-12-26 19:11 [PATCH v3 bpf-next 0/6] bpf: volatile compare Alexei Starovoitov
@ 2023-12-26 19:11 ` Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 2/6] bpf: Introduce "volatile compare" macros Alexei Starovoitov
` (5 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2023-12-26 19:11 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, martin.lau, dxu, memxor, john.fastabend, jolsa,
kernel-team
From: Alexei Starovoitov <ast@kernel.org>
GCC's -Wall includes -Wsign-compare while clang does not.
Since BPF programs are built with clang we need to add this flag explicitly
to catch problematic comparisons like:
int i = -1;
unsigned int j = 1;
if (i < j) // this is false.
long i = -1;
unsigned int j = 1;
if (i < j) // this is true.
C standard for reference:
- If either operand is unsigned long the other shall be converted to unsigned long.
- Otherwise, if one operand is a long int and the other unsigned int, then if a
long int can represent all the values of an unsigned int, the unsigned int
shall be converted to a long int; otherwise both operands shall be converted to
unsigned long int.
- Otherwise, if either operand is long, the other shall be converted to long.
- Otherwise, if either operand is unsigned, the other shall be converted to unsigned.
Unfortunately clang's -Wsign-compare is very noisy.
It complains about (s32)a == (u32)b which is safe and doen't have surprising behavior.
This patch fixes some of the issues. It needs a follow up to fix the rest.
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/testing/selftests/bpf/Makefile | 1 +
.../selftests/bpf/progs/bpf_iter_bpf_percpu_hash_map.c | 2 +-
tools/testing/selftests/bpf/progs/bpf_iter_task_vmas.c | 2 +-
tools/testing/selftests/bpf/progs/bpf_iter_tasks.c | 2 +-
tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c | 2 +-
.../selftests/bpf/progs/cgroup_getset_retval_setsockopt.c | 2 +-
tools/testing/selftests/bpf/progs/cgrp_ls_sleepable.c | 2 +-
tools/testing/selftests/bpf/progs/cpumask_success.c | 2 +-
tools/testing/selftests/bpf/progs/iters.c | 4 ++--
tools/testing/selftests/bpf/progs/linked_funcs1.c | 2 +-
tools/testing/selftests/bpf/progs/linked_funcs2.c | 2 +-
tools/testing/selftests/bpf/progs/linked_list.c | 2 +-
tools/testing/selftests/bpf/progs/local_storage.c | 2 +-
tools/testing/selftests/bpf/progs/lsm.c | 2 +-
tools/testing/selftests/bpf/progs/normal_map_btf.c | 2 +-
tools/testing/selftests/bpf/progs/profiler.inc.h | 4 ++--
tools/testing/selftests/bpf/progs/sockopt_inherit.c | 2 +-
tools/testing/selftests/bpf/progs/sockopt_multi.c | 2 +-
tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c | 2 +-
tools/testing/selftests/bpf/progs/test_bpf_ma.c | 2 +-
.../testing/selftests/bpf/progs/test_core_reloc_kernel.c | 2 +-
.../testing/selftests/bpf/progs/test_core_reloc_module.c | 8 ++++----
tools/testing/selftests/bpf/progs/test_fsverity.c | 2 +-
tools/testing/selftests/bpf/progs/test_skc_to_unix_sock.c | 2 +-
tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c | 2 +-
25 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 617ae55c3bb5..fd15017ed3b1 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -383,6 +383,7 @@ CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG),$(CLANG_TARGET_ARCH))
BPF_CFLAGS = -g -Wall -Werror -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
-I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \
-I$(abspath $(OUTPUT)/../usr/include)
+# TODO: enable me -Wsign-compare
CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
-Wno-compare-distinct-pointer-types
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_percpu_hash_map.c b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_percpu_hash_map.c
index feaaa2b89c57..5014a17d6c02 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_percpu_hash_map.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_percpu_hash_map.c
@@ -20,7 +20,7 @@ struct {
} hashmap1 SEC(".maps");
/* will set before prog run */
-volatile const __u32 num_cpus = 0;
+volatile const __s32 num_cpus = 0;
/* will collect results during prog run */
__u32 key_sum_a = 0, key_sum_b = 0, key_sum_c = 0;
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_vmas.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_vmas.c
index dd923dc637d5..423b39e60b6f 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_task_vmas.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_vmas.c
@@ -35,7 +35,7 @@ SEC("iter/task_vma") int proc_maps(struct bpf_iter__task_vma *ctx)
return 0;
file = vma->vm_file;
- if (task->tgid != pid) {
+ if (task->tgid != (pid_t)pid) {
if (one_task)
one_task_error = 1;
return 0;
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
index 96131b9a1caa..6cbb3393f243 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_tasks.c
@@ -22,7 +22,7 @@ int dump_task(struct bpf_iter__task *ctx)
return 0;
}
- if (task->pid != tid)
+ if (task->pid != (pid_t)tid)
num_unknown_tid++;
else
num_known_tid++;
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
index 400fdf8d6233..dbf61c44acac 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
@@ -45,7 +45,7 @@ int dump_bpf_map(struct bpf_iter__bpf_map *ctx)
}
/* fill seq_file buffer */
- for (i = 0; i < print_len; i++)
+ for (i = 0; i < (int)print_len; i++)
bpf_seq_write(seq, &seq_num, sizeof(seq_num));
return ret;
diff --git a/tools/testing/selftests/bpf/progs/cgroup_getset_retval_setsockopt.c b/tools/testing/selftests/bpf/progs/cgroup_getset_retval_setsockopt.c
index b7fa8804e19d..45a0e9f492a9 100644
--- a/tools/testing/selftests/bpf/progs/cgroup_getset_retval_setsockopt.c
+++ b/tools/testing/selftests/bpf/progs/cgroup_getset_retval_setsockopt.c
@@ -11,7 +11,7 @@
__u32 invocations = 0;
__u32 assertion_error = 0;
__u32 retval_value = 0;
-__u32 page_size = 0;
+__s32 page_size = 0;
SEC("cgroup/setsockopt")
int get_retval(struct bpf_sockopt *ctx)
diff --git a/tools/testing/selftests/bpf/progs/cgrp_ls_sleepable.c b/tools/testing/selftests/bpf/progs/cgrp_ls_sleepable.c
index facedd8b8250..5e282c16eadc 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_ls_sleepable.c
+++ b/tools/testing/selftests/bpf/progs/cgrp_ls_sleepable.c
@@ -15,7 +15,7 @@ struct {
__type(value, long);
} map_a SEC(".maps");
-__u32 target_pid;
+__s32 target_pid;
__u64 cgroup_id;
int target_hid;
bool is_cgroup1;
diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
index fc3666edf456..7a1e64c6c065 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_success.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
@@ -332,7 +332,7 @@ SEC("tp_btf/task_newtask")
int BPF_PROG(test_copy_any_anyand, struct task_struct *task, u64 clone_flags)
{
struct bpf_cpumask *mask1, *mask2, *dst1, *dst2;
- u32 cpu;
+ int cpu;
if (!is_test_task())
return 0;
diff --git a/tools/testing/selftests/bpf/progs/iters.c b/tools/testing/selftests/bpf/progs/iters.c
index 3aca3dc145b5..fe971992e635 100644
--- a/tools/testing/selftests/bpf/progs/iters.c
+++ b/tools/testing/selftests/bpf/progs/iters.c
@@ -6,7 +6,7 @@
#include <bpf/bpf_helpers.h>
#include "bpf_misc.h"
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#define ARRAY_SIZE(x) (int)(sizeof(x) / sizeof((x)[0]))
static volatile int zero = 0;
@@ -676,7 +676,7 @@ static __noinline int sum(struct bpf_iter_num *it, int *arr, __u32 n)
while ((t = bpf_iter_num_next(it))) {
i = *t;
- if (i >= n)
+ if ((__u32)i >= n)
break;
sum += arr[i];
}
diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c
index c4b49ceea967..cc79dddac182 100644
--- a/tools/testing/selftests/bpf/progs/linked_funcs1.c
+++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c
@@ -8,7 +8,7 @@
#include "bpf_misc.h"
/* weak and shared between two files */
-const volatile int my_tid __weak;
+const volatile __u32 my_tid __weak;
long syscall_id __weak;
int output_val1;
diff --git a/tools/testing/selftests/bpf/progs/linked_funcs2.c b/tools/testing/selftests/bpf/progs/linked_funcs2.c
index 013ff0645f0c..942cc5526ddf 100644
--- a/tools/testing/selftests/bpf/progs/linked_funcs2.c
+++ b/tools/testing/selftests/bpf/progs/linked_funcs2.c
@@ -68,7 +68,7 @@ int BPF_PROG(handler2, struct pt_regs *regs, long id)
{
static volatile int whatever;
- if (my_tid != (u32)bpf_get_current_pid_tgid() || id != syscall_id)
+ if (my_tid != (s32)bpf_get_current_pid_tgid() || id != syscall_id)
return 0;
/* make sure we have CO-RE relocations in main program */
diff --git a/tools/testing/selftests/bpf/progs/linked_list.c b/tools/testing/selftests/bpf/progs/linked_list.c
index 84d1777a9e6c..26205ca80679 100644
--- a/tools/testing/selftests/bpf/progs/linked_list.c
+++ b/tools/testing/selftests/bpf/progs/linked_list.c
@@ -6,7 +6,7 @@
#include "bpf_experimental.h"
#ifndef ARRAY_SIZE
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#define ARRAY_SIZE(x) (int)(sizeof(x) / sizeof((x)[0]))
#endif
#include "linked_list.h"
diff --git a/tools/testing/selftests/bpf/progs/local_storage.c b/tools/testing/selftests/bpf/progs/local_storage.c
index bc8ea56671a1..e5e3a8b8dd07 100644
--- a/tools/testing/selftests/bpf/progs/local_storage.c
+++ b/tools/testing/selftests/bpf/progs/local_storage.c
@@ -13,7 +13,7 @@ char _license[] SEC("license") = "GPL";
#define DUMMY_STORAGE_VALUE 0xdeadbeef
-int monitored_pid = 0;
+__u32 monitored_pid = 0;
int inode_storage_result = -1;
int sk_storage_result = -1;
int task_storage_result = -1;
diff --git a/tools/testing/selftests/bpf/progs/lsm.c b/tools/testing/selftests/bpf/progs/lsm.c
index fadfdd98707c..0c13b7409947 100644
--- a/tools/testing/selftests/bpf/progs/lsm.c
+++ b/tools/testing/selftests/bpf/progs/lsm.c
@@ -92,7 +92,7 @@ int BPF_PROG(test_int_hook, struct vm_area_struct *vma,
if (ret != 0)
return ret;
- __u32 pid = bpf_get_current_pid_tgid() >> 32;
+ __s32 pid = bpf_get_current_pid_tgid() >> 32;
int is_stack = 0;
is_stack = (vma->vm_start <= vma->vm_mm->start_stack &&
diff --git a/tools/testing/selftests/bpf/progs/normal_map_btf.c b/tools/testing/selftests/bpf/progs/normal_map_btf.c
index 66cde82aa86d..a45c9299552c 100644
--- a/tools/testing/selftests/bpf/progs/normal_map_btf.c
+++ b/tools/testing/selftests/bpf/progs/normal_map_btf.c
@@ -36,7 +36,7 @@ int add_to_list_in_array(void *ctx)
struct node_data *new;
int zero = 0;
- if (done || (u32)bpf_get_current_pid_tgid() != pid)
+ if (done || (int)bpf_get_current_pid_tgid() != pid)
return 0;
value = bpf_map_lookup_elem(&array, &zero);
diff --git a/tools/testing/selftests/bpf/progs/profiler.inc.h b/tools/testing/selftests/bpf/progs/profiler.inc.h
index 897061930cb7..ba99d17dac54 100644
--- a/tools/testing/selftests/bpf/progs/profiler.inc.h
+++ b/tools/testing/selftests/bpf/progs/profiler.inc.h
@@ -132,7 +132,7 @@ struct {
} disallowed_exec_inodes SEC(".maps");
#ifndef ARRAY_SIZE
-#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
+#define ARRAY_SIZE(arr) (int)(sizeof(arr) / sizeof(arr[0]))
#endif
static INLINE bool IS_ERR(const void* ptr)
@@ -645,7 +645,7 @@ int raw_tracepoint__sched_process_exit(void* ctx)
for (int i = 0; i < ARRAY_SIZE(arr_struct->array); i++) {
struct var_kill_data_t* past_kill_data = &arr_struct->array[i];
- if (past_kill_data != NULL && past_kill_data->kill_target_pid == tpid) {
+ if (past_kill_data != NULL && past_kill_data->kill_target_pid == (pid_t)tpid) {
bpf_probe_read_kernel(kill_data, sizeof(*past_kill_data),
past_kill_data);
void* payload = kill_data->payload;
diff --git a/tools/testing/selftests/bpf/progs/sockopt_inherit.c b/tools/testing/selftests/bpf/progs/sockopt_inherit.c
index c8f59caa4639..a3434b840928 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_inherit.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_inherit.c
@@ -9,7 +9,7 @@ char _license[] SEC("license") = "GPL";
#define CUSTOM_INHERIT2 1
#define CUSTOM_LISTENER 2
-__u32 page_size = 0;
+__s32 page_size = 0;
struct sockopt_inherit {
__u8 val;
diff --git a/tools/testing/selftests/bpf/progs/sockopt_multi.c b/tools/testing/selftests/bpf/progs/sockopt_multi.c
index 96f29fce050b..db67278e12d4 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_multi.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_multi.c
@@ -5,7 +5,7 @@
char _license[] SEC("license") = "GPL";
-__u32 page_size = 0;
+__s32 page_size = 0;
SEC("cgroup/getsockopt")
int _getsockopt_child(struct bpf_sockopt *ctx)
diff --git a/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c b/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
index dbe235ede7f3..83753b00a556 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_qos_to_cc.c
@@ -9,7 +9,7 @@
char _license[] SEC("license") = "GPL";
-__u32 page_size = 0;
+__s32 page_size = 0;
SEC("cgroup/setsockopt")
int sockopt_qos_to_cc(struct bpf_sockopt *ctx)
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_ma.c b/tools/testing/selftests/bpf/progs/test_bpf_ma.c
index 069db9085e78..b78f4f702ae0 100644
--- a/tools/testing/selftests/bpf/progs/test_bpf_ma.c
+++ b/tools/testing/selftests/bpf/progs/test_bpf_ma.c
@@ -21,7 +21,7 @@ const unsigned int data_sizes[] = {16, 32, 64, 96, 128, 192, 256, 512, 1024, 204
const volatile unsigned int data_btf_ids[ARRAY_SIZE(data_sizes)] = {};
int err = 0;
-int pid = 0;
+u32 pid = 0;
#define DEFINE_ARRAY_WITH_KPTR(_size) \
struct bin_data_##_size { \
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
index a17dd83eae67..ee4a601dcb06 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c
@@ -53,7 +53,7 @@ int test_core_kernel(void *ctx)
struct task_struct *task = (void *)bpf_get_current_task();
struct core_reloc_kernel_output *out = (void *)&data.out;
uint64_t pid_tgid = bpf_get_current_pid_tgid();
- uint32_t real_tgid = (uint32_t)pid_tgid;
+ int32_t real_tgid = (int32_t)pid_tgid;
int pid, tgid;
if (data.my_pid_tgid != pid_tgid)
diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_module.c b/tools/testing/selftests/bpf/progs/test_core_reloc_module.c
index f59f175c7baf..bcb31ff92dcc 100644
--- a/tools/testing/selftests/bpf/progs/test_core_reloc_module.c
+++ b/tools/testing/selftests/bpf/progs/test_core_reloc_module.c
@@ -43,8 +43,8 @@ int BPF_PROG(test_core_module_probed,
#if __has_builtin(__builtin_preserve_enum_value)
struct core_reloc_module_output *out = (void *)&data.out;
__u64 pid_tgid = bpf_get_current_pid_tgid();
- __u32 real_tgid = (__u32)(pid_tgid >> 32);
- __u32 real_pid = (__u32)pid_tgid;
+ __s32 real_tgid = (__s32)(pid_tgid >> 32);
+ __s32 real_pid = (__s32)pid_tgid;
if (data.my_pid_tgid != pid_tgid)
return 0;
@@ -77,8 +77,8 @@ int BPF_PROG(test_core_module_direct,
#if __has_builtin(__builtin_preserve_enum_value)
struct core_reloc_module_output *out = (void *)&data.out;
__u64 pid_tgid = bpf_get_current_pid_tgid();
- __u32 real_tgid = (__u32)(pid_tgid >> 32);
- __u32 real_pid = (__u32)pid_tgid;
+ __s32 real_tgid = (__s32)(pid_tgid >> 32);
+ __s32 real_pid = (__s32)pid_tgid;
if (data.my_pid_tgid != pid_tgid)
return 0;
diff --git a/tools/testing/selftests/bpf/progs/test_fsverity.c b/tools/testing/selftests/bpf/progs/test_fsverity.c
index 3975495b75c8..9e0f73e8189c 100644
--- a/tools/testing/selftests/bpf/progs/test_fsverity.c
+++ b/tools/testing/selftests/bpf/progs/test_fsverity.c
@@ -38,7 +38,7 @@ int BPF_PROG(test_file_open, struct file *f)
return 0;
got_fsverity = 1;
- for (i = 0; i < sizeof(digest); i++) {
+ for (i = 0; i < (int)sizeof(digest); i++) {
if (digest[i] != expected_digest[i])
return 0;
}
diff --git a/tools/testing/selftests/bpf/progs/test_skc_to_unix_sock.c b/tools/testing/selftests/bpf/progs/test_skc_to_unix_sock.c
index eacda9fe07eb..4cfa42aa9436 100644
--- a/tools/testing/selftests/bpf/progs/test_skc_to_unix_sock.c
+++ b/tools/testing/selftests/bpf/progs/test_skc_to_unix_sock.c
@@ -29,7 +29,7 @@ int BPF_PROG(unix_listen, struct socket *sock, int backlog)
len = unix_sk->addr->len - sizeof(short);
path[0] = '@';
for (i = 1; i < len; i++) {
- if (i >= sizeof(struct sockaddr_un))
+ if (i >= (int)sizeof(struct sockaddr_un))
break;
path[i] = unix_sk->addr->name->sun_path[i];
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
index 5baaafed0d2d..3abf068b8446 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_do_redirect.c
@@ -38,7 +38,7 @@ int xdp_redirect(struct xdp_md *xdp)
if (payload + 1 > data_end)
return XDP_ABORTED;
- if (xdp->ingress_ifindex != ifindex_in)
+ if (xdp->ingress_ifindex != (__u32)ifindex_in)
return XDP_ABORTED;
if (metadata + 1 > data)
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 bpf-next 2/6] bpf: Introduce "volatile compare" macros
2023-12-26 19:11 [PATCH v3 bpf-next 0/6] bpf: volatile compare Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 1/6] selftests/bpf: Attempt to build BPF programs with -Wsign-compare Alexei Starovoitov
@ 2023-12-26 19:11 ` Alexei Starovoitov
2024-01-03 19:20 ` Andrii Nakryiko
2023-12-26 19:11 ` [PATCH v3 bpf-next 3/6] selftests/bpf: Convert exceptions_assert.c to bpf_cmp Alexei Starovoitov
` (4 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Alexei Starovoitov @ 2023-12-26 19:11 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, martin.lau, dxu, memxor, john.fastabend, jolsa,
kernel-team
From: Alexei Starovoitov <ast@kernel.org>
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 bpf_cmp_likely/unlikely(var1, conditional_op, var2) macros that can be used as:
- if (seen >= 1000)
+ if (bpf_cmp_unlikely(seen, >=, 1000))
The macros take advantage of BPF assembly that is C like.
The macros check the sign of variable 'seen' and emits either
signed or unsigned compare.
For example:
int a;
bpf_cmp_unlikely(a, >, 0) will be translated to 'if rX s> 0 goto' in BPF assembly.
unsigned int a;
bpf_cmp_unlikely(a, >, 0) will be translated to 'if rX > 0 goto' in BPF assembly.
C type conversions coupled with comparison operator are tricky.
int i = -1;
unsigned int j = 1;
if (i < j) // this is false.
long i = -1;
unsigned int j = 1;
if (i < j) // this is true.
Make sure BPF program is compiled with -Wsign-compare then the macros will catch
the mistake.
The macros check LHS (left hand side) only to figure out the sign of compare.
'if 0 < rX goto' is not allowed in the assembly, so the users
have to use a variable on LHS anyway.
The patch updates few tests to demonstrate the use of the macros.
The macro allows to use BPF_JSET in C code, since LLVM doesn't generate it at
present. For example:
if (i & j) compiles into r0 &= r1; if r0 == 0 goto
while
if (bpf_cmp_unlikely(i, &, j)) compiles into if r0 & r1 goto
Note that the macros has to be careful with RHS assembly predicate.
Since:
u64 __rhs = 1ull << 42;
asm goto("if r0 < %[rhs] goto +1" :: [rhs] "ri" (__rhs));
LLVM will silently truncate 64-bit constant into s32 imm.
Note that [lhs] "r"((short)LHS) the type cast is a workaround for LLVM issue.
When LHS is exactly 32-bit LLVM emits redundant <<=32, >>=32 to zero upper 32-bits.
When LHS is 64 or 16 or 8-bit variable there are no shifts.
When LHS is 32-bit the (u64) cast doesn't help. Hence use (short) cast.
It does _not_ truncate the variable before it's assigned to a register.
Traditional likely()/unlikely() macros that use __builtin_expect(!!(x), 1 or 0)
have no effect on these macros, hence macros implement the logic manually.
bpf_cmp_unlikely() macro preserves compare operator as-is while
bpf_cmp_likely() macro flips the compare.
Consider two cases:
A.
for() {
if (foo >= 10) {
bar += foo;
}
other code;
}
B.
for() {
if (foo >= 10)
break;
other code;
}
It's ok to use either bpf_cmp_likely or bpf_cmp_unlikely macros in both cases,
but consider that 'break' is effectively 'goto out_of_the_loop'.
Hence it's better to use bpf_cmp_unlikely in the B case.
While 'bar += foo' is better to keep as 'fallthrough' == likely code path in the A case.
When it's written as:
A.
for() {
if (bpf_cmp_likely(foo, >=, 10)) {
bar += foo;
}
other code;
}
B.
for() {
if (bpf_cmp_unlikely(foo, >=, 10))
break;
other code;
}
The assembly will look like:
A.
for() {
if r1 < 10 goto L1;
bar += foo;
L1:
other code;
}
B.
for() {
if r1 >= 10 goto L2;
other code;
}
L2:
The bpf_cmp_likely vs bpf_cmp_unlikely changes basic block layout, hence it will
greatly influence the verification process. The number of processed instructions
will be different, since the verifier walks the fallthrough first.
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
.../testing/selftests/bpf/bpf_experimental.h | 69 +++++++++++++++++++
.../testing/selftests/bpf/progs/exceptions.c | 20 +++---
.../selftests/bpf/progs/iters_task_vma.c | 3 +-
3 files changed, 80 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 1386baf9ae4a..789abf316ad4 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -254,6 +254,75 @@ extern void bpf_throw(u64 cookie) __ksym;
} \
})
+#define __cmp_cannot_be_signed(x) \
+ __builtin_strcmp(#x, "==") == 0 || __builtin_strcmp(#x, "!=") == 0 || \
+ __builtin_strcmp(#x, "&") == 0
+
+#define __is_signed_type(type) (((type)(-1)) < (type)1)
+
+#define __bpf_cmp(LHS, OP, SIGN, PRED, RHS, DEFAULT) \
+ ({ \
+ __label__ l_true; \
+ bool ret = DEFAULT; \
+ asm volatile goto("if %[lhs] " SIGN #OP " %[rhs] goto %l[l_true]" \
+ :: [lhs] "r"((short)LHS), [rhs] PRED (RHS) :: l_true); \
+ ret = !DEFAULT; \
+l_true:\
+ ret;\
+ })
+
+/* C type conversions coupled with comparison operator are tricky.
+ * Make sure BPF program is compiled with -Wsign-compre then
+ * __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) \
+ ({ \
+ typeof(LHS) __lhs = (LHS); \
+ typeof(RHS) __rhs = (RHS); \
+ bool ret; \
+ _Static_assert(sizeof(&(LHS)), "1st argument must be an lvalue expression"); \
+ (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); \
+ else \
+ ret = __bpf_cmp(__lhs, OP, "", "i", __rhs, NOFLIP); \
+ } else { \
+ if (sizeof(__rhs) == 8) \
+ ret = __bpf_cmp(__lhs, OP, "s", "r", __rhs, NOFLIP); \
+ else \
+ ret = __bpf_cmp(__lhs, OP, "s", "i", __rhs, NOFLIP); \
+ } \
+ ret; \
+ })
+
+#ifndef bpf_cmp_unlikely
+#define bpf_cmp_unlikely(LHS, OP, RHS) _bpf_cmp(LHS, OP, RHS, true)
+#endif
+
+#ifndef bpf_cmp_likely
+#define bpf_cmp_likely(LHS, OP, RHS) \
+ ({ \
+ bool ret; \
+ if (__builtin_strcmp(#OP, "==") == 0) \
+ ret = _bpf_cmp(LHS, !=, RHS, false); \
+ else if (__builtin_strcmp(#OP, "!=") == 0) \
+ ret = _bpf_cmp(LHS, ==, RHS, false); \
+ else if (__builtin_strcmp(#OP, "<=") == 0) \
+ ret = _bpf_cmp(LHS, >, RHS, false); \
+ else if (__builtin_strcmp(#OP, "<") == 0) \
+ ret = _bpf_cmp(LHS, >=, RHS, false); \
+ else if (__builtin_strcmp(#OP, ">") == 0) \
+ ret = _bpf_cmp(LHS, <=, RHS, false); \
+ else if (__builtin_strcmp(#OP, ">=") == 0) \
+ ret = _bpf_cmp(LHS, <, RHS, false); \
+ else \
+ (void) "bug"; \
+ ret; \
+ })
+#endif
+
/* 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..f09cd14d8e04 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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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(bpf_cmp_unlikely(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..dc0c3691dcc2 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 (bpf_cmp_unlikely(seen, >=, 1000))
break;
- barrier_var(seen);
vm_ranges[seen].vm_start = vma->vm_start;
vm_ranges[seen].vm_end = vma->vm_end;
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 bpf-next 3/6] selftests/bpf: Convert exceptions_assert.c to bpf_cmp
2023-12-26 19:11 [PATCH v3 bpf-next 0/6] bpf: volatile compare Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 1/6] selftests/bpf: Attempt to build BPF programs with -Wsign-compare Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 2/6] bpf: Introduce "volatile compare" macros Alexei Starovoitov
@ 2023-12-26 19:11 ` Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 4/6] selftests/bpf: Remove bpf_assert_eq-like macros Alexei Starovoitov
` (3 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2023-12-26 19:11 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, martin.lau, dxu, memxor, john.fastabend, jolsa,
kernel-team
From: Alexei Starovoitov <ast@kernel.org>
Convert exceptions_assert.c to bpf_cmp_unlikely() macro.
Since
bpf_assert(bpf_cmp_unlikely(var, ==, 100));
other code;
will generate assembly code:
if r1 == 100 goto L2;
r0 = 0
call bpf_throw
L1:
other code;
...
L2: goto L1;
LLVM generates redundant basic block with extra goto. LLVM will be fixed eventually.
Right now it's less efficient than __bpf_assert(var, ==, 100) macro that produces:
if r1 == 100 goto L1;
r0 = 0
call bpf_throw
L1:
other code;
But extra goto doesn't hurt the verification process.
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
.../selftests/bpf/progs/exceptions_assert.c | 80 +++++++++----------
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/exceptions_assert.c b/tools/testing/selftests/bpf/progs/exceptions_assert.c
index 0ef81040da59..5e0a1ca96d4e 100644
--- a/tools/testing/selftests/bpf/progs/exceptions_assert.c
+++ b/tools/testing/selftests/bpf/progs/exceptions_assert.c
@@ -11,51 +11,51 @@
#define check_assert(type, op, name, value) \
SEC("?tc") \
__log_level(2) __failure \
- int check_assert_##op##_##name(void *ctx) \
+ int check_assert_##name(void *ctx) \
{ \
type num = bpf_ktime_get_ns(); \
- bpf_assert_##op(num, value); \
+ bpf_assert(bpf_cmp_unlikely(num, op, value)); \
return *(u64 *)num; \
}
-__msg(": R0_w=0xffffffff80000000 R10=fp0")
-check_assert(s64, eq, int_min, INT_MIN);
-__msg(": R0_w=0x7fffffff R10=fp0")
-check_assert(s64, eq, int_max, INT_MAX);
-__msg(": R0_w=0 R10=fp0")
-check_assert(s64, eq, zero, 0);
-__msg(": R0_w=0x8000000000000000 R1_w=0x8000000000000000 R10=fp0")
-check_assert(s64, eq, llong_min, LLONG_MIN);
-__msg(": R0_w=0x7fffffffffffffff R1_w=0x7fffffffffffffff R10=fp0")
-check_assert(s64, eq, llong_max, LLONG_MAX);
-
-__msg(": R0_w=scalar(smax=0x7ffffffe) R10=fp0")
-check_assert(s64, lt, pos, INT_MAX);
-__msg(": R0_w=scalar(smax=-1,umin=0x8000000000000000,var_off=(0x8000000000000000; 0x7fffffffffffffff))")
-check_assert(s64, lt, zero, 0);
-__msg(": R0_w=scalar(smax=0xffffffff7fffffff,umin=0x8000000000000000,umax=0xffffffff7fffffff,var_off=(0x8000000000000000; 0x7fffffffffffffff))")
-check_assert(s64, lt, neg, INT_MIN);
-
-__msg(": R0_w=scalar(smax=0x7fffffff) R10=fp0")
-check_assert(s64, le, pos, INT_MAX);
-__msg(": R0_w=scalar(smax=0) R10=fp0")
-check_assert(s64, le, zero, 0);
-__msg(": R0_w=scalar(smax=0xffffffff80000000,umin=0x8000000000000000,umax=0xffffffff80000000,var_off=(0x8000000000000000; 0x7fffffffffffffff))")
-check_assert(s64, le, neg, INT_MIN);
-
-__msg(": R0_w=scalar(smin=umin=0x80000000,umax=0x7fffffffffffffff,var_off=(0x0; 0x7fffffffffffffff))")
-check_assert(s64, gt, pos, INT_MAX);
-__msg(": R0_w=scalar(smin=umin=1,umax=0x7fffffffffffffff,var_off=(0x0; 0x7fffffffffffffff))")
-check_assert(s64, gt, zero, 0);
-__msg(": R0_w=scalar(smin=0xffffffff80000001) R10=fp0")
-check_assert(s64, gt, neg, INT_MIN);
-
-__msg(": R0_w=scalar(smin=umin=0x7fffffff,umax=0x7fffffffffffffff,var_off=(0x0; 0x7fffffffffffffff))")
-check_assert(s64, ge, pos, INT_MAX);
-__msg(": R0_w=scalar(smin=0,umax=0x7fffffffffffffff,var_off=(0x0; 0x7fffffffffffffff)) R10=fp0")
-check_assert(s64, ge, zero, 0);
-__msg(": R0_w=scalar(smin=0xffffffff80000000) R10=fp0")
-check_assert(s64, ge, neg, INT_MIN);
+__msg(": R0_w=0xffffffff80000000")
+check_assert(s64, ==, eq_int_min, INT_MIN);
+__msg(": R0_w=0x7fffffff")
+check_assert(s64, ==, eq_int_max, INT_MAX);
+__msg(": R0_w=0")
+check_assert(s64, ==, eq_zero, 0);
+__msg(": R0_w=0x8000000000000000 R1_w=0x8000000000000000")
+check_assert(s64, ==, eq_llong_min, LLONG_MIN);
+__msg(": R0_w=0x7fffffffffffffff R1_w=0x7fffffffffffffff")
+check_assert(s64, ==, eq_llong_max, LLONG_MAX);
+
+__msg(": R0_w=scalar(id=1,smax=0x7ffffffe)")
+check_assert(s64, <, lt_pos, INT_MAX);
+__msg(": R0_w=scalar(id=1,smax=-1,umin=0x8000000000000000,var_off=(0x8000000000000000; 0x7fffffffffffffff))")
+check_assert(s64, <, lt_zero, 0);
+__msg(": R0_w=scalar(id=1,smax=0xffffffff7fffffff")
+check_assert(s64, <, lt_neg, INT_MIN);
+
+__msg(": R0_w=scalar(id=1,smax=0x7fffffff)")
+check_assert(s64, <=, le_pos, INT_MAX);
+__msg(": R0_w=scalar(id=1,smax=0)")
+check_assert(s64, <=, le_zero, 0);
+__msg(": R0_w=scalar(id=1,smax=0xffffffff80000000")
+check_assert(s64, <=, le_neg, INT_MIN);
+
+__msg(": R0_w=scalar(id=1,smin=umin=0x80000000,umax=0x7fffffffffffffff,var_off=(0x0; 0x7fffffffffffffff))")
+check_assert(s64, >, gt_pos, INT_MAX);
+__msg(": R0_w=scalar(id=1,smin=umin=1,umax=0x7fffffffffffffff,var_off=(0x0; 0x7fffffffffffffff))")
+check_assert(s64, >, gt_zero, 0);
+__msg(": R0_w=scalar(id=1,smin=0xffffffff80000001")
+check_assert(s64, >, gt_neg, INT_MIN);
+
+__msg(": R0_w=scalar(id=1,smin=umin=0x7fffffff,umax=0x7fffffffffffffff,var_off=(0x0; 0x7fffffffffffffff))")
+check_assert(s64, >=, ge_pos, INT_MAX);
+__msg(": R0_w=scalar(id=1,smin=0,umax=0x7fffffffffffffff,var_off=(0x0; 0x7fffffffffffffff))")
+check_assert(s64, >=, ge_zero, 0);
+__msg(": R0_w=scalar(id=1,smin=0xffffffff80000000")
+check_assert(s64, >=, ge_neg, INT_MIN);
SEC("?tc")
__log_level(2) __failure
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 bpf-next 4/6] selftests/bpf: Remove bpf_assert_eq-like macros.
2023-12-26 19:11 [PATCH v3 bpf-next 0/6] bpf: volatile compare Alexei Starovoitov
` (2 preceding siblings ...)
2023-12-26 19:11 ` [PATCH v3 bpf-next 3/6] selftests/bpf: Convert exceptions_assert.c to bpf_cmp Alexei Starovoitov
@ 2023-12-26 19:11 ` Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 5/6] bpf: Add bpf_nop_mov() asm macro Alexei Starovoitov
` (2 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2023-12-26 19:11 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, martin.lau, dxu, memxor, john.fastabend, jolsa,
kernel-team
From: Alexei Starovoitov <ast@kernel.org>
Since the last user was converted to bpf_cmp, remove bpf_assert_eq/ne/... macros.
__bpf_assert_op() macro is kept for experiments, since it's slightly more efficient
than bpf_assert(bpf_cmp_unlikely()) until LLVM is fixed.
Acked-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
.../testing/selftests/bpf/bpf_experimental.h | 150 ------------------
1 file changed, 150 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 789abf316ad4..4c5ba91fa55a 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -341,156 +341,6 @@ l_true:\
*/
#define bpf_assert_with(cond, value) if (!(cond)) bpf_throw(value);
-/* Description
- * Assert that LHS is equal to RHS. This statement updates the known value
- * of LHS during verification. Note that RHS must be a constant value, and
- * must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the value zero when the assertion fails.
- */
-#define bpf_assert_eq(LHS, RHS) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, ==, RHS, 0, true); \
- })
-
-/* Description
- * Assert that LHS is equal to RHS. This statement updates the known value
- * of LHS during verification. Note that RHS must be a constant value, and
- * must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the specified value when the assertion fails.
- */
-#define bpf_assert_eq_with(LHS, RHS, value) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, ==, RHS, value, true); \
- })
-
-/* Description
- * Assert that LHS is less than RHS. This statement updates the known
- * bounds of LHS during verification. Note that RHS must be a constant
- * value, and must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the value zero when the assertion fails.
- */
-#define bpf_assert_lt(LHS, RHS) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, <, RHS, 0, false); \
- })
-
-/* Description
- * Assert that LHS is less than RHS. This statement updates the known
- * bounds of LHS during verification. Note that RHS must be a constant
- * value, and must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the specified value when the assertion fails.
- */
-#define bpf_assert_lt_with(LHS, RHS, value) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, <, RHS, value, false); \
- })
-
-/* Description
- * Assert that LHS is greater than RHS. This statement updates the known
- * bounds of LHS during verification. Note that RHS must be a constant
- * value, and must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the value zero when the assertion fails.
- */
-#define bpf_assert_gt(LHS, RHS) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, >, RHS, 0, false); \
- })
-
-/* Description
- * Assert that LHS is greater than RHS. This statement updates the known
- * bounds of LHS during verification. Note that RHS must be a constant
- * value, and must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the specified value when the assertion fails.
- */
-#define bpf_assert_gt_with(LHS, RHS, value) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, >, RHS, value, false); \
- })
-
-/* Description
- * Assert that LHS is less than or equal to RHS. This statement updates the
- * known bounds of LHS during verification. Note that RHS must be a
- * constant value, and must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the value zero when the assertion fails.
- */
-#define bpf_assert_le(LHS, RHS) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, <=, RHS, 0, false); \
- })
-
-/* Description
- * Assert that LHS is less than or equal to RHS. This statement updates the
- * known bounds of LHS during verification. Note that RHS must be a
- * constant value, and must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the specified value when the assertion fails.
- */
-#define bpf_assert_le_with(LHS, RHS, value) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, <=, RHS, value, false); \
- })
-
-/* Description
- * Assert that LHS is greater than or equal to RHS. This statement updates
- * the known bounds of LHS during verification. Note that RHS must be a
- * constant value, and must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the value zero when the assertion fails.
- */
-#define bpf_assert_ge(LHS, RHS) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, >=, RHS, 0, false); \
- })
-
-/* Description
- * Assert that LHS is greater than or equal to RHS. This statement updates
- * the known bounds of LHS during verification. Note that RHS must be a
- * constant value, and must fit within the data type of LHS.
- * Returns
- * Void.
- * Throws
- * An exception with the specified value when the assertion fails.
- */
-#define bpf_assert_ge_with(LHS, RHS, value) \
- ({ \
- barrier_var(LHS); \
- __bpf_assert_op(LHS, >=, RHS, value, false); \
- })
-
/* Description
* Assert that LHS is in the range [BEG, END] (inclusive of both). This
* statement updates the known bounds of LHS during verification. Note
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 bpf-next 5/6] bpf: Add bpf_nop_mov() asm macro.
2023-12-26 19:11 [PATCH v3 bpf-next 0/6] bpf: volatile compare Alexei Starovoitov
` (3 preceding siblings ...)
2023-12-26 19:11 ` [PATCH v3 bpf-next 4/6] selftests/bpf: Remove bpf_assert_eq-like macros Alexei Starovoitov
@ 2023-12-26 19:11 ` Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 6/6] selftests/bpf: Convert profiler.c to bpf_cmp Alexei Starovoitov
2024-01-03 19:30 ` [PATCH v3 bpf-next 0/6] bpf: volatile compare patchwork-bot+netdevbpf
6 siblings, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2023-12-26 19:11 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, martin.lau, dxu, memxor, john.fastabend, jolsa,
kernel-team
From: Alexei Starovoitov <ast@kernel.org>
bpf_nop_mov(var) asm macro emits nop register move: rX = rX.
If 'var' is a scalar and not a fixed constant the verifier will assign ID to it.
If it's later spilled the stack slot will carry that ID as well.
Hence the range refining comparison "if rX < const" will update all copies
including spilled slot.
This macro is a temporary workaround until the verifier gets smarter.
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/testing/selftests/bpf/bpf_experimental.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 4c5ba91fa55a..6ecee9866970 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -323,6 +323,11 @@ l_true:\
})
#endif
+#ifndef bpf_nop_mov
+#define bpf_nop_mov(var) \
+ asm volatile("%[reg]=%[reg]"::[reg]"r"((short)var))
+#endif
+
/* Description
* Assert that a conditional expression is true.
* Returns
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 bpf-next 6/6] selftests/bpf: Convert profiler.c to bpf_cmp.
2023-12-26 19:11 [PATCH v3 bpf-next 0/6] bpf: volatile compare Alexei Starovoitov
` (4 preceding siblings ...)
2023-12-26 19:11 ` [PATCH v3 bpf-next 5/6] bpf: Add bpf_nop_mov() asm macro Alexei Starovoitov
@ 2023-12-26 19:11 ` Alexei Starovoitov
2024-01-03 19:30 ` [PATCH v3 bpf-next 0/6] bpf: volatile compare patchwork-bot+netdevbpf
6 siblings, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2023-12-26 19:11 UTC (permalink / raw)
To: bpf
Cc: daniel, andrii, martin.lau, dxu, memxor, john.fastabend, jolsa,
kernel-team
From: Alexei Starovoitov <ast@kernel.org>
Convert profiler[123].c to "volatile compare" to compare barrier_var() approach vs bpf_cmp_likely() vs bpf_cmp_unlikely().
bpf_cmp_unlikely() produces correct code, but takes much longer to verify:
./veristat -C -e prog,insns,states before after_with_unlikely
Program Insns (A) Insns (B) Insns (DIFF) States (A) States (B) States (DIFF)
------------------------------------ --------- --------- ------------------ ---------- ---------- -----------------
kprobe__proc_sys_write 1603 19606 +18003 (+1123.08%) 123 1678 +1555 (+1264.23%)
kprobe__vfs_link 11815 70305 +58490 (+495.05%) 971 4967 +3996 (+411.53%)
kprobe__vfs_symlink 5464 42896 +37432 (+685.07%) 434 3126 +2692 (+620.28%)
kprobe_ret__do_filp_open 5641 44578 +38937 (+690.25%) 446 3162 +2716 (+608.97%)
raw_tracepoint__sched_process_exec 2770 35962 +33192 (+1198.27%) 226 3121 +2895 (+1280.97%)
raw_tracepoint__sched_process_exit 1526 2135 +609 (+39.91%) 133 208 +75 (+56.39%)
raw_tracepoint__sched_process_fork 265 337 +72 (+27.17%) 19 24 +5 (+26.32%)
tracepoint__syscalls__sys_enter_kill 18782 140407 +121625 (+647.56%) 1286 12176 +10890 (+846.81%)
bpf_cmp_likely() is equivalent to barrier_var():
./veristat -C -e prog,insns,states before after_with_likely
Program Insns (A) Insns (B) Insns (DIFF) States (A) States (B) States (DIFF)
------------------------------------ --------- --------- -------------- ---------- ---------- -------------
kprobe__proc_sys_write 1603 1663 +60 (+3.74%) 123 127 +4 (+3.25%)
kprobe__vfs_link 11815 12090 +275 (+2.33%) 971 971 +0 (+0.00%)
kprobe__vfs_symlink 5464 5448 -16 (-0.29%) 434 426 -8 (-1.84%)
kprobe_ret__do_filp_open 5641 5739 +98 (+1.74%) 446 446 +0 (+0.00%)
raw_tracepoint__sched_process_exec 2770 2608 -162 (-5.85%) 226 216 -10 (-4.42%)
raw_tracepoint__sched_process_exit 1526 1526 +0 (+0.00%) 133 133 +0 (+0.00%)
raw_tracepoint__sched_process_fork 265 265 +0 (+0.00%) 19 19 +0 (+0.00%)
tracepoint__syscalls__sys_enter_kill 18782 18970 +188 (+1.00%) 1286 1286 +0 (+0.00%)
kprobe__proc_sys_write 2700 2809 +109 (+4.04%) 107 109 +2 (+1.87%)
kprobe__vfs_link 12238 12366 +128 (+1.05%) 267 269 +2 (+0.75%)
kprobe__vfs_symlink 7139 7365 +226 (+3.17%) 167 175 +8 (+4.79%)
kprobe_ret__do_filp_open 7264 7070 -194 (-2.67%) 180 182 +2 (+1.11%)
raw_tracepoint__sched_process_exec 3768 3453 -315 (-8.36%) 211 199 -12 (-5.69%)
raw_tracepoint__sched_process_exit 3138 3138 +0 (+0.00%) 83 83 +0 (+0.00%)
raw_tracepoint__sched_process_fork 265 265 +0 (+0.00%) 19 19 +0 (+0.00%)
tracepoint__syscalls__sys_enter_kill 26679 24327 -2352 (-8.82%) 1067 1037 -30 (-2.81%)
kprobe__proc_sys_write 1833 1833 +0 (+0.00%) 157 157 +0 (+0.00%)
kprobe__vfs_link 9995 10127 +132 (+1.32%) 803 803 +0 (+0.00%)
kprobe__vfs_symlink 5606 5672 +66 (+1.18%) 451 451 +0 (+0.00%)
kprobe_ret__do_filp_open 5716 5782 +66 (+1.15%) 462 462 +0 (+0.00%)
raw_tracepoint__sched_process_exec 3042 3042 +0 (+0.00%) 278 278 +0 (+0.00%)
raw_tracepoint__sched_process_exit 1680 1680 +0 (+0.00%) 146 146 +0 (+0.00%)
raw_tracepoint__sched_process_fork 299 299 +0 (+0.00%) 25 25 +0 (+0.00%)
tracepoint__syscalls__sys_enter_kill 18372 18372 +0 (+0.00%) 1558 1558 +0 (+0.00%)
default (mcpu=v3), no_alu32, cpuv4 have similar differences.
Note one place where bpf_nop_mov() is used to workaround the verifier lack of link
between the scalar register and its spill to stack.
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
.../selftests/bpf/progs/profiler.inc.h | 64 ++++++-------------
1 file changed, 18 insertions(+), 46 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/profiler.inc.h b/tools/testing/selftests/bpf/progs/profiler.inc.h
index ba99d17dac54..de3b6e4e4d0a 100644
--- a/tools/testing/selftests/bpf/progs/profiler.inc.h
+++ b/tools/testing/selftests/bpf/progs/profiler.inc.h
@@ -7,6 +7,7 @@
#include "profiler.h"
#include "err.h"
+#include "bpf_experimental.h"
#ifndef NULL
#define NULL 0
@@ -221,8 +222,7 @@ static INLINE void* read_full_cgroup_path(struct kernfs_node* cgroup_node,
return payload;
if (cgroup_node == cgroup_root_node)
*root_pos = payload - payload_start;
- if (filepart_length <= MAX_PATH) {
- barrier_var(filepart_length);
+ if (bpf_cmp_likely(filepart_length, <=, MAX_PATH)) {
payload += filepart_length;
}
cgroup_node = BPF_CORE_READ(cgroup_node, parent);
@@ -305,9 +305,7 @@ static INLINE void* populate_cgroup_info(struct cgroup_data_t* cgroup_data,
size_t cgroup_root_length =
bpf_probe_read_kernel_str(payload, MAX_PATH,
BPF_CORE_READ(root_kernfs, name));
- barrier_var(cgroup_root_length);
- if (cgroup_root_length <= MAX_PATH) {
- barrier_var(cgroup_root_length);
+ if (bpf_cmp_likely(cgroup_root_length, <=, MAX_PATH)) {
cgroup_data->cgroup_root_length = cgroup_root_length;
payload += cgroup_root_length;
}
@@ -315,9 +313,7 @@ static INLINE void* populate_cgroup_info(struct cgroup_data_t* cgroup_data,
size_t cgroup_proc_length =
bpf_probe_read_kernel_str(payload, MAX_PATH,
BPF_CORE_READ(proc_kernfs, name));
- barrier_var(cgroup_proc_length);
- if (cgroup_proc_length <= MAX_PATH) {
- barrier_var(cgroup_proc_length);
+ if (bpf_cmp_likely(cgroup_proc_length, <=, MAX_PATH)) {
cgroup_data->cgroup_proc_length = cgroup_proc_length;
payload += cgroup_proc_length;
}
@@ -347,9 +343,7 @@ static INLINE void* populate_var_metadata(struct var_metadata_t* metadata,
metadata->comm_length = 0;
size_t comm_length = bpf_core_read_str(payload, TASK_COMM_LEN, &task->comm);
- barrier_var(comm_length);
- if (comm_length <= TASK_COMM_LEN) {
- barrier_var(comm_length);
+ if (bpf_cmp_likely(comm_length, <=, TASK_COMM_LEN)) {
metadata->comm_length = comm_length;
payload += comm_length;
}
@@ -494,10 +488,9 @@ read_absolute_file_path_from_dentry(struct dentry* filp_dentry, void* payload)
filepart_length =
bpf_probe_read_kernel_str(payload, MAX_PATH,
BPF_CORE_READ(filp_dentry, d_name.name));
- barrier_var(filepart_length);
- if (filepart_length > MAX_PATH)
+ bpf_nop_mov(filepart_length);
+ if (bpf_cmp_unlikely(filepart_length, >, MAX_PATH))
break;
- barrier_var(filepart_length);
payload += filepart_length;
length += filepart_length;
@@ -579,9 +572,7 @@ ssize_t BPF_KPROBE(kprobe__proc_sys_write,
size_t sysctl_val_length = bpf_probe_read_kernel_str(payload,
CTL_MAXNAME, buf);
- barrier_var(sysctl_val_length);
- if (sysctl_val_length <= CTL_MAXNAME) {
- barrier_var(sysctl_val_length);
+ if (bpf_cmp_likely(sysctl_val_length, <=, CTL_MAXNAME)) {
sysctl_data->sysctl_val_length = sysctl_val_length;
payload += sysctl_val_length;
}
@@ -590,9 +581,7 @@ ssize_t BPF_KPROBE(kprobe__proc_sys_write,
bpf_probe_read_kernel_str(payload, MAX_PATH,
BPF_CORE_READ(filp, f_path.dentry,
d_name.name));
- barrier_var(sysctl_path_length);
- if (sysctl_path_length <= MAX_PATH) {
- barrier_var(sysctl_path_length);
+ if (bpf_cmp_likely(sysctl_path_length, <=, MAX_PATH)) {
sysctl_data->sysctl_path_length = sysctl_path_length;
payload += sysctl_path_length;
}
@@ -658,9 +647,7 @@ int raw_tracepoint__sched_process_exit(void* ctx)
kill_data->kill_target_cgroup_proc_length = 0;
size_t comm_length = bpf_core_read_str(payload, TASK_COMM_LEN, &task->comm);
- barrier_var(comm_length);
- if (comm_length <= TASK_COMM_LEN) {
- barrier_var(comm_length);
+ if (bpf_cmp_likely(comm_length, <=, TASK_COMM_LEN)) {
kill_data->kill_target_name_length = comm_length;
payload += comm_length;
}
@@ -669,9 +656,7 @@ int raw_tracepoint__sched_process_exit(void* ctx)
bpf_probe_read_kernel_str(payload,
KILL_TARGET_LEN,
BPF_CORE_READ(proc_kernfs, name));
- barrier_var(cgroup_proc_length);
- if (cgroup_proc_length <= KILL_TARGET_LEN) {
- barrier_var(cgroup_proc_length);
+ if (bpf_cmp_likely(cgroup_proc_length, <=, KILL_TARGET_LEN)) {
kill_data->kill_target_cgroup_proc_length = cgroup_proc_length;
payload += cgroup_proc_length;
}
@@ -731,9 +716,7 @@ int raw_tracepoint__sched_process_exec(struct bpf_raw_tracepoint_args* ctx)
const char* filename = BPF_CORE_READ(bprm, filename);
size_t bin_path_length =
bpf_probe_read_kernel_str(payload, MAX_FILENAME_LEN, filename);
- barrier_var(bin_path_length);
- if (bin_path_length <= MAX_FILENAME_LEN) {
- barrier_var(bin_path_length);
+ if (bpf_cmp_likely(bin_path_length, <=, MAX_FILENAME_LEN)) {
proc_exec_data->bin_path_length = bin_path_length;
payload += bin_path_length;
}
@@ -743,8 +726,7 @@ int raw_tracepoint__sched_process_exec(struct bpf_raw_tracepoint_args* ctx)
unsigned int cmdline_length = probe_read_lim(payload, arg_start,
arg_end - arg_start, MAX_ARGS_LEN);
- if (cmdline_length <= MAX_ARGS_LEN) {
- barrier_var(cmdline_length);
+ if (bpf_cmp_likely(cmdline_length, <=, MAX_ARGS_LEN)) {
proc_exec_data->cmdline_length = cmdline_length;
payload += cmdline_length;
}
@@ -821,9 +803,7 @@ int kprobe_ret__do_filp_open(struct pt_regs* ctx)
payload = populate_cgroup_info(&filemod_data->cgroup_data, task, payload);
size_t len = read_absolute_file_path_from_dentry(filp_dentry, payload);
- barrier_var(len);
- if (len <= MAX_FILEPATH_LENGTH) {
- barrier_var(len);
+ if (bpf_cmp_likely(len, <=, MAX_FILEPATH_LENGTH)) {
payload += len;
filemod_data->dst_filepath_length = len;
}
@@ -876,17 +856,13 @@ int BPF_KPROBE(kprobe__vfs_link,
payload = populate_cgroup_info(&filemod_data->cgroup_data, task, payload);
size_t len = read_absolute_file_path_from_dentry(old_dentry, payload);
- barrier_var(len);
- if (len <= MAX_FILEPATH_LENGTH) {
- barrier_var(len);
+ if (bpf_cmp_likely(len, <=, MAX_FILEPATH_LENGTH)) {
payload += len;
filemod_data->src_filepath_length = len;
}
len = read_absolute_file_path_from_dentry(new_dentry, payload);
- barrier_var(len);
- if (len <= MAX_FILEPATH_LENGTH) {
- barrier_var(len);
+ if (bpf_cmp_likely(len, <=, MAX_FILEPATH_LENGTH)) {
payload += len;
filemod_data->dst_filepath_length = len;
}
@@ -936,16 +912,12 @@ int BPF_KPROBE(kprobe__vfs_symlink, struct inode* dir, struct dentry* dentry,
size_t len = bpf_probe_read_kernel_str(payload, MAX_FILEPATH_LENGTH,
oldname);
- barrier_var(len);
- if (len <= MAX_FILEPATH_LENGTH) {
- barrier_var(len);
+ if (bpf_cmp_likely(len, <=, MAX_FILEPATH_LENGTH)) {
payload += len;
filemod_data->src_filepath_length = len;
}
len = read_absolute_file_path_from_dentry(dentry, payload);
- barrier_var(len);
- if (len <= MAX_FILEPATH_LENGTH) {
- barrier_var(len);
+ if (bpf_cmp_likely(len, <=, MAX_FILEPATH_LENGTH)) {
payload += len;
filemod_data->dst_filepath_length = len;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 bpf-next 2/6] bpf: Introduce "volatile compare" macros
2023-12-26 19:11 ` [PATCH v3 bpf-next 2/6] bpf: Introduce "volatile compare" macros Alexei Starovoitov
@ 2024-01-03 19:20 ` Andrii Nakryiko
2024-01-04 6:03 ` Alexei Starovoitov
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2024-01-03 19:20 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, daniel, andrii, martin.lau, dxu, memxor, john.fastabend,
jolsa, kernel-team
On Tue, Dec 26, 2023 at 11:12 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> 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 bpf_cmp_likely/unlikely(var1, conditional_op, var2) macros that can be used as:
>
> - if (seen >= 1000)
> + if (bpf_cmp_unlikely(seen, >=, 1000))
>
> The macros take advantage of BPF assembly that is C like.
>
> The macros check the sign of variable 'seen' and emits either
> signed or unsigned compare.
>
> For example:
> int a;
> bpf_cmp_unlikely(a, >, 0) will be translated to 'if rX s> 0 goto' in BPF assembly.
>
> unsigned int a;
> bpf_cmp_unlikely(a, >, 0) will be translated to 'if rX > 0 goto' in BPF assembly.
>
> C type conversions coupled with comparison operator are tricky.
> int i = -1;
> unsigned int j = 1;
> if (i < j) // this is false.
>
> long i = -1;
> unsigned int j = 1;
> if (i < j) // this is true.
>
> Make sure BPF program is compiled with -Wsign-compare then the macros will catch
> the mistake.
>
> The macros check LHS (left hand side) only to figure out the sign of compare.
>
> 'if 0 < rX goto' is not allowed in the assembly, so the users
> have to use a variable on LHS anyway.
>
> The patch updates few tests to demonstrate the use of the macros.
>
> The macro allows to use BPF_JSET in C code, since LLVM doesn't generate it at
> present. For example:
>
> if (i & j) compiles into r0 &= r1; if r0 == 0 goto
>
> while
>
> if (bpf_cmp_unlikely(i, &, j)) compiles into if r0 & r1 goto
>
> Note that the macros has to be careful with RHS assembly predicate.
> Since:
> u64 __rhs = 1ull << 42;
> asm goto("if r0 < %[rhs] goto +1" :: [rhs] "ri" (__rhs));
> LLVM will silently truncate 64-bit constant into s32 imm.
>
> Note that [lhs] "r"((short)LHS) the type cast is a workaround for LLVM issue.
> When LHS is exactly 32-bit LLVM emits redundant <<=32, >>=32 to zero upper 32-bits.
> When LHS is 64 or 16 or 8-bit variable there are no shifts.
> When LHS is 32-bit the (u64) cast doesn't help. Hence use (short) cast.
> It does _not_ truncate the variable before it's assigned to a register.
>
> Traditional likely()/unlikely() macros that use __builtin_expect(!!(x), 1 or 0)
> have no effect on these macros, hence macros implement the logic manually.
> bpf_cmp_unlikely() macro preserves compare operator as-is while
> bpf_cmp_likely() macro flips the compare.
>
> Consider two cases:
> A.
> for() {
> if (foo >= 10) {
> bar += foo;
> }
> other code;
> }
>
> B.
> for() {
> if (foo >= 10)
> break;
> other code;
> }
>
> It's ok to use either bpf_cmp_likely or bpf_cmp_unlikely macros in both cases,
> but consider that 'break' is effectively 'goto out_of_the_loop'.
> Hence it's better to use bpf_cmp_unlikely in the B case.
> While 'bar += foo' is better to keep as 'fallthrough' == likely code path in the A case.
>
> When it's written as:
> A.
> for() {
> if (bpf_cmp_likely(foo, >=, 10)) {
> bar += foo;
> }
> other code;
> }
>
> B.
> for() {
> if (bpf_cmp_unlikely(foo, >=, 10))
> break;
> other code;
> }
>
> The assembly will look like:
> A.
> for() {
> if r1 < 10 goto L1;
> bar += foo;
> L1:
> other code;
> }
>
> B.
> for() {
> if r1 >= 10 goto L2;
> other code;
> }
> L2:
>
> The bpf_cmp_likely vs bpf_cmp_unlikely changes basic block layout, hence it will
> greatly influence the verification process. The number of processed instructions
> will be different, since the verifier walks the fallthrough first.
>
> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
> .../testing/selftests/bpf/bpf_experimental.h | 69 +++++++++++++++++++
> .../testing/selftests/bpf/progs/exceptions.c | 20 +++---
> .../selftests/bpf/progs/iters_task_vma.c | 3 +-
> 3 files changed, 80 insertions(+), 12 deletions(-)
>
Likely/unlikely distinctions make 100% sense. Last year when I was
looking into improving verifier heuristics I noticed how sensitive and
important it is for verifier to first verify code path that is most
generic. This significantly aids state pruning. So code generation
makes a big difference, and if an "unlikely" short-cut (usually error
handling or early exit) condition is verified first, it will cause
verifier more work. Which totally explains the regressions you were
seeing in the last patch.
Let's hope users will actually make the right likely/unlikely decision
in their code when using bpf_cmp() :)
I left a few more comments below, but they can be a follow up. This
looks great, we should move this to bpf_helpers.h in libbpf after a
bit more testing in real-world production use cases.
I also aligned all those \ terminators in macros, as they are very
distracting when reading the code and I'd want to do that before
moving into bpf_helpers.h anyways :) hope you don't mind (and there
were some inconsistencies with single space and no space before \
anyways; it's hard to keep track of that)
> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
> index 1386baf9ae4a..789abf316ad4 100644
> --- a/tools/testing/selftests/bpf/bpf_experimental.h
> +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> @@ -254,6 +254,75 @@ extern void bpf_throw(u64 cookie) __ksym;
> } \
> })
>
> +#define __cmp_cannot_be_signed(x) \
> + __builtin_strcmp(#x, "==") == 0 || __builtin_strcmp(#x, "!=") == 0 || \
> + __builtin_strcmp(#x, "&") == 0
> +
> +#define __is_signed_type(type) (((type)(-1)) < (type)1)
> +
> +#define __bpf_cmp(LHS, OP, SIGN, PRED, RHS, DEFAULT) \
> + ({ \
> + __label__ l_true; \
> + bool ret = DEFAULT; \
> + asm volatile goto("if %[lhs] " SIGN #OP " %[rhs] goto %l[l_true]" \
> + :: [lhs] "r"((short)LHS), [rhs] PRED (RHS) :: l_true); \
> + ret = !DEFAULT; \
> +l_true:\
> + ret;\
> + })
> +
> +/* C type conversions coupled with comparison operator are tricky.
> + * Make sure BPF program is compiled with -Wsign-compre then
fixed typo while applying: compare
> + * __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) \
nit: NOFLIP name is absolutely confusing, tbh, it would be nice to
rename it to something more easily understood (DEFAULT is IMO better)
> + ({ \
> + typeof(LHS) __lhs = (LHS); \
> + typeof(RHS) __rhs = (RHS); \
> + bool ret; \
> + _Static_assert(sizeof(&(LHS)), "1st argument must be an lvalue expression"); \
> + (void)(__lhs OP __rhs); \
what is this part for? Is this what "__lhs OP __rhs below will catch
the mistake" in the comment refers to?
BTW, I think we hit this one when invalid OP is specified, before we
get to (void)"bug" (see below). So maybe it would be better to push it
deeper into __bpf_cmp itself?
> + if (__cmp_cannot_be_signed(OP) || !__is_signed_type(typeof(__lhs))) {\
> + if (sizeof(__rhs) == 8) \
> + ret = __bpf_cmp(__lhs, OP, "", "r", __rhs, NOFLIP); \
> + else \
> + ret = __bpf_cmp(__lhs, OP, "", "i", __rhs, NOFLIP); \
tbh, I'm also not 100% sure why this sizeof() == 8 and corresponding r
vs i change? Can you please elaborate (and add a comment in a follow
up, perhaps?)
> + } else { \
> + if (sizeof(__rhs) == 8) \
> + ret = __bpf_cmp(__lhs, OP, "s", "r", __rhs, NOFLIP); \
> + else \
> + ret = __bpf_cmp(__lhs, OP, "s", "i", __rhs, NOFLIP); \
one simplification that would reduce number of arguments to __bpf_cmp:
in __bpf_cmp (drop # before OP):
asm volatile goto("if %[lhs] " OP " %[rhs] goto %l[l_true]"
And here you just stringify operator, appending "s" if necessary:
ret = __bpf_cmp(__lhs, #OP, "i", __rhs, NOFLIP);
or
ret = __bpf_cmp(__lhs, "s"#OP, "r", __rhs, NOFLIP);
Consider for a follow up, if you agree it is a simplification.
> + } \
> + ret; \
> + })
> +
> +#ifndef bpf_cmp_unlikely
> +#define bpf_cmp_unlikely(LHS, OP, RHS) _bpf_cmp(LHS, OP, RHS, true)
> +#endif
> +
> +#ifndef bpf_cmp_likely
> +#define bpf_cmp_likely(LHS, OP, RHS) \
> + ({ \
> + bool ret; \
> + if (__builtin_strcmp(#OP, "==") == 0) \
> + ret = _bpf_cmp(LHS, !=, RHS, false); \
> + else if (__builtin_strcmp(#OP, "!=") == 0) \
> + ret = _bpf_cmp(LHS, ==, RHS, false); \
> + else if (__builtin_strcmp(#OP, "<=") == 0) \
> + ret = _bpf_cmp(LHS, >, RHS, false); \
> + else if (__builtin_strcmp(#OP, "<") == 0) \
> + ret = _bpf_cmp(LHS, >=, RHS, false); \
> + else if (__builtin_strcmp(#OP, ">") == 0) \
> + ret = _bpf_cmp(LHS, <=, RHS, false); \
> + else if (__builtin_strcmp(#OP, ">=") == 0) \
> + ret = _bpf_cmp(LHS, <, RHS, false); \
> + else \
> + (void) "bug"; \
doesn't seem like this is doing anything, I tried using wrong OP and I'm getting
progs/iters_task_vma.c:31:32: error: expected expression
31 | if (bpf_cmp_unlikely(seen, >==, 1000))
| ^
regardless of this (void)"bug" business. It doesn't hurt, but also
doesn't seem to be doing anything.
If I uses some text instead of operator, I get different one:
progs/iters_task_vma.c:31:30: error: expected ')'
31 | if (bpf_cmp_unlikely(seen, op, 1000))
| ^
progs/iters_task_vma.c:31:7: note: to match this '('
31 | if (bpf_cmp_unlikely(seen, op, 1000))
| ^
/data/users/andriin/linux/tools/testing/selftests/bpf/bpf_experimental.h:301:40:
note: expanded from macro 'bpf_cmp_unlikely'
301 | #define bpf_cmp_unlikely(LHS, OP, RHS) _bpf_cmp(LHS, OP, RHS, true)
| ^
/data/users/andriin/linux/tools/testing/selftests/bpf/bpf_experimental.h:285:9:
note: expanded from macro '_bpf_cmp'
285 | (void)(__lhs OP __rhs); \
| ^
1 error generated.
But still, (void)"bug" doesn't change anything (updated realization I
got while finishing this email: see below about difference between
likely/unlikely).
And just to complete exploration, if we use valid C operator that's
not supported in assembly (like <<), we can see different error still:
progs/iters_task_vma.c:31:7: error: invalid operand for instruction
31 | if (bpf_cmp_unlikely(seen, <<, 1000))
| ^
/data/users/andriin/linux/tools/testing/selftests/bpf/bpf_experimental.h:301:40:
note: expanded from macro 'bpf_cmp_unlikely'
301 | #define bpf_cmp_unlikely(LHS, OP, RHS) _bpf_cmp(LHS, OP, RHS, true)
| ^
/data/users/andriin/linux/tools/testing/selftests/bpf/bpf_experimental.h:290:11:
note: expanded from macro '_bpf_cmp'
290 | ret = __bpf_cmp(__lhs, OP, "",
"i", __rhs, NOFLIP); \
| ^
/data/users/andriin/linux/tools/testing/selftests/bpf/bpf_experimental.h:267:21:
note: expanded from macro '__bpf_cmp'
267 | asm volatile goto("if %[lhs] " SIGN #OP "
%[rhs] goto %l[l_true]" \
| ^
<inline asm>:1:8: note: instantiated into assembly here
1 | if r6 << 1000 goto LBB0_6
| ^
This one is kind of surprising, we got to asm even though the operator
was wrong. And it took me a bit to realize a huge asymmetry between
bpf_cmp_likely and bpf_cmp_unlikely. likely variant explicitly lists
supported operators and should bail out early on unrecognized one
((void)"bug") part. But unlikely variant just passes through
everything user provided directly into asm-generating macro.
I actually like a more verbose but also more explicit way of likely
implementation, listing explicitly supported operators. It will also
be easier for users to check what they are supposed to pass. So that's
another thing to maybe do in the follow up?
> + ret; \
> + })
> +#endif
> +
> /* Description
> * Assert that a conditional expression is true.
> * Returns
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 bpf-next 0/6] bpf: volatile compare
2023-12-26 19:11 [PATCH v3 bpf-next 0/6] bpf: volatile compare Alexei Starovoitov
` (5 preceding siblings ...)
2023-12-26 19:11 ` [PATCH v3 bpf-next 6/6] selftests/bpf: Convert profiler.c to bpf_cmp Alexei Starovoitov
@ 2024-01-03 19:30 ` patchwork-bot+netdevbpf
6 siblings, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-01-03 19:30 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, daniel, andrii, martin.lau, dxu, memxor, john.fastabend,
jolsa, kernel-team
Hello:
This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
On Tue, 26 Dec 2023 11:11:42 -0800 you wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> v2->v3:
> Debugged profiler.c regression. It was caused by basic block layout.
> Introduce bpf_cmp_likely() and bpf_cmp_unlikely() macros.
> Debugged redundant <<=32, >>=32 with u32 variables. Added cast workaround.
>
> [...]
Here is the summary with links:
- [v3,bpf-next,1/6] selftests/bpf: Attempt to build BPF programs with -Wsign-compare
https://git.kernel.org/bpf/bpf-next/c/495d2d8133fd
- [v3,bpf-next,2/6] bpf: Introduce "volatile compare" macros
https://git.kernel.org/bpf/bpf-next/c/a8b242d77bd7
- [v3,bpf-next,3/6] selftests/bpf: Convert exceptions_assert.c to bpf_cmp
https://git.kernel.org/bpf/bpf-next/c/624cd2a17672
- [v3,bpf-next,4/6] selftests/bpf: Remove bpf_assert_eq-like macros.
https://git.kernel.org/bpf/bpf-next/c/907dbd3ede5f
- [v3,bpf-next,5/6] bpf: Add bpf_nop_mov() asm macro.
https://git.kernel.org/bpf/bpf-next/c/0bcc62aa9813
- [v3,bpf-next,6/6] selftests/bpf: Convert profiler.c to bpf_cmp.
https://git.kernel.org/bpf/bpf-next/c/7e3811cb998f
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 bpf-next 2/6] bpf: Introduce "volatile compare" macros
2024-01-03 19:20 ` Andrii Nakryiko
@ 2024-01-04 6:03 ` Alexei Starovoitov
2024-01-04 19:04 ` Andrii Nakryiko
0 siblings, 1 reply; 11+ messages in thread
From: Alexei Starovoitov @ 2024-01-04 6:03 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau,
Daniel Xu, Kumar Kartikeya Dwivedi, John Fastabend, Jiri Olsa,
Kernel Team
On Wed, Jan 3, 2024 at 11:20 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> > +
> > +/* C type conversions coupled with comparison operator are tricky.
> > + * Make sure BPF program is compiled with -Wsign-compre then
>
> fixed typo while applying: compare
ohh. I cannot even copy-paste properly.
> > + * __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) \
>
> nit: NOFLIP name is absolutely confusing, tbh, it would be nice to
> rename it to something more easily understood (DEFAULT is IMO better)
I actually had it as DEFAULT, but it was less clear.
NOFLIP means whether the condition should be flipped or not.
DEFAULT is too ambiguous.
> > + ({ \
> > + typeof(LHS) __lhs = (LHS); \
> > + typeof(RHS) __rhs = (RHS); \
> > + bool ret; \
> > + _Static_assert(sizeof(&(LHS)), "1st argument must be an lvalue expression"); \
> > + (void)(__lhs OP __rhs); \
>
> what is this part for? Is this what "__lhs OP __rhs below will catch
> the mistake" in the comment refers to?
Yes. This one will give proper error either on GCC -Wall
or with clang -Wall -Wsign-compare.
>
> BTW, I think we hit this one when invalid OP is specified, before we
> get to (void)"bug" (see below). So maybe it would be better to push it
> deeper into __bpf_cmp itself?
>
> > + if (__cmp_cannot_be_signed(OP) || !__is_signed_type(typeof(__lhs))) {\
> > + if (sizeof(__rhs) == 8) \
> > + ret = __bpf_cmp(__lhs, OP, "", "r", __rhs, NOFLIP); \
> > + else \
> > + ret = __bpf_cmp(__lhs, OP, "", "i", __rhs, NOFLIP); \
>
> tbh, I'm also not 100% sure why this sizeof() == 8 and corresponding r
> vs i change? Can you please elaborate (and add a comment in a follow
> up, perhaps?)
That was in the commit log:
"
Note that the macros has to be careful with RHS assembly predicate.
Since:
u64 __rhs = 1ull << 42;
asm goto("if r0 < %[rhs] goto +1" :: [rhs] "ri" (__rhs));
LLVM will silently truncate 64-bit constant into s32 imm.
"
I will add a comment to the code as well.
>
> > + } else { \
> > + if (sizeof(__rhs) == 8) \
> > + ret = __bpf_cmp(__lhs, OP, "s", "r", __rhs, NOFLIP); \
> > + else \
> > + ret = __bpf_cmp(__lhs, OP, "s", "i", __rhs, NOFLIP); \
>
> one simplification that would reduce number of arguments to __bpf_cmp:
>
> in __bpf_cmp (drop # before OP):
>
> asm volatile goto("if %[lhs] " OP " %[rhs] goto %l[l_true]"
>
>
> And here you just stringify operator, appending "s" if necessary:
>
> ret = __bpf_cmp(__lhs, #OP, "i", __rhs, NOFLIP);
>
> or
>
> ret = __bpf_cmp(__lhs, "s"#OP, "r", __rhs, NOFLIP);
>
> Consider for a follow up, if you agree it is a simplification.
Just to reduce the number of arguments? I will give it a try.
> I actually like a more verbose but also more explicit way of likely
> implementation, listing explicitly supported operators. It will also
> be easier for users to check what they are supposed to pass. So that's
> another thing to maybe do in the follow up?
I thought about making unlikely explicit, but it looked weird and noisy:
if (__builtin_strcmp(#OP, "==") == 0 ||
__builtin_strcmp(#OP, "!=") == 0 ||
__builtin_strcmp(#OP, "<") == 0 ||
...
and all the noise just to make unlikely match likely.
I felt it's not worth it and the error in the current form as you noticed:
> progs/iters_task_vma.c:31:7: error: invalid operand for instruction
> 31 | if (bpf_cmp_unlikely(seen, <<, 1000))
> <inline asm>:1:8: note: instantiated into assembly here
> 1 | if r6 << 1000 goto LBB0_6
is much cleaner instead of (void) return.
I've tried many different ways to have a helper macro
#define flip_cmp_opcode(OP)
and use it inside the bpf_cmp, but couldn't make it work.
This is the shortest version of macros I came up with.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 bpf-next 2/6] bpf: Introduce "volatile compare" macros
2024-01-04 6:03 ` Alexei Starovoitov
@ 2024-01-04 19:04 ` Andrii Nakryiko
0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2024-01-04 19:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau,
Daniel Xu, Kumar Kartikeya Dwivedi, John Fastabend, Jiri Olsa,
Kernel Team
On Wed, Jan 3, 2024 at 10:04 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jan 3, 2024 at 11:20 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > > +
> > > +/* C type conversions coupled with comparison operator are tricky.
> > > + * Make sure BPF program is compiled with -Wsign-compre then
> >
> > fixed typo while applying: compare
>
> ohh. I cannot even copy-paste properly.
>
> > > + * __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) \
> >
> > nit: NOFLIP name is absolutely confusing, tbh, it would be nice to
> > rename it to something more easily understood (DEFAULT is IMO better)
>
> I actually had it as DEFAULT, but it was less clear.
> NOFLIP means whether the condition should be flipped or not.
> DEFAULT is too ambiguous.
LIKELY (or UNLIKELY, whichever makes sense) might be another
suggestion. But it's minor, so feel free to ignore.
>
> > > + ({ \
> > > + typeof(LHS) __lhs = (LHS); \
> > > + typeof(RHS) __rhs = (RHS); \
> > > + bool ret; \
> > > + _Static_assert(sizeof(&(LHS)), "1st argument must be an lvalue expression"); \
> > > + (void)(__lhs OP __rhs); \
> >
> > what is this part for? Is this what "__lhs OP __rhs below will catch
> > the mistake" in the comment refers to?
>
> Yes. This one will give proper error either on GCC -Wall
> or with clang -Wall -Wsign-compare.
>
> >
> > BTW, I think we hit this one when invalid OP is specified, before we
> > get to (void)"bug" (see below). So maybe it would be better to push it
> > deeper into __bpf_cmp itself?
> >
> > > + if (__cmp_cannot_be_signed(OP) || !__is_signed_type(typeof(__lhs))) {\
> > > + if (sizeof(__rhs) == 8) \
> > > + ret = __bpf_cmp(__lhs, OP, "", "r", __rhs, NOFLIP); \
> > > + else \
> > > + ret = __bpf_cmp(__lhs, OP, "", "i", __rhs, NOFLIP); \
> >
> > tbh, I'm also not 100% sure why this sizeof() == 8 and corresponding r
> > vs i change? Can you please elaborate (and add a comment in a follow
> > up, perhaps?)
>
> That was in the commit log:
> "
> Note that the macros has to be careful with RHS assembly predicate.
> Since:
> u64 __rhs = 1ull << 42;
> asm goto("if r0 < %[rhs] goto +1" :: [rhs] "ri" (__rhs));
> LLVM will silently truncate 64-bit constant into s32 imm.
> "
>
> I will add a comment to the code as well.
ah, ok, it didn't click for me, thanks for adding a comment
while on the topic, is there a problem specifying "ri" for sizeof() <
8 case? At least for alu32 cases, shouldn't we allow w1 < w2 kind of
situations?
>
> >
> > > + } else { \
> > > + if (sizeof(__rhs) == 8) \
> > > + ret = __bpf_cmp(__lhs, OP, "s", "r", __rhs, NOFLIP); \
> > > + else \
> > > + ret = __bpf_cmp(__lhs, OP, "s", "i", __rhs, NOFLIP); \
> >
> > one simplification that would reduce number of arguments to __bpf_cmp:
> >
> > in __bpf_cmp (drop # before OP):
> >
> > asm volatile goto("if %[lhs] " OP " %[rhs] goto %l[l_true]"
> >
> >
> > And here you just stringify operator, appending "s" if necessary:
> >
> > ret = __bpf_cmp(__lhs, #OP, "i", __rhs, NOFLIP);
> >
> > or
> >
> > ret = __bpf_cmp(__lhs, "s"#OP, "r", __rhs, NOFLIP);
> >
> > Consider for a follow up, if you agree it is a simplification.
>
> Just to reduce the number of arguments? I will give it a try.
yeah, pretty much just for that
>
> > I actually like a more verbose but also more explicit way of likely
> > implementation, listing explicitly supported operators. It will also
> > be easier for users to check what they are supposed to pass. So that's
> > another thing to maybe do in the follow up?
>
> I thought about making unlikely explicit, but it looked weird and noisy:
> if (__builtin_strcmp(#OP, "==") == 0 ||
> __builtin_strcmp(#OP, "!=") == 0 ||
> __builtin_strcmp(#OP, "<") == 0 ||
> ...
> and all the noise just to make unlikely match likely.
>
> I felt it's not worth it and the error in the current form as you noticed:
>
> > progs/iters_task_vma.c:31:7: error: invalid operand for instruction
> > 31 | if (bpf_cmp_unlikely(seen, <<, 1000))
> > <inline asm>:1:8: note: instantiated into assembly here
> > 1 | if r6 << 1000 goto LBB0_6
>
> is much cleaner instead of (void) return.
Right, I only played with unlikely last time. Trying the same with
likely right now, it's not that great:
In file included from progs/profiler2.c:6:
progs/profiler.inc.h:225:7: error: variable 'ret' is uninitialized
when used here [-Werror,-Wuninitialized]
225 | if (bpf_cmp_likely(filepart_length, <==, MAX_PATH)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/data/users/andriin/linux/tools/testing/selftests/bpf/bpf_experimental.h:322:3:
note: expanded from macro 'bpf_cmp_likely'
322 | ret;
\
| ^~~
progs/profiler.inc.h:225:7: note: variable 'ret' is declared here
/data/users/andriin/linux/tools/testing/selftests/bpf/bpf_experimental.h:307:3:
note: expanded from macro 'bpf_cmp_likely'
307 | bool ret;
\
| ^
I tried adding _Static_assert, and it is triggered for all valid cases
as well, so it seems like the compiler doesn't detect this last branch
as dead code, unfortunately.
>
> I've tried many different ways to have a helper macro
> #define flip_cmp_opcode(OP)
> and use it inside the bpf_cmp, but couldn't make it work.
> This is the shortest version of macros I came up with.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-01-04 19:05 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-26 19:11 [PATCH v3 bpf-next 0/6] bpf: volatile compare Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 1/6] selftests/bpf: Attempt to build BPF programs with -Wsign-compare Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 2/6] bpf: Introduce "volatile compare" macros Alexei Starovoitov
2024-01-03 19:20 ` Andrii Nakryiko
2024-01-04 6:03 ` Alexei Starovoitov
2024-01-04 19:04 ` Andrii Nakryiko
2023-12-26 19:11 ` [PATCH v3 bpf-next 3/6] selftests/bpf: Convert exceptions_assert.c to bpf_cmp Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 4/6] selftests/bpf: Remove bpf_assert_eq-like macros Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 5/6] bpf: Add bpf_nop_mov() asm macro Alexei Starovoitov
2023-12-26 19:11 ` [PATCH v3 bpf-next 6/6] selftests/bpf: Convert profiler.c to bpf_cmp Alexei Starovoitov
2024-01-03 19:30 ` [PATCH v3 bpf-next 0/6] bpf: volatile compare patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox