Messages in this thread | | | Subject | Re: [PATCH v2] bpf: Remove config check to enable bpf support for branch records | From | Daniel Borkmann <> | Date | Fri, 19 Nov 2021 17:08:46 +0100 |
| |
On 11/19/21 10:35 AM, kajoljain wrote: > On 11/19/21 4:18 AM, Andrii Nakryiko wrote: >> On Thu, Nov 18, 2021 at 5:10 AM Kajol Jain <kjain@linux.ibm.com> wrote: >>> >>> Branch data available to bpf programs can be very useful to get >>> stack traces out of userspace application. >>> >>> Commit fff7b64355ea ("bpf: Add bpf_read_branch_records() helper") >>> added bpf support to capture branch records in x86. Enable this feature >>> for other architectures as well by removing check specific to x86. >>> Incase any platform didn't support branch stack, it will return with >>> -EINVAL. >>> >>> Selftest 'perf_branches' result on power9 machine with branch stacks >>> support. >>> >>> Before this patch changes: >>> [command]# ./test_progs -t perf_branches >>> #88/1 perf_branches/perf_branches_hw:FAIL >>> #88/2 perf_branches/perf_branches_no_hw:OK >>> #88 perf_branches:FAIL >>> Summary: 0/1 PASSED, 0 SKIPPED, 1 FAILED >>> >>> After this patch changes: >>> [command]# ./test_progs -t perf_branches >>> #88/1 perf_branches/perf_branches_hw:OK >>> #88/2 perf_branches/perf_branches_no_hw:OK >>> #88 perf_branches:OK >>> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED >>> >>> Selftest 'perf_branches' result on power9 machine which doesn't >>> support branch stack >>> >>> After this patch changes: >>> [command]# ./test_progs -t perf_branches >>> #88/1 perf_branches/perf_branches_hw:SKIP >>> #88/2 perf_branches/perf_branches_no_hw:OK >>> #88 perf_branches:OK >>> Summary: 1/1 PASSED, 1 SKIPPED, 0 FAILED >>> >>> Fixes: fff7b64355eac ("bpf: Add bpf_read_branch_records() helper") >>> Suggested-by: Peter Zijlstra <peterz@infradead.org> >>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com> >>> --- >>> >>> Tested this patch changes on power9 machine using selftest >>> 'perf branches' which is added in commit 67306f84ca78 ("selftests/bpf: >>> Add bpf_read_branch_records()") >>> >>> Changelog: >>> v1 -> v2 >>> - Inorder to add bpf support to capture branch record in >>> powerpc, rather then adding config for powerpc, entirely >>> remove config check from bpf_read_branch_records function >>> as suggested by Peter Zijlstra >> >> what will be returned for architectures that don't support branch >> records? Will it be zero instead of -ENOENT? > > Hi Andrii, > Incase any architecture doesn't support branch records and if it > tries to do branch sampling with sample type as > PERF_SAMPLE_BRANCH_STACK, perf_event_open itself will fail. > > And even if, perf_event_open succeeds we have appropriate checks in > bpf_read_branch_records function, which will return -EINVAL for those > architectures. > > Reference from linux/kernel/trace/bpf_trace.c > > Here, br_stack will be empty, for unsupported architectures. > > BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx, > void *, buf, u32, size, u64, flags) > { > ..... > if (unlikely(flags & ~BPF_F_GET_BRANCH_RECORDS_SIZE)) > return -EINVAL; > > if (unlikely(!br_stack)) > return -EINVAL;
In that case for unsupported archs we should probably bail out with -ENOENT here as helper doc says '**-ENOENT** if architecture does not support branch records' (see bpf_read_branch_records() doc in include/uapi/linux/bpf.h).
> .... > } > > Thanks, > Kajol Jain
| |