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

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

On 19/11/2024 11:16, Andrii Nakryiko wrote:
> On Tue, Nov 19, 2024 at 10:03 AM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> 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.
> 
> Let's go with bpf_get_cpu_time_counter() and bpf_cpu_time_counter_to_ns().

Ok, sure. @Peter are you OK with these names?