[PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[Vadim Fedorenko]

New kfunc to return ARCH-specific timecounter. For x86 BPF JIT converts
it into rdtsc ordered call. Other architectures will get JIT
implementation too if supported. The fallback is to
__arch_get_hw_counter().

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
v1 -> v2:
* Fix incorrect function return value type to u64
* Introduce bpf_jit_inlines_kfunc_call() and use it in
  mark_fastcall_pattern_for_call() to avoid clobbering in case of
	running programs with no JIT (Eduard)
* Avoid rewriting instruction and check function pointer directly
  in JIT (Alexei)
* Change includes to fix compile issues on non x86 architectures
---
 arch/x86/net/bpf_jit_comp.c   | 30 ++++++++++++++++++++++++++++++
 arch/x86/net/bpf_jit_comp32.c | 16 ++++++++++++++++
 include/linux/filter.h        |  1 +
 kernel/bpf/core.c             | 11 +++++++++++
 kernel/bpf/helpers.c          |  7 +++++++
 kernel/bpf/verifier.c         |  4 +++-
 6 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 06b080b61aa5..a8cffbb19cf2 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1412,6 +1412,8 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
 #define LOAD_TAIL_CALL_CNT_PTR(stack)				\
 	__LOAD_TCC_PTR(BPF_TAIL_CALL_CNT_PTR_STACK_OFF(stack))
 
+u64 bpf_get_hw_counter(void);
+
 static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
 		  int oldproglen, struct jit_context *ctx, bool jmp_padding)
 {
@@ -2126,6 +2128,26 @@ st:			if (is_imm8(insn->off))
 		case BPF_JMP | BPF_CALL: {
 			u8 *ip = image + addrs[i - 1];
 
+			if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
+			    imm32 == BPF_CALL_IMM(bpf_get_hw_counter)) {
+				/* Save RDX because RDTSC will use EDX:EAX to return u64 */
+				emit_mov_reg(&prog, true, AUX_REG, BPF_REG_3);
+				if (boot_cpu_has(X86_FEATURE_LFENCE_RDTSC))
+					EMIT_LFENCE();
+				EMIT2(0x0F, 0x31);
+
+				/* shl RDX, 32 */
+				maybe_emit_1mod(&prog, BPF_REG_3, true);
+				EMIT3(0xC1, add_1reg(0xE0, BPF_REG_3), 32);
+				/* or RAX, RDX */
+				maybe_emit_mod(&prog, BPF_REG_0, BPF_REG_3, true);
+				EMIT2(0x09, add_2reg(0xC0, BPF_REG_0, BPF_REG_3));
+				/* restore RDX from R11 */
+				emit_mov_reg(&prog, true, BPF_REG_3, AUX_REG);
+
+				break;
+			}
+
 			func = (u8 *) __bpf_call_base + imm32;
 			if (tail_call_reachable) {
 				LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
@@ -3652,3 +3674,11 @@ u64 bpf_arch_uaddress_limit(void)
 {
 	return 0;
 }
+
+/* x86-64 JIT can inline kfunc */
+bool bpf_jit_inlines_helper_call(s32 imm)
+{
+	if (imm == BPF_CALL_IMM(bpf_get_hw_counter))
+		return true;
+	return false;
+}
diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
index de0f9e5f9f73..66525cb1892c 100644
--- a/arch/x86/net/bpf_jit_comp32.c
+++ b/arch/x86/net/bpf_jit_comp32.c
@@ -1656,6 +1656,8 @@ static int emit_kfunc_call(const struct bpf_prog *bpf_prog, u8 *end_addr,
 	return 0;
 }
 
+u64 bpf_get_hw_counter(void);
+
 static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 		  int oldproglen, struct jit_context *ctx)
 {
@@ -2094,6 +2096,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
 				int err;
 
+				if (imm32 == BPF_CALL_IMM(bpf_get_hw_counter)) {
+					if (boot_cpu_has(X86_FEATURE_LFENCE_RDTSC))
+						EMIT3(0x0F, 0xAE, 0xE8);
+					EMIT2(0x0F, 0x31);
+					break;
+				}
+
 				err = emit_kfunc_call(bpf_prog,
 						      image + addrs[i],
 						      insn, &prog);
@@ -2621,3 +2630,10 @@ bool bpf_jit_supports_kfunc_call(void)
 {
 	return true;
 }
+
+bool bpf_jit_inlines_helper_call(s32 imm)
+{
+	if (imm == BPF_CALL_IMM(bpf_get_hw_counter))
+		return true;
+	return false;
+}
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 7d7578a8eac1..8bdd5e6b2a65 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1111,6 +1111,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog);
 void bpf_jit_compile(struct bpf_prog *prog);
 bool bpf_jit_needs_zext(void);
 bool bpf_jit_inlines_helper_call(s32 imm);
+bool bpf_jit_inlines_kfunc_call(s32 imm);
 bool bpf_jit_supports_subprog_tailcalls(void);
 bool bpf_jit_supports_percpu_insn(void);
 bool bpf_jit_supports_kfunc_call(void);
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 233ea78f8f1b..ab6a2452ade0 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2965,6 +2965,17 @@ bool __weak bpf_jit_inlines_helper_call(s32 imm)
 	return false;
 }
 
