On Fri, Oct 25, 2024 at 7:01 AM Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx> wrote: > > 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. 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. > > > > > [...] >
