On Mon, Nov 18, 2024 at 10:52:43AM -0800, Vadim Fedorenko wrote:
> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
> + imm32 == BPF_CALL_IMM(bpf_cpu_cycles_to_ns) &&
> + cpu_feature_enabled(X86_FEATURE_CONSTANT_TSC)) {
> + u32 mult, shift;
> +
> + clocks_calc_mult_shift(&mult, &shift, tsc_khz, USEC_PER_SEC, 0);
> + /* imul RAX, RDI, mult */
> + maybe_emit_mod(&prog, BPF_REG_1, BPF_REG_0, true);
> + EMIT2_off32(0x69, add_2reg(0xC0, BPF_REG_1, BPF_REG_0),
> + mult);
> +
> + /* shr RAX, shift (which is less than 64) */
> + maybe_emit_1mod(&prog, BPF_REG_0, true);
> + EMIT3(0xC1, add_1reg(0xE8, BPF_REG_0), shift);
> +
> + break;
> + }
This is ludicrously horrible. Why are you using your own mult/shift and
not offset here instead of using the one from either sched_clock or
clocksource_tsc ?
And being totally inconsistent with your own alternative implementation
which uses the VDSO, which in turn uses clocksource_tsc:
> +__bpf_kfunc u64 bpf_cpu_cycles_to_ns(u64 cycles)
> +{
> + const struct vdso_data *vd = __arch_get_k_vdso_data();
> +
> + vd = &vd[CS_RAW];
> + /* kfunc implementation does less manipulations than vDSO
> + * implementation. BPF use-case assumes two measurements are close
> + * in time and can simplify the logic.
> + */
> + return mul_u64_u32_shr(cycles, vd->mult, vd->shift);
> +}
Also, if I'm not mistaken, the above is broken, you really should add
the offset, without it I don't think we guarantee the result is
monotonic.
