> On Aug 31, 2021, at 9:02 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Aug 31, 2021 at 7:01 PM Song Liu <songliubraving@xxxxxx> 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@xxxxxx>
>> ---
>> 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
