On Tue, Nov 19, 2024 at 06:38:51AM -0800, Vadim Fedorenko wrote:
> On 19/11/2024 03:28, Peter Zijlstra wrote:
> > 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 ?
>
> With X86_FEATURE_CONSTANT_TSC, tsc_khz is actually constant after
> switching from tsc_early. And the very same call to
> clocks_calc_mult_shift() is used to create clocksource_tsc mult and
> shift constants. Unfortunately, clocksources don't have proper API to
> get the underlying info, that's why I have to calculate shift and mult
> values on my own.
There is cyc2ns_read_begin() / cyc2ns_read_end(), and you can use the
VDSO thing you do below.
> > And being totally inconsistent with your own alternative implementation
> > which uses the VDSO, which in turn uses clocksource_tsc:
>
> With what I said above it is consistent with 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.
>
> Not quite sure how constant offset can affect monotonic guarantee of
> cycles, given that the main use case will be to calculate ns out of
> small deltas?
Well, when I read this patch I didn't know, because your changelogs
don't mention anything at all.
