Re: [PATCH 1/2] x86/tsc: Add new BPF helper call bpf_rdtsc

[Tero Kristo]

On 04/07/2023 07:49, Yonghong Song wrote:
> On 7/3/23 3:57 AM, Tero Kristo wrote:
> > Currently the raw TSC counter can be read within kernel via 
> > rdtsc_ordered()
> > and friends, and additionally even userspace has access to it via the
> > RDTSC assembly instruction. BPF programs on the other hand don't have
> > direct access to the TSC counter, but alternatively must go through the
> > performance subsystem (bpf_perf_event_read), which only provides 
> > relative
> > value compared to the start point of the program, and is also much 
> > slower
> > than the direct read. Add a new BPF helper definition for bpf_rdtsc() 
> > which
> > can be used for any accurate profiling needs.
> >
> > A use-case for the new API is for example wakeup latency tracing via
> > eBPF on Intel architecture, where it is extremely beneficial to be able
> > to get raw TSC timestamps and compare these directly to the value
> > programmed to the MSR_IA32_TSC_DEADLINE register. This way a direct
> > latency value from the hardware interrupt to the execution of the
> > interrupt handler can be calculated. Having the functionality within
> > eBPF also has added benefits of allowing to filter any other relevant
> > data like C-state residency values, and also to drop any irrelevant
> > data points directly in the kernel context, without passing all the
> > data to userspace for post-processing.
> >
> > Signed-off-by: Tero Kristo <tero.kristo@xxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/include/asm/msr.h |  1 +
> >   arch/x86/kernel/tsc.c      | 23 +++++++++++++++++++++++
> >   2 files changed, 24 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> > index 65ec1965cd28..3dde673cb563 100644
> > --- a/arch/x86/include/asm/msr.h
> > +++ b/arch/x86/include/asm/msr.h
> > @@ -309,6 +309,7 @@ struct msr *msrs_alloc(void);
> >   void msrs_free(struct msr *msrs);
> >   int msr_set_bit(u32 msr, u8 bit);
> >   int msr_clear_bit(u32 msr, u8 bit);
> > +u64 bpf_rdtsc(void);
> >     #ifdef CONFIG_SMP
> >   int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 344698852146..ded857abef81 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -15,6 +15,8 @@
> >   #include <linux/timex.h>
> >   #include <linux/static_key.h>
> >   #include <linux/static_call.h>
> > +#include <linux/btf.h>
> > +#include <linux/btf_ids.h>
> >     #include <asm/hpet.h>
> >   #include <asm/timer.h>
> > @@ -29,6 +31,7 @@
> >   #include <asm/intel-family.h>
> >   #include <asm/i8259.h>
> >   #include <asm/uv/uv.h>
> > +#include <asm/tlbflush.h>
> >     unsigned int __read_mostly cpu_khz;    /* TSC clocks / usec, not 
> > used here */
> >   EXPORT_SYMBOL(cpu_khz);
> > @@ -1551,6 +1554,24 @@ void __init tsc_early_init(void)
> >       tsc_enable_sched_clock();
> >   }
> >   +u64 bpf_rdtsc(void)
>
> Please see kernel/bpf/helpers.c. For kfunc definition, we should have
>
> __diag_push();
> __diag_ignore_all("-Wmissing-prototypes",
>                   "Global functions as their definitions will be in 
> vmlinux BTF");
>
> _bpf_kfunc u64 bpf_rdtsc(void)
> {
>     ...
> }
>
> __diag_pop();
Thanks, I'll modify this for next rev.
> > +{
> > +    /* Check if Time Stamp is enabled only in ring 0 */
> > +    if (cr4_read_shadow() & X86_CR4_TSD)
> > +        return 0;
> > +
> > +    return rdtsc_ordered();
> > +}
> > +
> > +BTF_SET8_START(tsc_bpf_kfunc_ids)
> > +BTF_ID_FLAGS(func, bpf_rdtsc)
> > +BTF_SET8_END(tsc_bpf_kfunc_ids)
> > +
> > +static const struct btf_kfunc_id_set tsc_bpf_kfunc_set = {
> > +    .owner        = THIS_MODULE,
> > +    .set        = &tsc_bpf_kfunc_ids,
> > +};
> > +
> >   void __init tsc_init(void)
> >   {
> >       if (!cpu_feature_enabled(X86_FEATURE_TSC)) {
> > @@ -1594,6 +1615,8 @@ void __init tsc_init(void)
> >         clocksource_register_khz(&clocksource_tsc_early, tsc_khz);
> >       detect_art();
> > +
> > +    register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, 
> > &tsc_bpf_kfunc_set);
>
> register_btf_kfunc_id_set() could fail, maybe you at least wants to
> have a warning so bpf prog users may be aware that kfunc bpf_rdtsc()
> not really available to bpf programs?

Yes, I'll add a warning print.

-Tero

> >   }
> >     #ifdef CONFIG_SMP