Re: [PATCH bpf-next v8 1/4] bpf: add bpf_get_cpu_time_counter kfunc

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

On 21/11/2024 15:51, Andrii Nakryiko wrote:
> On Thu, Nov 21, 2024 at 7:33 AM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Thu, Nov 21, 2024 at 06:35:39AM -0800, Vadim Fedorenko wrote:
>>> On 21/11/2024 03:32, Peter Zijlstra wrote:
>>>> On Wed, Nov 20, 2024 at 04:08:11PM -0800, Vadim Fedorenko wrote:
>>>>> New kfunc to return ARCH-specific timecounter. For x86 BPF JIT converts
>>>>> it into rdtsc ordered call. Other architectures will get JIT
>>>>> implementation too if supported. The fallback is to
>>>>> __arch_get_hw_counter().
>>>>
>>>> Still not a single word as to *WHY* and what you're going to do with
>>>> those values.
>>>>
>>>> NAK
>>>
>>> Did you have a chance to read cover letter?
>>
>> Cover letter is disposable and not retained when applying patches, as
>> such I rarely read it.
> 
> It's not disposable for BPF trees. We preserve them as part of the
> merge commit for the patch set, e.g. [0]. Both bpf and netdev
> maintainers use a set of scripts to apply patches (pw-apply,
> specifically), that does all that automatically.

Yeah, was going to write the same...

> Vadim,
> 
> Please do another careful pass over commit messages and cover letter.
> I'd suggest moving the version history into cover letter (see other
> multi-version cover letter for an example). You can use an example
> from your BPF selftests as an intended use case (measuring the
> duration of some BPF piece of logic), and I'd also mention that this
> is useful to measure the duration of two related BPF events. E.g.,
> uprobe entry and exit, of kprobe entry/exit. kprobe.session and
> uprobe.session programs are especially well suited for that, as they
> allow to capture initial timestamp, store it in session cookie, then
> retrieve it in return probe and calculate the difference.

Sure, I'm on it already.

> 
> Please also update all the "cycles" references to "time counter",
> stuff like that.

Yeah, I've done it already in my local branch.

Thanks.

> 
> pw-bot: cr
> 
> 
>    [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?h=bpf-next-6.13&id=379d5ee624eda6a897d9e1f7f88c68ea482bd5fa
>    [1] https://git.kernel.org/pub/scm/linux/kernel/git/dborkman/pw.git/