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 <[email protected]>
Signed-off-by: Kajol Jain <[email protected]>
---
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
On Thu, Nov 18, 2021 at 5:10 AM Kajol Jain <[email protected]> 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 <[email protected]>
> Signed-off-by: Kajol Jain <[email protected]>
> ---
>
> 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 <[email protected]> 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 <[email protected]>
>> Signed-off-by: Kajol Jain <[email protected]>
>> ---
>>
>> 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 <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Kajol Jain <[email protected]>
>>> ---
>>>
>>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>> Signed-off-by: Kajol Jain <[email protected]>
> >>> ---
> >>>
> >>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: Kajol Jain <[email protected]>
>>>>> ---
>>>>>
>>>>> 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