On 25/10/2024 00:17, Andrii Nakryiko wrote:
On Thu, Oct 24, 2024 at 1:51 PM Vadim Fedorenko <vadfed@xxxxxxxx> 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@xxxxxxxx>
---
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.
[...]
