Re: [PATCH bpf-next v7 2/4] bpf: add bpf_cpu_cycles_to_ns helper

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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.

> 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? AFAIU, the offset is needed to get ns of CLOCK_MONOTONIC,
which can be affected by NTP manipulation. But in this helper we don't
follow any clock_id, we just want to calculate nanoseconds value out of
stable and monotonically increasing counter provided by architecture.