+/* Return true if the JIT inlines the call to the kfunc corresponding to
+ * the imm.
+ *
+ * The verifier will not patch the insn->imm for the call to the helper if
+ * this returns true.
+ */
+bool __weak bpf_jit_inlines_kfunc_call(s32 imm)
+{
+	return false;
+}
+
 /* Return TRUE if the JIT backend supports mixing bpf2bpf and tailcalls. */
 bool __weak bpf_jit_supports_subprog_tailcalls(void)
 {
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 5c3fdb29c1b1..f7bf3debbcc4 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -23,6 +23,7 @@
 #include <linux/btf_ids.h>
 #include <linux/bpf_mem_alloc.h>
 #include <linux/kasan.h>
+#include <vdso/datapage.h>
 
 #include "../../lib/kstrtox.h"
 
@@ -3023,6 +3024,11 @@ __bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__sz, const void __user
 	return ret + 1;
 }
 
+__bpf_kfunc u64 bpf_get_hw_counter(void)
+{
+	return __arch_get_hw_counter(1, NULL);
+}
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(generic_btf_ids)
@@ -3112,6 +3118,7 @@ BTF_ID_FLAGS(func, bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_bits_destroy, KF_ITER_DESTROY)
 BTF_ID_FLAGS(func, bpf_copy_from_user_str, KF_SLEEPABLE)
 BTF_ID_FLAGS(func, bpf_get_kmem_cache)
+BTF_ID_FLAGS(func, bpf_get_hw_counter, KF_FASTCALL)
 BTF_KFUNCS_END(common_btf_ids)
 
 static const struct btf_kfunc_id_set common_kfunc_set = {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f514247ba8ba..428e7b84bb02 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11326,6 +11326,7 @@ BTF_ID(func, bpf_session_cookie)
 BTF_ID_UNUSED
 #endif
 BTF_ID(func, bpf_get_kmem_cache)
+BTF_ID(func, bpf_get_hw_counter)
 
 static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
 {
@@ -16291,7 +16292,8 @@ static void mark_fastcall_pattern_for_call(struct bpf_verifier_env *env,
 			return;
 
 		clobbered_regs_mask = kfunc_fastcall_clobber_mask(&meta);
-		can_be_inlined = is_fastcall_kfunc_call(&meta);
+		can_be_inlined = is_fastcall_kfunc_call(&meta) && !call->off &&
+				 bpf_jit_inlines_kfunc_call(call->imm);
 	}
 
 	if (clobbered_regs_mask == ALL_CALLER_SAVED_REGS)
-- 
2.43.5

[PATCH bpf-next v2 2/2] selftests/bpf: add selftest to check rdtsc jit

[Vadim Fedorenko]

get_hw_counter() is replaced with rdtsc instruction on x86_64. Add
tests to check it.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 .../selftests/bpf/prog_tests/verifier.c       |  2 +
 .../selftests/bpf/progs/verifier_rdtsc.c      | 58 +++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_rdtsc.c

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index e26b5150fc43..cf7417465010 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -95,6 +95,7 @@
 #include "verifier_xdp_direct_packet_access.skel.h"
 #include "verifier_bits_iter.skel.h"
 #include "verifier_lsm.skel.h"
+#include "verifier_rdtsc.skel.h"
 
 #define MAX_ENTRIES 11
 
@@ -220,6 +221,7 @@ void test_verifier_xdp(void)                  { RUN(verifier_xdp); }
 void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_packet_access); }
 void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
 void test_verifier_lsm(void)                  { RUN(verifier_lsm); }
