* [PATCH] perf: Fix interpretation of branch records
@ 2022-11-30 16:51 James Clark
2022-11-30 18:35 ` Namhyung Kim
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: James Clark @ 2022-11-30 16:51 UTC (permalink / raw)
To: linux-perf-users, acme, sandipan.das, Anshuman.Khandual
Cc: linux-kernel, James Clark, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim
Commit 93315e46b000 ("perf/core: Add speculation info to branch
entries") added a new field in between type and new_type. Perf has
its own copy of this struct so update it to match the kernel side.
This doesn't currently cause any issues because new_type is only used
by the Arm BRBE driver which isn't merged yet.
Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
Signed-off-by: James Clark <james.clark@arm.com>
---
tools/perf/util/branch.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
index d6017c9b1872..3ed792db1125 100644
--- a/tools/perf/util/branch.h
+++ b/tools/perf/util/branch.h
@@ -22,9 +22,10 @@ struct branch_flags {
u64 abort:1;
u64 cycles:16;
u64 type:4;
+ u64 spec:2;
u64 new_type:4;
u64 priv:3;
- u64 reserved:33;
+ u64 reserved:31;
};
};
};
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: Fix interpretation of branch records
2022-11-30 16:51 [PATCH] perf: Fix interpretation of branch records James Clark
@ 2022-11-30 18:35 ` Namhyung Kim
2022-12-01 4:16 ` Anshuman Khandual
2022-12-01 9:02 ` Sandipan Das
2 siblings, 0 replies; 6+ messages in thread
From: Namhyung Kim @ 2022-11-30 18:35 UTC (permalink / raw)
To: James Clark
Cc: linux-perf-users, acme, sandipan.das, Anshuman.Khandual,
linux-kernel, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa
On Wed, Nov 30, 2022 at 8:52 AM James Clark <james.clark@arm.com> wrote:
>
> Commit 93315e46b000 ("perf/core: Add speculation info to branch
> entries") added a new field in between type and new_type. Perf has
> its own copy of this struct so update it to match the kernel side.
>
> This doesn't currently cause any issues because new_type is only used
> by the Arm BRBE driver which isn't merged yet.
>
> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
> Signed-off-by: James Clark <james.clark@arm.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
> ---
> tools/perf/util/branch.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
> index d6017c9b1872..3ed792db1125 100644
> --- a/tools/perf/util/branch.h
> +++ b/tools/perf/util/branch.h
> @@ -22,9 +22,10 @@ struct branch_flags {
> u64 abort:1;
> u64 cycles:16;
> u64 type:4;
> + u64 spec:2;
> u64 new_type:4;
> u64 priv:3;
> - u64 reserved:33;
> + u64 reserved:31;
> };
> };
> };
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: Fix interpretation of branch records
2022-11-30 16:51 [PATCH] perf: Fix interpretation of branch records James Clark
2022-11-30 18:35 ` Namhyung Kim
@ 2022-12-01 4:16 ` Anshuman Khandual
2022-12-02 18:33 ` Arnaldo Carvalho de Melo
2022-12-01 9:02 ` Sandipan Das
2 siblings, 1 reply; 6+ messages in thread
From: Anshuman Khandual @ 2022-12-01 4:16 UTC (permalink / raw)
To: James Clark, linux-perf-users, acme, sandipan.das
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim
On 11/30/22 22:21, James Clark wrote:
> Commit 93315e46b000 ("perf/core: Add speculation info to branch
> entries") added a new field in between type and new_type. Perf has
> its own copy of this struct so update it to match the kernel side.
>
> This doesn't currently cause any issues because new_type is only used
> by the Arm BRBE driver which isn't merged yet.
>
> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
> Signed-off-by: James Clark <james.clark@arm.com>
Again, problem from having the same structure in two different places
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> tools/perf/util/branch.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
> index d6017c9b1872..3ed792db1125 100644
> --- a/tools/perf/util/branch.h
> +++ b/tools/perf/util/branch.h
> @@ -22,9 +22,10 @@ struct branch_flags {
> u64 abort:1;
> u64 cycles:16;
> u64 type:4;
> + u64 spec:2;
> u64 new_type:4;
> u64 priv:3;
> - u64 reserved:33;
> + u64 reserved:31;
> };
> };
> };
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: Fix interpretation of branch records
2022-11-30 16:51 [PATCH] perf: Fix interpretation of branch records James Clark
2022-11-30 18:35 ` Namhyung Kim
2022-12-01 4:16 ` Anshuman Khandual
@ 2022-12-01 9:02 ` Sandipan Das
2022-12-01 10:17 ` James Clark
2 siblings, 1 reply; 6+ messages in thread
From: Sandipan Das @ 2022-12-01 9:02 UTC (permalink / raw)
To: James Clark, linux-perf-users, acme, Anshuman.Khandual
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim
On 11/30/2022 10:21 PM, James Clark wrote:
> Commit 93315e46b000 ("perf/core: Add speculation info to branch
> entries") added a new field in between type and new_type. Perf has
> its own copy of this struct so update it to match the kernel side.
>
> This doesn't currently cause any issues because new_type is only used
> by the Arm BRBE driver which isn't merged yet.
>
> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
Technically, in the kernel sources, commit 93315e46b000 ("perf/core: Add
speculation info to branch entries") landed before commit b190bc4ac9e6
("perf: Extend branch type classification").
So I think the Fixes tag should instead have commit 0ddea8e2a0c2
("perf branch: Extend branch type classification") which added the
UAPI changes to the perf tool headers.
Aside from that, the patch looks good to me.
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> tools/perf/util/branch.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
> index d6017c9b1872..3ed792db1125 100644
> --- a/tools/perf/util/branch.h
> +++ b/tools/perf/util/branch.h
> @@ -22,9 +22,10 @@ struct branch_flags {
> u64 abort:1;
> u64 cycles:16;
> u64 type:4;
> + u64 spec:2;
> u64 new_type:4;
> u64 priv:3;
> - u64 reserved:33;
> + u64 reserved:31;
> };
> };
> };
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: Fix interpretation of branch records
2022-12-01 9:02 ` Sandipan Das
@ 2022-12-01 10:17 ` James Clark
0 siblings, 0 replies; 6+ messages in thread
From: James Clark @ 2022-12-01 10:17 UTC (permalink / raw)
To: Sandipan Das, linux-perf-users, acme, Anshuman.Khandual
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim
On 01/12/2022 09:02, Sandipan Das wrote:
> On 11/30/2022 10:21 PM, James Clark wrote:
>> Commit 93315e46b000 ("perf/core: Add speculation info to branch
>> entries") added a new field in between type and new_type. Perf has
>> its own copy of this struct so update it to match the kernel side.
>>
>> This doesn't currently cause any issues because new_type is only used
>> by the Arm BRBE driver which isn't merged yet.
>>
>> Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
>
> Technically, in the kernel sources, commit 93315e46b000 ("perf/core: Add
> speculation info to branch entries") landed before commit b190bc4ac9e6
> ("perf: Extend branch type classification").
>
> So I think the Fixes tag should instead have commit 0ddea8e2a0c2
> ("perf branch: Extend branch type classification") which added the
> UAPI changes to the perf tool headers.
Yes maybe, or 831c05a7621b ("tools headers UAPI: Sync linux/perf_event.h
with the kernel sources") where spec was added to the perf copy of the
headers.
It's hard to say for sure which one is best. I think that the earliest
possible one is best in case anyone is debugging that kernel version
with userspace Perf. And to me it seems any kernel that has the spec
record should have it matching in userspace so I would still prefer
93315e46b000. Applying it on top of any other change would still lead to
misleading interpretation of the records.
>
> Aside from that, the patch looks good to me.
Thanks for the review.
>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> tools/perf/util/branch.h | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
>> index d6017c9b1872..3ed792db1125 100644
>> --- a/tools/perf/util/branch.h
>> +++ b/tools/perf/util/branch.h
>> @@ -22,9 +22,10 @@ struct branch_flags {
>> u64 abort:1;
>> u64 cycles:16;
>> u64 type:4;
>> + u64 spec:2;
>> u64 new_type:4;
>> u64 priv:3;
>> - u64 reserved:33;
>> + u64 reserved:31;
>> };
>> };
>> };
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf: Fix interpretation of branch records
2022-12-01 4:16 ` Anshuman Khandual
@ 2022-12-02 18:33 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-12-02 18:33 UTC (permalink / raw)
To: Anshuman Khandual
Cc: James Clark, linux-perf-users, sandipan.das, linux-kernel,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim
Em Thu, Dec 01, 2022 at 09:46:53AM +0530, Anshuman Khandual escreveu:
>
>
> On 11/30/22 22:21, James Clark wrote:
> > Commit 93315e46b000 ("perf/core: Add speculation info to branch
> > entries") added a new field in between type and new_type. Perf has
> > its own copy of this struct so update it to match the kernel side.
> >
> > This doesn't currently cause any issues because new_type is only used
> > by the Arm BRBE driver which isn't merged yet.
> >
> > Fixes: 93315e46b000 ("perf/core: Add speculation info to branch entries")
> > Signed-off-by: James Clark <james.clark@arm.com>
>
> Again, problem from having the same structure in two different places
Indeed, this was my first reaction, how about backward compatibility, is
this really an ABI?
- Arnaldo
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > ---
> > tools/perf/util/branch.h | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/branch.h b/tools/perf/util/branch.h
> > index d6017c9b1872..3ed792db1125 100644
> > --- a/tools/perf/util/branch.h
> > +++ b/tools/perf/util/branch.h
> > @@ -22,9 +22,10 @@ struct branch_flags {
> > u64 abort:1;
> > u64 cycles:16;
> > u64 type:4;
> > + u64 spec:2;
> > u64 new_type:4;
> > u64 priv:3;
> > - u64 reserved:33;
> > + u64 reserved:31;
> > };
> > };
> > };
--
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-02 18:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-30 16:51 [PATCH] perf: Fix interpretation of branch records James Clark
2022-11-30 18:35 ` Namhyung Kim
2022-12-01 4:16 ` Anshuman Khandual
2022-12-02 18:33 ` Arnaldo Carvalho de Melo
2022-12-01 9:02 ` Sandipan Das
2022-12-01 10:17 ` James Clark
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).