Re: [PATCH bpf-next v7 1/4] bpf: add bpf_get_cpu_cycles kfunc

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

On 19/11/2024 08:17, Peter Zijlstra wrote:
> On Tue, Nov 19, 2024 at 06:29:09AM -0800, Vadim Fedorenko wrote:
>> On 19/11/2024 03:18, Peter Zijlstra wrote:
>>> On Mon, Nov 18, 2024 at 10:52:42AM -0800, Vadim Fedorenko wrote:
>>>> @@ -2094,6 +2094,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
>>>>    			if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>>>>    				int err;
>>>> +				if (imm32 == BPF_CALL_IMM(bpf_get_cpu_cycles)) {
>>>> +					if (cpu_feature_enabled(X86_FEATURE_LFENCE_RDTSC))
>>>> +						EMIT3(0x0F, 0xAE, 0xE8);
>>>> +					EMIT2(0x0F, 0x31);
>>>> +					break;
>>>> +				}
>>>
>>> TSC != cycles. Naming is bad.
>>
>> Any suggestions?
>>
>> JIT for other architectures will come after this one is merged and some
>> of them will be using cycles, so not too far away form the truth..
> 
> bpf_get_time_stamp() ?
> bpf_get_counter() ?

Well, we have already been somewhere nearby these names [1].

[1] 
https://lore.kernel.org/bpf/CAEf4BzaBNNCYaf9a4oHsB2AzYyc6JCWXpHx6jk22Btv=UAgX4A@mail.gmail.com/

bpf_get_time_stamp() doesn't really explain that the actual timestamp
will be provided by CPU hardware.
bpf_get_counter() is again too general, doesn't provide any information
about what type of counter will be returned. The more specific name,
bpf_get_cycles_counter(), was also discussed in v3 (accidentally, it
didn't reach mailing list). The quote of feedback from Andrii is:

   Bikeshedding time, but let's be consistently slightly verbose, but
   readable. Give nwe have bpf_get_cpu_cycles_counter (which maybe we
   should shorten to "bpf_get_cpu_cycles()"), we should call this
   something like "bpf_cpu_cycles_to_ns()".

It might make a bit more sense to name it bpf_get_cpu_counter(), but it
still looks too general.

Honestly, I'm not a fan of renaming functions once again, I would let
Andrii to vote for naming.