| Message ID | 20211118130507.170154-1-kjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] bpf: Remove config check to enable bpf support for branch records | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf | fail | VM_Test |
| bpf/vmtest-bpf-PR | fail | PR summary |
| bpf/vmtest-bpf-next | fail | VM_Test |
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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? > > - Link to the v1 patch: https://lkml.org/lkml/2021/11/14/434 > > kernel/trace/bpf_trace.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 7396488793ff..5e445985c6b4 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1402,9 +1402,6 @@ static const struct bpf_func_proto bpf_perf_prog_read_value_proto = { > BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx, > void *, buf, u32, size, u64, flags) > { > -#ifndef CONFIG_X86 > - return -ENOENT; > -#else > static const u32 br_entry_size = sizeof(struct perf_branch_entry); > struct perf_branch_stack *br_stack = ctx->data->br_stack; > u32 to_copy; > @@ -1425,7 +1422,6 @@ BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx, > memcpy(buf, br_stack->entries, to_copy); > > return to_copy; > -#endif > } > > static const struct bpf_func_proto bpf_read_branch_records_proto = { > -- > 2.27.0 >
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; .... } Thanks, Kajol Jain >> >> - Link to the v1 patch: https://lkml.org/lkml/2021/11/14/434 >> >> kernel/trace/bpf_trace.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index 7396488793ff..5e445985c6b4 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -1402,9 +1402,6 @@ static const struct bpf_func_proto bpf_perf_prog_read_value_proto = { >> BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx, >> void *, buf, u32, size, u64, flags) >> { >> -#ifndef CONFIG_X86 >> - return -ENOENT; >> -#else >> static const u32 br_entry_size = sizeof(struct perf_branch_entry); >> struct perf_branch_stack *br_stack = ctx->data->br_stack; >> u32 to_copy; >> @@ -1425,7 +1422,6 @@ BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx, >> memcpy(buf, br_stack->entries, to_copy); >> >> return to_copy; >> -#endif >> } >> >> static const struct bpf_func_proto bpf_read_branch_records_proto = { >> -- >> 2.27.0 >>
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
On Fri, Nov 19, 2021 at 8:08 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > 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). Yep, I think so too. > > > .... > > } > > > > Thanks, > > Kajol Jain
On 11/20/21 4:15 AM, Andrii Nakryiko wrote: > On Fri, Nov 19, 2021 at 8:08 AM Daniel Borkmann <daniel@iogearbox.net> wrote: >> >> 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). > > Yep, I think so too. > Hi Andrii/Daniel, I agree, changing return type to -ENOENT make sense, I will update in next version of this patch. Thanks, Kajol Jain >> >>> .... >>> } >>> >>> Thanks, >>> Kajol Jain
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 7396488793ff..5e445985c6b4 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1402,9 +1402,6 @@ static const struct bpf_func_proto bpf_perf_prog_read_value_proto = { BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx, void *, buf, u32, size, u64, flags) { -#ifndef CONFIG_X86 - return -ENOENT; -#else static const u32 br_entry_size = sizeof(struct perf_branch_entry); struct perf_branch_stack *br_stack = ctx->data->br_stack; u32 to_copy; @@ -1425,7 +1422,6 @@ BPF_CALL_4(bpf_read_branch_records, struct bpf_perf_event_data_kern *, ctx, memcpy(buf, br_stack->entries, to_copy); return to_copy; -#endif } static const struct bpf_func_proto bpf_read_branch_records_proto = {
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 - Link to the v1 patch: https://lkml.org/lkml/2021/11/14/434 kernel/trace/bpf_trace.c | 4 ---- 1 file changed, 4 deletions(-)