Re: [PATCH v4 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot

[Song Liu <songliubraving@fb.com>]

> On Aug 31, 2021, at 9:02 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Aug 31, 2021 at 7:01 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> Introduce bpf_get_branch_snapshot(), which allows tracing pogram to get
>> branch trace from hardware (e.g. Intel LBR). To use the feature, the
>> user need to create perf_event with proper branch_record filtering
>> on each cpu, and then calls bpf_get_branch_snapshot in the bpf function.
>> On Intel CPUs, VLBR event (raw event 0x1b00) can be use for this.
>> 
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> include/uapi/linux/bpf.h       | 22 +++++++++++++++++++
>> kernel/bpf/trampoline.c        |  3 ++-
>> kernel/trace/bpf_trace.c       | 40 ++++++++++++++++++++++++++++++++++
>> tools/include/uapi/linux/bpf.h | 22 +++++++++++++++++++
>> 4 files changed, 86 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 791f31dd0abee..c986e6fad5bc0 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -4877,6 +4877,27 @@ union bpf_attr {
>>  *             Get the struct pt_regs associated with **task**.
>>  *     Return
>>  *             A pointer to struct pt_regs.
>> + *
>> + * long bpf_get_branch_snapshot(void *entries, u32 size, u64 flags)
>> + *     Description
>> + *             Get branch trace from hardware engines like Intel LBR. The
>> + *             branch trace is taken soon after the trigger point of the
>> + *             BPF program, so it may contain some entries after the
>> + *             trigger point. The user need to filter these entries
>> + *             accordingly.
>> + *
>> + *             The data is stored as struct perf_branch_entry into output
>> + *             buffer *entries*. *size* is the size of *entries* in bytes.
>> + *             *flags* is reserved for now and must be zero.
>> + *
>> + *     Return
>> + *             On success, number of bytes written to *buf*. On error, a
>> + *             negative value.
>> + *
>> + *             **-EINVAL** if arguments invalid or **size** not a multiple
>> + *             of **sizeof**\ (**struct perf_branch_entry**\ ).
>> + *
>> + *             **-ENOENT** if architecture does not support branch records.
>>  */
>> #define __BPF_FUNC_MAPPER(FN)          \
>>        FN(unspec),                     \
>> @@ -5055,6 +5076,7 @@ union bpf_attr {
>>        FN(get_func_ip),                \
>>        FN(get_attach_cookie),          \
>>        FN(task_pt_regs),               \
>> +       FN(get_branch_snapshot),        \
>>        /* */
>> 
>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
>> index fe1e857324e66..39eaaff81953d 100644
>> --- a/kernel/bpf/trampoline.c
>> +++ b/kernel/bpf/trampoline.c
>> @@ -10,6 +10,7 @@
>> #include <linux/rcupdate_trace.h>
>> #include <linux/rcupdate_wait.h>
>> #include <linux/module.h>
>> +#include <linux/static_call.h>
>> 
>> /* dummy _ops. The verifier will operate on target program's ops. */
>> const struct bpf_verifier_ops bpf_extension_verifier_ops = {
>> @@ -526,7 +527,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
>> }
>> 
>> #define NO_START_TIME 1
>> -static u64 notrace bpf_prog_start_time(void)
>> +static __always_inline u64 notrace bpf_prog_start_time(void)
>> {
>>        u64 start = NO_START_TIME;
>> 
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index 8e2eb950aa829..a8ec3634a3329 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -1017,6 +1017,44 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto_pe = {
>>        .arg1_type      = ARG_PTR_TO_CTX,
>> };
>> 
>> +static DEFINE_PER_CPU(struct perf_branch_snapshot, bpf_perf_branch_snapshot);
>> +
>> +BPF_CALL_3(bpf_get_branch_snapshot, void *, buf, u32, size, u64, flags)
>> +{
>> +#ifndef CONFIG_X86
>> +       return -ENOENT;
> 
> nit: -EOPNOTSUPP probably makes more sense for this?

I had -EOPNOTSUPP in earlier version. But bpf_read_branch_records uses
-ENOENT, so I updated here in v4. I guess -ENOENT also makes sense? I 
won't insist if you think -EOPNOTSUPP is better.  

> 
>> +#else
>> +       static const u32 br_entry_size = sizeof(struct perf_branch_entry);
>> +       u32 to_copy;
>> +
>> +       if (unlikely(flags))
>> +               return -EINVAL;
>> +
>> +       if (!buf || (size % br_entry_size != 0))
>> +               return -EINVAL;
>> +
>> +       static_call(perf_snapshot_branch_stack)(this_cpu_ptr(&bpf_perf_branch_snapshot));
> 
> First, you have four this_cpu_ptr(&bpf_perf_branch_snapshot)
> invocations in this function, probably cleaner to store the pointer in
> local variable?
> 
> But second, this still has the reentrancy problem, right? And further,
> we copy the same LBR data twice (to per-cpu buffer and into
> user-provided destination).
> 
> What if we change perf_snapshot_branch_stack signature to this:
> 
> int perf_snapshot_branch_stack(struct perf_branch_entry *entries, int
> max_nr_entries);
> 
> with the semantics that it will copy only min(max_nr_entreis,
> PERF_MAX_BRANCH_RECORDS) * sizeof(struct perf_branch_entry) bytes.
> That way we can copy directly into a user-provided buffer with no
> per-cpu storage. Of course, perf_snapshot_branch_stack will return
> number of entries copied, either as return result, or if static calls
> don't support that, as another int *nr_entries output argument.

I like this idea. Once we get feedback from Peter, I will change this 
in v5. 

Thanks,
Song