* [PATCH v2] bpf: Remove config check to enable bpf support for branch records
@ 2021-11-18 13:05 Kajol Jain
2021-11-18 22:48 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Kajol Jain @ 2021-11-18 13:05 UTC (permalink / raw)
To: bpf, linux-kernel
Cc: acme, peterz, songliubraving, daniel, andrii, kafai, yhs,
john.fastabend, davem, kpsingh, hawk, kuba, maddy, atrajeev,
linux-perf-users, rnsastry, kjain
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bpf: Remove config check to enable bpf support for branch records
2021-11-18 13:05 [PATCH v2] bpf: Remove config check to enable bpf support for branch records Kajol Jain
@ 2021-11-18 22:48 ` Andrii Nakryiko
2021-11-19 9:35 ` kajoljain
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2021-11-18 22:48 UTC (permalink / raw)
To: Kajol Jain
Cc: bpf, open list, Arnaldo Carvalho de Melo, Peter Ziljstra,
Song Liu, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Yonghong Song, john fastabend, David S. Miller, KP Singh,
Jesper Dangaard Brouer, Jakub Kicinski, maddy, atrajeev,
linux-perf-use.,
rnsastry
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bpf: Remove config check to enable bpf support for branch records
2021-11-18 22:48 ` Andrii Nakryiko
@ 2021-11-19 9:35 ` kajoljain
2021-11-19 16:08 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: kajoljain @ 2021-11-19 9:35 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, open list, Arnaldo Carvalho de Melo, Peter Ziljstra,
Song Liu, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Yonghong Song, john fastabend, David S. Miller, KP Singh,
Jesper Dangaard Brouer, Jakub Kicinski, maddy, atrajeev,
linux-perf-use.,
rnsastry
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bpf: Remove config check to enable bpf support for branch records
2021-11-19 9:35 ` kajoljain
@ 2021-11-19 16:08 ` Daniel Borkmann
2021-11-19 22:45 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2021-11-19 16:08 UTC (permalink / raw)
To: kajoljain, Andrii Nakryiko
Cc: bpf, open list, Arnaldo Carvalho de Melo, Peter Ziljstra,
Song Liu, Andrii Nakryiko, Martin Lau, Yonghong Song,
john fastabend, David S. Miller, KP Singh,
Jesper Dangaard Brouer, Jakub Kicinski, maddy, atrajeev,
linux-perf-use.,
rnsastry
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bpf: Remove config check to enable bpf support for branch records
2021-11-19 16:08 ` Daniel Borkmann
@ 2021-11-19 22:45 ` Andrii Nakryiko
2021-11-23 7:44 ` kajoljain
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2021-11-19 22:45 UTC (permalink / raw)
To: Daniel Borkmann
Cc: kajoljain, bpf, open list, Arnaldo Carvalho de Melo,
Peter Ziljstra, Song Liu, Andrii Nakryiko, Martin Lau,
Yonghong Song, john fastabend, David S. Miller, KP Singh,
Jesper Dangaard Brouer, Jakub Kicinski, maddy, atrajeev,
linux-perf-use.,
rnsastry
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] bpf: Remove config check to enable bpf support for branch records
2021-11-19 22:45 ` Andrii Nakryiko
@ 2021-11-23 7:44 ` kajoljain
0 siblings, 0 replies; 6+ messages in thread
From: kajoljain @ 2021-11-23 7:44 UTC (permalink / raw)
To: Andrii Nakryiko, Daniel Borkmann
Cc: bpf, open list, Arnaldo Carvalho de Melo, Peter Ziljstra,
Song Liu, Andrii Nakryiko, Martin Lau, Yonghong Song,
john fastabend, David S. Miller, KP Singh,
Jesper Dangaard Brouer, Jakub Kicinski, maddy, atrajeev,
linux-perf-use.,
rnsastry
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-23 7:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 13:05 [PATCH v2] bpf: Remove config check to enable bpf support for branch records Kajol Jain
2021-11-18 22:48 ` Andrii Nakryiko
2021-11-19 9:35 ` kajoljain
2021-11-19 16:08 ` Daniel Borkmann
2021-11-19 22:45 ` Andrii Nakryiko
2021-11-23 7:44 ` kajoljain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).