+void test_verifier_rdtsc(void)                { RUN(verifier_rdtsc); }
 
 static int init_test_val_map(struct bpf_object *obj, char *map_name)
 {
diff --git a/tools/testing/selftests/bpf/progs/verifier_rdtsc.c b/tools/testing/selftests/bpf/progs/verifier_rdtsc.c
new file mode 100644
index 000000000000..cfce4172c9ab
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_rdtsc.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Inc. */
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+SEC("syscall")
+__arch_x86_64
+__xlated("0: call kernel-function")
+__naked int bpf_rdtsc(void)
+{
+	asm volatile(
+	"call %[bpf_get_hw_counter];"
+	"exit"
+	:
+	: __imm(bpf_get_hw_counter)
+	: __clobber_all
+	);
+}
+
+SEC("syscall")
+__arch_x86_64
+/* program entry for bpf_rdtsc_jit_x86_64(), regular function prologue */
+__jited("	endbr64")
+__jited("	nopl	(%rax,%rax)")
+__jited("	nopl	(%rax)")
+__jited("	pushq	%rbp")
+__jited("	movq	%rsp, %rbp")
+__jited("	endbr64")
+/* save RDX in R11 as it will be overwritten */
+__jited("	movq	%rdx, %r11")
+/* lfence may not be executed depending on cpu features */
+__jited("	{{(lfence|)}}")
+__jited("	rdtsc")
+/* combine EDX:EAX into RAX */
+__jited("	shlq	${{(32|0x20)}}, %rdx")
+__jited("	orq	%rdx, %rax")
+/* restore RDX from R11 */
+__jited("	movq	%r11, %rdx")
+__jited("	leave")
+__naked int bpf_rdtsc_jit_x86_64(void)
+{
+	asm volatile(
+	"call %[bpf_get_hw_counter];"
+	"exit"
+	:
+	: __imm(bpf_get_hw_counter)
+	: __clobber_all
+	);
+}
+
+void rdtsc(void)
+{
+	bpf_get_hw_counter();
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.43.5

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[Alexei Starovoitov]

On Thu, Oct 24, 2024 at 1:51 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>
> New kfunc to return ARCH-specific timecounter. For x86 BPF JIT converts
> it into rdtsc ordered call. Other architectures will get JIT
> implementation too if supported. The fallback is to
> __arch_get_hw_counter().
>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
> v1 -> v2:
> * Fix incorrect function return value type to u64
> * Introduce bpf_jit_inlines_kfunc_call() and use it in
>   mark_fastcall_pattern_for_call() to avoid clobbering in case of
>         running programs with no JIT (Eduard)
> * Avoid rewriting instruction and check function pointer directly
>   in JIT (Alexei)
> * Change includes to fix compile issues on non x86 architectures
> ---
>  arch/x86/net/bpf_jit_comp.c   | 30 ++++++++++++++++++++++++++++++
>  arch/x86/net/bpf_jit_comp32.c | 16 ++++++++++++++++
>  include/linux/filter.h        |  1 +
>  kernel/bpf/core.c             | 11 +++++++++++
>  kernel/bpf/helpers.c          |  7 +++++++
>  kernel/bpf/verifier.c         |  4 +++-
>  6 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 06b080b61aa5..a8cffbb19cf2 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1412,6 +1412,8 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
>  #define LOAD_TAIL_CALL_CNT_PTR(stack)                          \
>         __LOAD_TCC_PTR(BPF_TAIL_CALL_CNT_PTR_STACK_OFF(stack))
>
> +u64 bpf_get_hw_counter(void);

just add it to some .h

>  static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
>                   int oldproglen, struct jit_context *ctx, bool jmp_padding)
>  {
> @@ -2126,6 +2128,26 @@ st:                      if (is_imm8(insn->off))
>                 case BPF_JMP | BPF_CALL: {
>                         u8 *ip = image + addrs[i - 1];
>
> +                       if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
> +                           imm32 == BPF_CALL_IMM(bpf_get_hw_counter)) {
> +                               /* Save RDX because RDTSC will use EDX:EAX to return u64 */
> +                               emit_mov_reg(&prog, true, AUX_REG, BPF_REG_3);
> +                               if (boot_cpu_has(X86_FEATURE_LFENCE_RDTSC))
> +                                       EMIT_LFENCE();
> +                               EMIT2(0x0F, 0x31);
> +
> +                               /* shl RDX, 32 */
> +                               maybe_emit_1mod(&prog, BPF_REG_3, true);
> +                               EMIT3(0xC1, add_1reg(0xE0, BPF_REG_3), 32);
> +                               /* or RAX, RDX */
> +                               maybe_emit_mod(&prog, BPF_REG_0, BPF_REG_3, true);
> +                               EMIT2(0x09, add_2reg(0xC0, BPF_REG_0, BPF_REG_3));
> +                               /* restore RDX from R11 */
> +                               emit_mov_reg(&prog, true, BPF_REG_3, AUX_REG);
> +
> +                               break;
> +                       }
> +
>                         func = (u8 *) __bpf_call_base + imm32;
>                         if (tail_call_reachable) {
>                                 LOAD_TAIL_CALL_CNT_PTR(bpf_prog->aux->stack_depth);
> @@ -3652,3 +3674,11 @@ u64 bpf_arch_uaddress_limit(void)
>  {
>         return 0;
>  }
> +
> +/* x86-64 JIT can inline kfunc */
> +bool bpf_jit_inlines_helper_call(s32 imm)

kfunc

> +{
> +       if (imm == BPF_CALL_IMM(bpf_get_hw_counter))
> +               return true;
> +       return false;
> +}
> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index de0f9e5f9f73..66525cb1892c 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c
> @@ -1656,6 +1656,8 @@ static int emit_kfunc_call(const struct bpf_prog *bpf_prog, u8 *end_addr,
>         return 0;
>  }
>
> +u64 bpf_get_hw_counter(void);
> +
>  static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
>                   int oldproglen, struct jit_context *ctx)
>  {
> @@ -2094,6 +2096,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
>                         if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>                                 int err;
>
> +                               if (imm32 == BPF_CALL_IMM(bpf_get_hw_counter)) {
> +                                       if (boot_cpu_has(X86_FEATURE_LFENCE_RDTSC))
> +                                               EMIT3(0x0F, 0xAE, 0xE8);
> +                                       EMIT2(0x0F, 0x31);
> +                                       break;
> +                               }
> +
>                                 err = emit_kfunc_call(bpf_prog,
>                                                       image + addrs[i],
>                                                       insn, &prog);
> @@ -2621,3 +2630,10 @@ bool bpf_jit_supports_kfunc_call(void)
>  {
>         return true;
>  }
> +
> +bool bpf_jit_inlines_helper_call(s32 imm)

kfunc

> +{
> +       if (imm == BPF_CALL_IMM(bpf_get_hw_counter))
> +               return true;
> +       return false;
> +}
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 7d7578a8eac1..8bdd5e6b2a65 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1111,6 +1111,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog);
>  void bpf_jit_compile(struct bpf_prog *prog);
>  bool bpf_jit_needs_zext(void);
>  bool bpf_jit_inlines_helper_call(s32 imm);
> +bool bpf_jit_inlines_kfunc_call(s32 imm);
>  bool bpf_jit_supports_subprog_tailcalls(void);
>  bool bpf_jit_supports_percpu_insn(void);
>  bool bpf_jit_supports_kfunc_call(void);
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 233ea78f8f1b..ab6a2452ade0 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2965,6 +2965,17 @@ bool __weak bpf_jit_inlines_helper_call(s32 imm)
>         return false;
>  }
>
> +/* Return true if the JIT inlines the call to the kfunc corresponding to
> + * the imm.
> + *
> + * The verifier will not patch the insn->imm for the call to the helper if
> + * this returns true.
> + */
> +bool __weak bpf_jit_inlines_kfunc_call(s32 imm)
> +{
> +       return false;
> +}
> +
>  /* Return TRUE if the JIT backend supports mixing bpf2bpf and tailcalls. */
>  bool __weak bpf_jit_supports_subprog_tailcalls(void)
>  {
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 5c3fdb29c1b1..f7bf3debbcc4 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -23,6 +23,7 @@
>  #include <linux/btf_ids.h>
>  #include <linux/bpf_mem_alloc.h>
>  #include <linux/kasan.h>
> +#include <vdso/datapage.h>
>
>  #include "../../lib/kstrtox.h"
>
> @@ -3023,6 +3024,11 @@ __bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__sz, const void __user
>         return ret + 1;
>  }
>
> +__bpf_kfunc u64 bpf_get_hw_counter(void)
> +{
> +       return __arch_get_hw_counter(1, NULL);
> +}
> +
>  __bpf_kfunc_end_defs();
>
>  BTF_KFUNCS_START(generic_btf_ids)
> @@ -3112,6 +3118,7 @@ BTF_ID_FLAGS(func, bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_iter_bits_destroy, KF_ITER_DESTROY)
>  BTF_ID_FLAGS(func, bpf_copy_from_user_str, KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_get_kmem_cache)
> +BTF_ID_FLAGS(func, bpf_get_hw_counter, KF_FASTCALL)
>  BTF_KFUNCS_END(common_btf_ids)
>
>  static const struct btf_kfunc_id_set common_kfunc_set = {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f514247ba8ba..428e7b84bb02 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11326,6 +11326,7 @@ BTF_ID(func, bpf_session_cookie)
>  BTF_ID_UNUSED
>  #endif
>  BTF_ID(func, bpf_get_kmem_cache)
> +BTF_ID(func, bpf_get_hw_counter)
>
>  static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
>  {
> @@ -16291,7 +16292,8 @@ static void mark_fastcall_pattern_for_call(struct bpf_verifier_env *env,
>                         return;
>
>                 clobbered_regs_mask = kfunc_fastcall_clobber_mask(&meta);
> -               can_be_inlined = is_fastcall_kfunc_call(&meta);
> +               can_be_inlined = is_fastcall_kfunc_call(&meta) && !call->off &&

what call->off check is for?

See errors in BPF CI.

pw-bot: cr

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[Eduard Zingerman]

On Thu, 2024-10-24 at 15:14 -0700, Alexei Starovoitov wrote:

[...]

> > @@ -16291,7 +16292,8 @@ static void mark_fastcall_pattern_for_call(struct bpf_verifier_env *env,
> >                         return;
> > 
> >                 clobbered_regs_mask = kfunc_fastcall_clobber_mask(&meta);
> > -               can_be_inlined = is_fastcall_kfunc_call(&meta);
> > +               can_be_inlined = is_fastcall_kfunc_call(&meta) && !call->off &&
> 
> what call->off check is for?

call->imm is BTF id, call->off is ID of the BTF itself.
I asked Vadim to add this check to make sure that imm points to the
kernel BTF.

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[Alexei Starovoitov]

On Thu, Oct 24, 2024 at 3:17 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-10-24 at 15:14 -0700, Alexei Starovoitov wrote:
>
> [...]
>
> > > @@ -16291,7 +16292,8 @@ static void mark_fastcall_pattern_for_call(struct bpf_verifier_env *env,
> > >                         return;
> > >
> > >                 clobbered_regs_mask = kfunc_fastcall_clobber_mask(&meta);
> > > -               can_be_inlined = is_fastcall_kfunc_call(&meta);
> > > +               can_be_inlined = is_fastcall_kfunc_call(&meta) && !call->off &&
> >
> > what call->off check is for?
>
> call->imm is BTF id, call->off is ID of the BTF itself.

it's actually offset in fd_array

> I asked Vadim to add this check to make sure that imm points to the
> kernel BTF.

makes sense.

is_fastcall_kfunc_call(&meta) && meta.btf == btf_vmlinux && ..

would have been much more obvious.

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[Eduard Zingerman]

On Thu, 2024-10-24 at 15:28 -0700, Alexei Starovoitov wrote:

[...]

> > call->imm is BTF id, call->off is ID of the BTF itself.
> 
> it's actually offset in fd_array

Sure.

> > I asked Vadim to add this check to make sure that imm points to the
> > kernel BTF.
> 
> makes sense.
> 
> is_fastcall_kfunc_call(&meta) && meta.btf == btf_vmlinux && ..
> 
> would have been much more obvious.

Yes, this one looks better.

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[Andrii Nakryiko]

On Thu, Oct 24, 2024 at 1:51 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>
> New kfunc to return ARCH-specific timecounter. For x86 BPF JIT converts
> it into rdtsc ordered call. Other architectures will get JIT
> implementation too if supported. The fallback is to
> __arch_get_hw_counter().
>
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
> v1 -> v2:
> * Fix incorrect function return value type to u64
> * Introduce bpf_jit_inlines_kfunc_call() and use it in
>   mark_fastcall_pattern_for_call() to avoid clobbering in case of
>         running programs with no JIT (Eduard)
> * Avoid rewriting instruction and check function pointer directly
>   in JIT (Alexei)
> * Change includes to fix compile issues on non x86 architectures
> ---
>  arch/x86/net/bpf_jit_comp.c   | 30 ++++++++++++++++++++++++++++++
>  arch/x86/net/bpf_jit_comp32.c | 16 ++++++++++++++++
>  include/linux/filter.h        |  1 +
>  kernel/bpf/core.c             | 11 +++++++++++
>  kernel/bpf/helpers.c          |  7 +++++++
>  kernel/bpf/verifier.c         |  4 +++-
>  6 files changed, 68 insertions(+), 1 deletion(-)
>

[...]

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 5c3fdb29c1b1..f7bf3debbcc4 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -23,6 +23,7 @@
>  #include <linux/btf_ids.h>
>  #include <linux/bpf_mem_alloc.h>
>  #include <linux/kasan.h>
> +#include <vdso/datapage.h>
>
>  #include "../../lib/kstrtox.h"
>
> @@ -3023,6 +3024,11 @@ __bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__sz, const void __user
>         return ret + 1;
>  }
>
> +__bpf_kfunc u64 bpf_get_hw_counter(void)

Hm... so the main idea behind this helper is to measure latency (i.e.,
time), right? So, first of all, the name itself doesn't make it clear
that this is **time stamp** counter, so maybe let's mention
"timestamp" somehow?

But then also, if I understand correctly, it will return the number of
cycles, right? And users would need to somehow convert that to
nanoseconds to make it useful. Is it trivial to do that from the BPF
side? If not, can we specify this helper to return nanoseconds instead
of cycles, maybe?

It would be great if selftest demonstratef the intended use case of
measuring some kernel function latency (or BPF helper latency, doesn't
matter much).

[...]

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[Vadim Fedorenko]

On 25/10/2024 00:17, Andrii Nakryiko wrote:
> On Thu, Oct 24, 2024 at 1:51 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>>
>> New kfunc to return ARCH-specific timecounter. For x86 BPF JIT converts
>> it into rdtsc ordered call. Other architectures will get JIT
>> implementation too if supported. The fallback is to
>> __arch_get_hw_counter().
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>> ---
>> v1 -> v2:
>> * Fix incorrect function return value type to u64
>> * Introduce bpf_jit_inlines_kfunc_call() and use it in
>>    mark_fastcall_pattern_for_call() to avoid clobbering in case of
>>          running programs with no JIT (Eduard)
>> * Avoid rewriting instruction and check function pointer directly
>>    in JIT (Alexei)
>> * Change includes to fix compile issues on non x86 architectures
>> ---
>>   arch/x86/net/bpf_jit_comp.c   | 30 ++++++++++++++++++++++++++++++
>>   arch/x86/net/bpf_jit_comp32.c | 16 ++++++++++++++++
>>   include/linux/filter.h        |  1 +
>>   kernel/bpf/core.c             | 11 +++++++++++
>>   kernel/bpf/helpers.c          |  7 +++++++
>>   kernel/bpf/verifier.c         |  4 +++-
>>   6 files changed, 68 insertions(+), 1 deletion(-)
>>
> 
> [...]
> 
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index 5c3fdb29c1b1..f7bf3debbcc4 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/btf_ids.h>
>>   #include <linux/bpf_mem_alloc.h>
>>   #include <linux/kasan.h>
>> +#include <vdso/datapage.h>
>>
>>   #include "../../lib/kstrtox.h"
>>
>> @@ -3023,6 +3024,11 @@ __bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__sz, const void __user
>>          return ret + 1;
>>   }
>>
>> +__bpf_kfunc u64 bpf_get_hw_counter(void)
> 
> Hm... so the main idea behind this helper is to measure latency (i.e.,
> time), right? So, first of all, the name itself doesn't make it clear
> that this is **time stamp** counter, so maybe let's mention
> "timestamp" somehow?

Well, it's time stamp counter only on x86. Other architectures use cycle
or time counter naming. We might think of changing it to
bpf_get_hw_cycle_counter() if it gives more information.

> But then also, if I understand correctly, it will return the number of
> cycles, right? 

Yes, it will return the amount of cycles passed from the last CPU reset.

> And users would need to somehow convert that to
> nanoseconds to make it useful.

That's questionable. If you think about comparing the performance of the
same kernel function or bpf program on machines with the same
architecture but different generation or slightly different base
frequency. It's much more meaningful to compare CPU cycles instead of
nanoseconds. And with current CPU base frequencies cycles will be more
precise than nanoseconds.

> Is it trivial to do that from the BPF side?

Unfortunately, it is not. The program has to have an access to the cycle
counter configuration/specification to convert cycles to any time value.

 > If not, can we specify this helper to return nanoseconds instead> of 
cycles, maybe?

If we change the specification of the helper to return nanoseconds,
there will be no actual difference between this helper and
bpf_ktime_get_ns() which ends up in read_tsc() if tsc is setup as
system clock source.
At the same time I agree that it might be useful to have an option to
convert cycles into nanoseconds. I can introduce another helper to do
the actual conversion of cycles into nanoseconds using the same 
mechanics as in timekeeping or vDSO implementation of gettimeofday().
The usecase I see here is that the program can save start point in
cycles, then execute the function to check the latency, get the
cycles right after function ends and then use another kfunc to convert
cycles spent into nanoseconds. There will be no need to have this
additional kfunc inlined because it won't be on hot-path. WDYT?

> It would be great if selftest demonstratef the intended use case of
> measuring some kernel function latency (or BPF helper latency, doesn't
> matter much).

I can implement a use case described above if it's OK.

> 
> [...]

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[Andrii Nakryiko]

On Fri, Oct 25, 2024 at 7:01 AM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 25/10/2024 00:17, Andrii Nakryiko wrote:
> > On Thu, Oct 24, 2024 at 1:51 PM Vadim Fedorenko <vadfed@meta.com> wrote:
> >>
> >> New kfunc to return ARCH-specific timecounter. For x86 BPF JIT converts
> >> it into rdtsc ordered call. Other architectures will get JIT
> >> implementation too if supported. The fallback is to
> >> __arch_get_hw_counter().
> >>
> >> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> >> ---
> >> v1 -> v2:
> >> * Fix incorrect function return value type to u64
> >> * Introduce bpf_jit_inlines_kfunc_call() and use it in
> >>    mark_fastcall_pattern_for_call() to avoid clobbering in case of
> >>          running programs with no JIT (Eduard)
> >> * Avoid rewriting instruction and check function pointer directly
> >>    in JIT (Alexei)
> >> * Change includes to fix compile issues on non x86 architectures
> >> ---
> >>   arch/x86/net/bpf_jit_comp.c   | 30 ++++++++++++++++++++++++++++++
> >>   arch/x86/net/bpf_jit_comp32.c | 16 ++++++++++++++++
> >>   include/linux/filter.h        |  1 +
> >>   kernel/bpf/core.c             | 11 +++++++++++
> >>   kernel/bpf/helpers.c          |  7 +++++++
> >>   kernel/bpf/verifier.c         |  4 +++-
> >>   6 files changed, 68 insertions(+), 1 deletion(-)
> >>
> >
> > [...]
> >
> >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> >> index 5c3fdb29c1b1..f7bf3debbcc4 100644
> >> --- a/kernel/bpf/helpers.c
> >> +++ b/kernel/bpf/helpers.c
> >> @@ -23,6 +23,7 @@
> >>   #include <linux/btf_ids.h>
> >>   #include <linux/bpf_mem_alloc.h>
> >>   #include <linux/kasan.h>
> >> +#include <vdso/datapage.h>
> >>
> >>   #include "../../lib/kstrtox.h"
> >>
> >> @@ -3023,6 +3024,11 @@ __bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__sz, const void __user
> >>          return ret + 1;
> >>   }
> >>
> >> +__bpf_kfunc u64 bpf_get_hw_counter(void)
> >
> > Hm... so the main idea behind this helper is to measure latency (i.e.,
> > time), right? So, first of all, the name itself doesn't make it clear
> > that this is **time stamp** counter, so maybe let's mention
> > "timestamp" somehow?
>
> Well, it's time stamp counter only on x86. Other architectures use cycle
> or time counter naming. We might think of changing it to
> bpf_get_hw_cycle_counter() if it gives more information.

bpf_get_cpu_cycles_counter()? or just bpf_get_cpu_cycles()?

>
> > But then also, if I understand correctly, it will return the number of
> > cycles, right?
>
> Yes, it will return the amount of cycles passed from the last CPU reset.
>
> > And users would need to somehow convert that to
> > nanoseconds to make it useful.
>
> That's questionable. If you think about comparing the performance of the
> same kernel function or bpf program on machines with the same
> architecture but different generation or slightly different base
> frequency. It's much more meaningful to compare CPU cycles instead of
> nanoseconds. And with current CPU base frequencies cycles will be more
> precise than nanoseconds.

I'm thinking not about narrow micro-benchmarking use cases, but
generic tracing and observability cases where in addition to
everything else, users almost always want to capture the duration of
whatever they are tracing. In human-relatable (and comparable across
various hosts) time units, not in cycles.

So in practice we'll have to show users how to convert this into
nanoseconds anyways. So let's at least have a test demonstrating how
to do it? (and an extra kfunc might be a solution, yep)

>
> > Is it trivial to do that from the BPF side?
>
> Unfortunately, it is not. The program has to have an access to the cycle
> counter configuration/specification to convert cycles to any time value.
>
>  > If not, can we specify this helper to return nanoseconds instead> of
> cycles, maybe?
>
> If we change the specification of the helper to return nanoseconds,
> there will be no actual difference between this helper and
> bpf_ktime_get_ns() which ends up in read_tsc() if tsc is setup as
> system clock source.
> At the same time I agree that it might be useful to have an option to
> convert cycles into nanoseconds. I can introduce another helper to do
> the actual conversion of cycles into nanoseconds using the same
> mechanics as in timekeeping or vDSO implementation of gettimeofday().
> The usecase I see here is that the program can save start point in
> cycles, then execute the function to check the latency, get the
> cycles right after function ends and then use another kfunc to convert
> cycles spent into nanoseconds. There will be no need to have this
> additional kfunc inlined because it won't be on hot-path. WDYT?

Sounds good to me. My main ask and the goal here is to *eventually*
have time units, because that's the only thing that can be compared
across hosts.

>
> > It would be great if selftest demonstratef the intended use case of
> > measuring some kernel function latency (or BPF helper latency, doesn't
> > matter much).
>
> I can implement a use case described above if it's OK.

Great, thanks.

>
> >
> > [...]
>

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[kernel test robot]

Hi Vadim,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Vadim-Fedorenko/selftests-bpf-add-selftest-to-check-rdtsc-jit/20241025-045340
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20241024205113.762622-1-vadfed%40meta.com
patch subject: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc
config: m68k-sun3_defconfig (https://download.01.org/0day-ci/archive/20241026/202410260829.tdMd3ywG-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241026/202410260829.tdMd3ywG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410260829.tdMd3ywG-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/vdso/datapage.h:17,
                    from kernel/bpf/helpers.c:26:
>> include/vdso/processor.h:10:10: fatal error: asm/vdso/processor.h: No such file or directory
      10 | #include <asm/vdso/processor.h>
         |          ^~~~~~~~~~~~~~~~~~~~~~
   compilation terminated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +10 include/vdso/processor.h

d8bb6993d871f5 Vincenzo Frascino 2020-03-20   9  
d8bb6993d871f5 Vincenzo Frascino 2020-03-20 @10  #include <asm/vdso/processor.h>
d8bb6993d871f5 Vincenzo Frascino 2020-03-20  11  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc

[kernel test robot]

Hi Vadim,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Vadim-Fedorenko/selftests-bpf-add-selftest-to-check-rdtsc-jit/20241025-045340
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20241024205113.762622-1-vadfed%40meta.com
patch subject: [PATCH bpf-next v2 1/2] bpf: add bpf_get_hw_counter kfunc
config: um-allyesconfig (https://download.01.org/0day-ci/archive/20241026/202410260919.mccgFynd-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241026/202410260919.mccgFynd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410260919.mccgFynd-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/vdso/processor.h:10,
                    from include/vdso/datapage.h:17,
                    from kernel/bpf/helpers.c:26:
>> arch/x86/include/asm/vdso/processor.h:11:29: error: redefinition of 'rep_nop'
      11 | static __always_inline void rep_nop(void)
         |                             ^~~~~~~
   In file included from include/linux/spinlock_up.h:8,
                    from include/linux/spinlock.h:97,
                    from include/linux/debugobjects.h:6,
                    from include/linux/timer.h:8,
                    from include/linux/workqueue.h:9,
                    from include/linux/bpf.h:10,
                    from kernel/bpf/helpers.c:4:
   arch/x86/um/asm/processor.h:25:29: note: previous definition of 'rep_nop' with type 'void(void)'
      25 | static __always_inline void rep_nop(void)
         |                             ^~~~~~~
>> arch/x86/include/asm/vdso/processor.h:16:29: error: redefinition of 'cpu_relax'
      16 | static __always_inline void cpu_relax(void)
         |                             ^~~~~~~~~
   arch/x86/um/asm/processor.h:30:29: note: previous definition of 'cpu_relax' with type 'void(void)'
      30 | static __always_inline void cpu_relax(void)
         |                             ^~~~~~~~~
   In file included from include/uapi/linux/filter.h:9,
                    from include/linux/bpf.h:8:
   arch/x86/include/asm/vdso/gettimeofday.h: In function '__arch_get_hw_counter':
   arch/x86/include/asm/vdso/gettimeofday.h:253:34: error: 'VDSO_CLOCKMODE_TSC' undeclared (first use in this function); did you mean 'VDSO_CLOCKMODE_MAX'?
     253 |         if (likely(clock_mode == VDSO_CLOCKMODE_TSC))
         |                                  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:76:45: note: in definition of macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   arch/x86/include/asm/vdso/gettimeofday.h:253:34: note: each undeclared identifier is reported only once for each function it appears in
     253 |         if (likely(clock_mode == VDSO_CLOCKMODE_TSC))
         |                                  ^~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:76:45: note: in definition of macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   arch/x86/include/asm/vdso/gettimeofday.h: In function 'vdso_calc_ns':
>> arch/x86/include/asm/vdso/gettimeofday.h:334:32: error: 'const struct vdso_data' has no member named 'max_cycles'
     334 |         if (unlikely(delta > vd->max_cycles)) {
         |                                ^~
   include/linux/compiler.h:77:45: note: in definition of macro 'unlikely'
      77 | # define unlikely(x)    __builtin_expect(!!(x), 0)
         |                                             ^

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MODVERSIONS
   Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
   Selected by [y]:
   - RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=n] || GCC_PLUGINS [=y]) && MODULES [=y]
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [y]:
   - RESOURCE_KUNIT_TEST [=y] && RUNTIME_TESTING_MENU [=y] && KUNIT [=y]


vim +/rep_nop +11 arch/x86/include/asm/vdso/processor.h

abc22418db02b9 Vincenzo Frascino 2020-03-20   9  
abc22418db02b9 Vincenzo Frascino 2020-03-20  10  /* REP NOP (PAUSE) is a good thing to insert into busy-wait loops. */
abc22418db02b9 Vincenzo Frascino 2020-03-20 @11  static __always_inline void rep_nop(void)
abc22418db02b9 Vincenzo Frascino 2020-03-20  12  {
abc22418db02b9 Vincenzo Frascino 2020-03-20  13  	asm volatile("rep; nop" ::: "memory");
abc22418db02b9 Vincenzo Frascino 2020-03-20  14  }
abc22418db02b9 Vincenzo Frascino 2020-03-20  15  
abc22418db02b9 Vincenzo Frascino 2020-03-20 @16  static __always_inline void cpu_relax(void)
abc22418db02b9 Vincenzo Frascino 2020-03-20  17  {
abc22418db02b9 Vincenzo Frascino 2020-03-20  18  	rep_nop();
abc22418db02b9 Vincenzo Frascino 2020-03-20  19  }
abc22418db02b9 Vincenzo Frascino 2020-03-20  20  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki