To make it simpler to reset all the info fields on the perf_branch_entry in
a single store, we use a union. This avoids missing some of the bitfields and
improves generated code by minimizing the number of stores.
Using an anonymous struct around the bitfields to guarantee field ordering.
A single perf_branch_entry.val = 0 guarantees all fields are all zeroes on any arch.
Signed-off-by: Stephane Eranian <[email protected]>
---
arch/x86/events/intel/lbr.c | 13 +++----------
include/uapi/linux/perf_event.h | 19 ++++++++++++-------
2 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index 9e6d6eaeb4cb..27aa48cce3c6 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -801,15 +801,9 @@ void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc)
rdmsrl(x86_pmu.lbr_from + lbr_idx, msr_lastbranch.lbr);
+ cpuc->lbr_entries[i].val = 0;
cpuc->lbr_entries[i].from = msr_lastbranch.from;
cpuc->lbr_entries[i].to = msr_lastbranch.to;
- cpuc->lbr_entries[i].mispred = 0;
- cpuc->lbr_entries[i].predicted = 0;
- cpuc->lbr_entries[i].in_tx = 0;
- cpuc->lbr_entries[i].abort = 0;
- cpuc->lbr_entries[i].cycles = 0;
- cpuc->lbr_entries[i].type = 0;
- cpuc->lbr_entries[i].reserved = 0;
}
cpuc->lbr_stack.nr = i;
cpuc->lbr_stack.hw_idx = tos;
@@ -896,6 +890,7 @@ void intel_pmu_lbr_read_64(struct cpu_hw_events *cpuc)
if (abort && x86_pmu.lbr_double_abort && out > 0)
out--;
+ cpuc->lbr_entries[out].val = 0;
cpuc->lbr_entries[out].from = from;
cpuc->lbr_entries[out].to = to;
cpuc->lbr_entries[out].mispred = mis;
@@ -903,8 +898,6 @@ void intel_pmu_lbr_read_64(struct cpu_hw_events *cpuc)
cpuc->lbr_entries[out].in_tx = in_tx;
cpuc->lbr_entries[out].abort = abort;
cpuc->lbr_entries[out].cycles = cycles;
- cpuc->lbr_entries[out].type = 0;
- cpuc->lbr_entries[out].reserved = 0;
out++;
}
cpuc->lbr_stack.nr = out;
@@ -966,6 +959,7 @@ static void intel_pmu_store_lbr(struct cpu_hw_events *cpuc,
to = rdlbr_to(i, lbr);
info = rdlbr_info(i, lbr);
+ e->val = 0;
e->from = from;
e->to = to;
e->mispred = get_lbr_mispred(info);
@@ -974,7 +968,6 @@ static void intel_pmu_store_lbr(struct cpu_hw_events *cpuc,
e->abort = !!(info & LBR_INFO_ABORT);
e->cycles = get_lbr_cycles(info);
e->type = get_lbr_br_type(info);
- e->reserved = 0;
}
cpuc->lbr_stack.nr = i;
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index f92880a15645..eb11f383f4be 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -1329,13 +1329,18 @@ union perf_mem_data_src {
struct perf_branch_entry {
__u64 from;
__u64 to;
- __u64 mispred:1, /* target mispredicted */
- predicted:1,/* target predicted */
- in_tx:1, /* in transaction */
- abort:1, /* transaction abort */
- cycles:16, /* cycle count to last branch */
- type:4, /* branch type */
- reserved:40;
+ union {
+ __u64 val; /* to make it easier to clear all fields */
+ struct {
+ __u64 mispred:1, /* target mispredicted */
+ predicted:1,/* target predicted */
+ in_tx:1, /* in transaction */
+ abort:1, /* transaction abort */
+ cycles:16, /* cycle count to last branch */
+ type:4, /* branch type */
+ reserved:40;
+ };
+ };
};
union perf_sample_weight {
--
2.33.0.153.gba50c8fa24-goog
On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote:
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index f92880a15645..eb11f383f4be 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -1329,13 +1329,18 @@ union perf_mem_data_src {
> struct perf_branch_entry {
> __u64 from;
> __u64 to;
> - __u64 mispred:1, /* target mispredicted */
> - predicted:1,/* target predicted */
> - in_tx:1, /* in transaction */
> - abort:1, /* transaction abort */
> - cycles:16, /* cycle count to last branch */
> - type:4, /* branch type */
> - reserved:40;
> + union {
> + __u64 val; /* to make it easier to clear all fields */
> + struct {
> + __u64 mispred:1, /* target mispredicted */
> + predicted:1,/* target predicted */
> + in_tx:1, /* in transaction */
> + abort:1, /* transaction abort */
> + cycles:16, /* cycle count to last branch */
> + type:4, /* branch type */
> + reserved:40;
> + };
> + };
> };
Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this
one. Power folks, could you please have a look?
Peter Zijlstra <[email protected]> writes:
> On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote:
>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>> index f92880a15645..eb11f383f4be 100644
>> --- a/include/uapi/linux/perf_event.h
>> +++ b/include/uapi/linux/perf_event.h
>> @@ -1329,13 +1329,18 @@ union perf_mem_data_src {
>> struct perf_branch_entry {
>> __u64 from;
>> __u64 to;
>> - __u64 mispred:1, /* target mispredicted */
>> - predicted:1,/* target predicted */
>> - in_tx:1, /* in transaction */
>> - abort:1, /* transaction abort */
>> - cycles:16, /* cycle count to last branch */
>> - type:4, /* branch type */
>> - reserved:40;
>> + union {
>> + __u64 val; /* to make it easier to clear all fields */
>> + struct {
>> + __u64 mispred:1, /* target mispredicted */
>> + predicted:1,/* target predicted */
>> + in_tx:1, /* in transaction */
>> + abort:1, /* transaction abort */
>> + cycles:16, /* cycle count to last branch */
>> + type:4, /* branch type */
>> + reserved:40;
>> + };
>> + };
>> };
>
>
> Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this
> one. Power folks, could you please have a look?
The bit number of each field changes between big and little endian, but
as long as kernel and userspace are the same endian, and both only
access values via the bitfields then it works.
It's preferable to have the ENDIAN_BITFIELD thing, so that the bit
numbers are stable, but we can't change it now without breaking ABI :/
Adding the union risks having code that accesses val and expects to see
mispred in bit 0 for example, which it's not on big endian.
If it's just for clearing easily we could do that with a static inline
that sets all the bitfields. With my compiler here (GCC 10) it's smart
enough to just turn it into a single unsigned long store of 0.
eg:
static inline void clear_perf_branch_entry_flags(struct perf_branch_entry *e)
{
e->mispred = 0;
e->predicted = 0;
e->in_tx = 0;
e->abort = 0;
e->cycles = 0;
e->type = 0;
e->reserved = 0;
}
It does look like we have a bug in perf tool though, if I take a
perf.data from a big endian system to a little endian one I don't see
any of the branch flags decoded. eg:
BE:
2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
... branch stack: nr:28
..... 0: c00000000045c028 -> c00000000dce7604 0 cycles P 0
LE:
2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
... branch stack: nr:28
..... 0: c00000000045c028 -> c00000000dce7604 0 cycles 0
^
missing P
I guess we're missing a byte swap somewhere.
cheers
Michael Ellerman <[email protected]> writes:
> Peter Zijlstra <[email protected]> writes:
>> On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote:
>>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>>> index f92880a15645..eb11f383f4be 100644
>>> --- a/include/uapi/linux/perf_event.h
>>> +++ b/include/uapi/linux/perf_event.h
>>> @@ -1329,13 +1329,18 @@ union perf_mem_data_src {
>>> struct perf_branch_entry {
>>> __u64 from;
>>> __u64 to;
>>> - __u64 mispred:1, /* target mispredicted */
>>> - predicted:1,/* target predicted */
>>> - in_tx:1, /* in transaction */
>>> - abort:1, /* transaction abort */
>>> - cycles:16, /* cycle count to last branch */
>>> - type:4, /* branch type */
>>> - reserved:40;
>>> + union {
>>> + __u64 val; /* to make it easier to clear all fields */
>>> + struct {
>>> + __u64 mispred:1, /* target mispredicted */
>>> + predicted:1,/* target predicted */
>>> + in_tx:1, /* in transaction */
>>> + abort:1, /* transaction abort */
>>> + cycles:16, /* cycle count to last branch */
>>> + type:4, /* branch type */
>>> + reserved:40;
>>> + };
>>> + };
>>> };
>>
>>
>> Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this
>> one. Power folks, could you please have a look?
>
> The bit number of each field changes between big and little endian, but
> as long as kernel and userspace are the same endian, and both only
> access values via the bitfields then it works.
...
>
> It does look like we have a bug in perf tool though, if I take a
> perf.data from a big endian system to a little endian one I don't see
> any of the branch flags decoded. eg:
>
> BE:
>
> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
> ... branch stack: nr:28
> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles P 0
>
> LE:
>
> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
> ... branch stack: nr:28
> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles 0
> ^
> missing P
>
> I guess we're missing a byte swap somewhere.
Ugh. We _do_ have a byte swap, but we also need a bit swap.
That works for the single bit fields, not sure if it will for the
multi-bit fields.
So that's a bit of a mess :/
cheers
Michael,
On Fri, Sep 10, 2021 at 7:16 AM Michael Ellerman <[email protected]> wrote:
>
> Michael Ellerman <[email protected]> writes:
> > Peter Zijlstra <[email protected]> writes:
> >> On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote:
> >>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> >>> index f92880a15645..eb11f383f4be 100644
> >>> --- a/include/uapi/linux/perf_event.h
> >>> +++ b/include/uapi/linux/perf_event.h
> >>> @@ -1329,13 +1329,18 @@ union perf_mem_data_src {
> >>> struct perf_branch_entry {
> >>> __u64 from;
> >>> __u64 to;
> >>> - __u64 mispred:1, /* target mispredicted */
> >>> - predicted:1,/* target predicted */
> >>> - in_tx:1, /* in transaction */
> >>> - abort:1, /* transaction abort */
> >>> - cycles:16, /* cycle count to last branch */
> >>> - type:4, /* branch type */
> >>> - reserved:40;
> >>> + union {
> >>> + __u64 val; /* to make it easier to clear all fields */
> >>> + struct {
> >>> + __u64 mispred:1, /* target mispredicted */
> >>> + predicted:1,/* target predicted */
> >>> + in_tx:1, /* in transaction */
> >>> + abort:1, /* transaction abort */
> >>> + cycles:16, /* cycle count to last branch */
> >>> + type:4, /* branch type */
> >>> + reserved:40;
> >>> + };
> >>> + };
> >>> };
> >>
> >>
> >> Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this
> >> one. Power folks, could you please have a look?
> >
> > The bit number of each field changes between big and little endian, but
> > as long as kernel and userspace are the same endian, and both only
> > access values via the bitfields then it works.
> ...
> >
> > It does look like we have a bug in perf tool though, if I take a
> > perf.data from a big endian system to a little endian one I don't see
> > any of the branch flags decoded. eg:
> >
> > BE:
> >
> > 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
> > ... branch stack: nr:28
> > ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles P 0
> >
> > LE:
> >
> > 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
> > ... branch stack: nr:28
> > ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles 0
> > ^
> > missing P
> >
> > I guess we're missing a byte swap somewhere.
>
> Ugh. We _do_ have a byte swap, but we also need a bit swap.
>
> That works for the single bit fields, not sure if it will for the
> multi-bit fields.
>
> So that's a bit of a mess :/
>
Based on what I see in perf_event.h for other structures, I think I
can make up what you would need for struct branch_entry. But Iit would
be easier if you could send me a patch that you would have verified on
your systems.
Thanks.
On 9/15/21 11:33 AM, Stephane Eranian wrote:
> Michael,
>
>
> On Fri, Sep 10, 2021 at 7:16 AM Michael Ellerman <[email protected]> wrote:
>> Michael Ellerman <[email protected]> writes:
>>> Peter Zijlstra <[email protected]> writes:
>>>> On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote:
>>>>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>>>>> index f92880a15645..eb11f383f4be 100644
>>>>> --- a/include/uapi/linux/perf_event.h
>>>>> +++ b/include/uapi/linux/perf_event.h
>>>>> @@ -1329,13 +1329,18 @@ union perf_mem_data_src {
>>>>> struct perf_branch_entry {
>>>>> __u64 from;
>>>>> __u64 to;
>>>>> - __u64 mispred:1, /* target mispredicted */
>>>>> - predicted:1,/* target predicted */
>>>>> - in_tx:1, /* in transaction */
>>>>> - abort:1, /* transaction abort */
>>>>> - cycles:16, /* cycle count to last branch */
>>>>> - type:4, /* branch type */
>>>>> - reserved:40;
>>>>> + union {
>>>>> + __u64 val; /* to make it easier to clear all fields */
>>>>> + struct {
>>>>> + __u64 mispred:1, /* target mispredicted */
>>>>> + predicted:1,/* target predicted */
>>>>> + in_tx:1, /* in transaction */
>>>>> + abort:1, /* transaction abort */
>>>>> + cycles:16, /* cycle count to last branch */
>>>>> + type:4, /* branch type */
>>>>> + reserved:40;
>>>>> + };
>>>>> + };
>>>>> };
>>>>
>>>> Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this
>>>> one. Power folks, could you please have a look?
>>> The bit number of each field changes between big and little endian, but
>>> as long as kernel and userspace are the same endian, and both only
>>> access values via the bitfields then it works.
>> ...
>>> It does look like we have a bug in perf tool though, if I take a
>>> perf.data from a big endian system to a little endian one I don't see
>>> any of the branch flags decoded. eg:
>>>
>>> BE:
>>>
>>> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
>>> ... branch stack: nr:28
>>> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles P 0
>>>
>>> LE:
>>>
>>> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
>>> ... branch stack: nr:28
>>> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles 0
>>> ^
>>> missing P
>>>
>>> I guess we're missing a byte swap somewhere.
>> Ugh. We _do_ have a byte swap, but we also need a bit swap.
>>
>> That works for the single bit fields, not sure if it will for the
>> multi-bit fields.
>>
>> So that's a bit of a mess :/
>>
> Based on what I see in perf_event.h for other structures, I think I
> can make up what you would need for struct branch_entry. But Iit would
> be easier if you could send me a patch that you would have verified on
> your systems.
> Thanks.
Attached patch fixes the issue. Have tested both in both in BE and LE case.
Maddy
From f816ba2e6ef8d5975f78442d7ecb50d66c3c4326 Mon Sep 17 00:00:00 2001
From: Madhavan Srinivasan <[email protected]>
Date: Wed, 15 Sep 2021 22:29:09 +0530
Subject: [RFC PATCH] tools/perf: Add reverse_64b macro
branch_stack struct has bit field definition
producing different bit ordering for big/little endian.
Because of this, when branch_stack sample collected
in a BE system viewed/reported in a LE system,
bit fields of the branch stack are not presented
properly. To address this issue, a reverse_64b
macro is defined and introduced in evsel__parse_sample.
Signed-off-by: Madhavan Srinivasan <[email protected]>
---
tools/perf/util/evsel.c | 35 +++++++++++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index dbfeceb2546c..3151606e516e 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2221,6 +2221,9 @@ void __weak arch_perf_parse_sample_weight(struct
perf_sample *data,
data->weight = *array;
}
+#define reverse_64b(src, pos, size) \
+ (((src >> pos) & (( 1ull <<size) - 1)) << (63 - (pos + size - 1)))
+
int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
struct perf_sample *data)
{
@@ -2408,6 +2411,8 @@ int evsel__parse_sample(struct evsel *evsel, union
perf_event *event,
if (type & PERF_SAMPLE_BRANCH_STACK) {
const u64 max_branch_nr = UINT64_MAX /
sizeof(struct branch_entry);
+ struct branch_entry *e;
+ unsigned i;
OVERFLOW_CHECK_u64(array);
data->branch_stack = (struct branch_stack *)array++;
@@ -2416,10 +2421,36 @@ int evsel__parse_sample(struct evsel *evsel,
union perf_event *event,
return -EFAULT;
sz = data->branch_stack->nr * sizeof(struct branch_entry);
- if (evsel__has_branch_hw_idx(evsel))
+ if (evsel__has_branch_hw_idx(evsel)) {
sz += sizeof(u64);
- else
+ e = &data->branch_stack->entries[0];
+ } else {
data->no_hw_idx = true;
+ e = (struct branch_entry *)&data->branch_stack->hw_idx;
+ }
+
+ if (swapped) {
+ for (i = 0; i < data->branch_stack->nr; i++, e++) {
+ u64 new_val = 0;
+
+ /* mispred:1 target mispredicted */
+ new_val = reverse_64b(e->flags.value, 0, 1);
+ /* predicted:1 target predicted */
+ new_val |= reverse_64b(e->flags.value, 1, 1);
+ /* in_tx:1 in transaction */
+ new_val |= reverse_64b(e->flags.value, 2, 1);
+ /* abort:1 transaction abort */
+ new_val |= reverse_64b(e->flags.value, 3, 1);
+ /* cycles:16 cycle count to last branch */
+ new_val |= reverse_64b(e->flags.value, 4, 16);
+ /* type:4 branch type */
+ new_val |= reverse_64b(e->flags.value, 20, 4);
+ /* reserved:40 */
+ new_val |= reverse_64b(e->flags.value, 24, 40);
+ e->flags.value = new_val;
+ }
+ }
+
OVERFLOW_CHECK(array, sz, max_size);
array = (void *)array + sz;
}
--
2.31.1
Hi,
On Fri, Sep 17, 2021 at 12:05 AM Michael Ellerman <[email protected]> wrote:
>
> Stephane Eranian <[email protected]> writes:
> > Hi,
> >
> > Thanks for fixing this in the perf tool. But what about the struct
> > branch_entry in the header?
>
> I'm not sure what you mean.
>
> We can't change the order of the fields in the header, without breaking
> existing userspace on BE systems.
>
Ok, I think I had missed that. You are saying that the
#ifdef (__BIG_ENDIAN_BITFIELD) vs __LITTLE_ENDIAN_BITFIELD
is only added to kernel-only data structures?
> It's annoying that the bit numbers are different between LE & BE, but I
> think it's too late to change that.
>
I agree.
> So nothing should change in the branch_entry definition in the header.
>
> My comment on your patch was that adding the union with val, makes it
> easier to misuse the bitfields, because now the values can be accessed
> via the bitfields and also via val, but when using val you have to know
> that the bit numbers differ between BE/LE.
>
Ok, I get it now. We do not need to expose val to user. This is added
for kernel code
convenience only. But if we keep it in kernel, that may break some
other rules about
uapi headers.
> Maybe that's over-paranoid on my part, but if we just want val for
> clearing the values easily then I think the static inline I suggested is
> preferable.
>
> cheers
>
> > On Thu, Sep 16, 2021 at 11:38 PM Madhavan Srinivasan
> > <[email protected]> wrote:
> >>
> >>
> >> On 9/15/21 11:33 AM, Stephane Eranian wrote:
> >> > Michael,
> >> >
> >> >
> >> > On Fri, Sep 10, 2021 at 7:16 AM Michael Ellerman <[email protected]> wrote:
> >> >> Michael Ellerman <[email protected]> writes:
> >> >>> Peter Zijlstra <[email protected]> writes:
> >> >>>> On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote:
> >> >>>>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> >> >>>>> index f92880a15645..eb11f383f4be 100644
> >> >>>>> --- a/include/uapi/linux/perf_event.h
> >> >>>>> +++ b/include/uapi/linux/perf_event.h
> >> >>>>> @@ -1329,13 +1329,18 @@ union perf_mem_data_src {
> >> >>>>> struct perf_branch_entry {
> >> >>>>> __u64 from;
> >> >>>>> __u64 to;
> >> >>>>> - __u64 mispred:1, /* target mispredicted */
> >> >>>>> - predicted:1,/* target predicted */
> >> >>>>> - in_tx:1, /* in transaction */
> >> >>>>> - abort:1, /* transaction abort */
> >> >>>>> - cycles:16, /* cycle count to last branch */
> >> >>>>> - type:4, /* branch type */
> >> >>>>> - reserved:40;
> >> >>>>> + union {
> >> >>>>> + __u64 val; /* to make it easier to clear all fields */
> >> >>>>> + struct {
> >> >>>>> + __u64 mispred:1, /* target mispredicted */
> >> >>>>> + predicted:1,/* target predicted */
> >> >>>>> + in_tx:1, /* in transaction */
> >> >>>>> + abort:1, /* transaction abort */
> >> >>>>> + cycles:16, /* cycle count to last branch */
> >> >>>>> + type:4, /* branch type */
> >> >>>>> + reserved:40;
> >> >>>>> + };
> >> >>>>> + };
> >> >>>>> };
> >> >>>>
> >> >>>> Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this
> >> >>>> one. Power folks, could you please have a look?
> >> >>> The bit number of each field changes between big and little endian, but
> >> >>> as long as kernel and userspace are the same endian, and both only
> >> >>> access values via the bitfields then it works.
> >> >> ...
> >> >>> It does look like we have a bug in perf tool though, if I take a
> >> >>> perf.data from a big endian system to a little endian one I don't see
> >> >>> any of the branch flags decoded. eg:
> >> >>>
> >> >>> BE:
> >> >>>
> >> >>> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
> >> >>> ... branch stack: nr:28
> >> >>> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles P 0
> >> >>>
> >> >>> LE:
> >> >>>
> >> >>> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
> >> >>> ... branch stack: nr:28
> >> >>> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles 0
> >> >>> ^
> >> >>> missing P
> >> >>>
> >> >>> I guess we're missing a byte swap somewhere.
> >> >> Ugh. We _do_ have a byte swap, but we also need a bit swap.
> >> >>
> >> >> That works for the single bit fields, not sure if it will for the
> >> >> multi-bit fields.
> >> >>
> >> >> So that's a bit of a mess :/
> >> >>
> >> > Based on what I see in perf_event.h for other structures, I think I
> >> > can make up what you would need for struct branch_entry. But Iit would
> >> > be easier if you could send me a patch that you would have verified on
> >> > your systems.
> >> > Thanks.
> >> Attached patch fixes the issue. Have tested both in both in BE and LE case.
> >>
> >> Maddy
> >>
> >> From f816ba2e6ef8d5975f78442d7ecb50d66c3c4326 Mon Sep 17 00:00:00 2001
> >> From: Madhavan Srinivasan <[email protected]>
> >> Date: Wed, 15 Sep 2021 22:29:09 +0530
> >> Subject: [RFC PATCH] tools/perf: Add reverse_64b macro
> >>
> >> branch_stack struct has bit field definition
> >> producing different bit ordering for big/little endian.
> >> Because of this, when branch_stack sample collected
> >> in a BE system viewed/reported in a LE system,
> >> bit fields of the branch stack are not presented
> >> properly. To address this issue, a reverse_64b
> >> macro is defined and introduced in evsel__parse_sample.
> >>
> >> Signed-off-by: Madhavan Srinivasan <[email protected]>
> >> ---
> >> tools/perf/util/evsel.c | 35 +++++++++++++++++++++++++++++++++--
> >> 1 file changed, 33 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >> index dbfeceb2546c..3151606e516e 100644
> >> --- a/tools/perf/util/evsel.c
> >> +++ b/tools/perf/util/evsel.c
> >> @@ -2221,6 +2221,9 @@ void __weak arch_perf_parse_sample_weight(struct
> >> perf_sample *data,
> >> data->weight = *array;
> >> }
> >>
> >> +#define reverse_64b(src, pos, size) \
> >> + (((src >> pos) & (( 1ull <<size) - 1)) << (63 - (pos + size - 1)))
> >> +
> >> int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
> >> struct perf_sample *data)
> >> {
> >> @@ -2408,6 +2411,8 @@ int evsel__parse_sample(struct evsel *evsel, union
> >> perf_event *event,
> >> if (type & PERF_SAMPLE_BRANCH_STACK) {
> >> const u64 max_branch_nr = UINT64_MAX /
> >> sizeof(struct branch_entry);
> >> + struct branch_entry *e;
> >> + unsigned i;
> >>
> >> OVERFLOW_CHECK_u64(array);
> >> data->branch_stack = (struct branch_stack *)array++;
> >> @@ -2416,10 +2421,36 @@ int evsel__parse_sample(struct evsel *evsel,
> >> union perf_event *event,
> >> return -EFAULT;
> >>
> >> sz = data->branch_stack->nr * sizeof(struct branch_entry);
> >> - if (evsel__has_branch_hw_idx(evsel))
> >> + if (evsel__has_branch_hw_idx(evsel)) {
> >> sz += sizeof(u64);
> >> - else
> >> + e = &data->branch_stack->entries[0];
> >> + } else {
> >> data->no_hw_idx = true;
> >> + e = (struct branch_entry *)&data->branch_stack->hw_idx;
> >> + }
> >> +
> >> + if (swapped) {
> >> + for (i = 0; i < data->branch_stack->nr; i++, e++) {
> >> + u64 new_val = 0;
> >> +
> >> + /* mispred:1 target mispredicted */
> >> + new_val = reverse_64b(e->flags.value, 0, 1);
> >> + /* predicted:1 target predicted */
> >> + new_val |= reverse_64b(e->flags.value, 1, 1);
> >> + /* in_tx:1 in transaction */
> >> + new_val |= reverse_64b(e->flags.value, 2, 1);
> >> + /* abort:1 transaction abort */
> >> + new_val |= reverse_64b(e->flags.value, 3, 1);
> >> + /* cycles:16 cycle count to last branch */
> >> + new_val |= reverse_64b(e->flags.value, 4, 16);
> >> + /* type:4 branch type */
> >> + new_val |= reverse_64b(e->flags.value, 20, 4);
> >> + /* reserved:40 */
> >> + new_val |= reverse_64b(e->flags.value, 24, 40);
> >> + e->flags.value = new_val;
> >> + }
> >> + }
> >> +
> >> OVERFLOW_CHECK(array, sz, max_size);
> >> array = (void *)array + sz;
> >> }
> >> --
> >> 2.31.1
> >>
> >>
Hi,
Thanks for fixing this in the perf tool. But what about the struct
branch_entry in the header?
On Thu, Sep 16, 2021 at 11:38 PM Madhavan Srinivasan
<[email protected]> wrote:
>
>
> On 9/15/21 11:33 AM, Stephane Eranian wrote:
> > Michael,
> >
> >
> > On Fri, Sep 10, 2021 at 7:16 AM Michael Ellerman <[email protected]> wrote:
> >> Michael Ellerman <[email protected]> writes:
> >>> Peter Zijlstra <[email protected]> writes:
> >>>> On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote:
> >>>>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> >>>>> index f92880a15645..eb11f383f4be 100644
> >>>>> --- a/include/uapi/linux/perf_event.h
> >>>>> +++ b/include/uapi/linux/perf_event.h
> >>>>> @@ -1329,13 +1329,18 @@ union perf_mem_data_src {
> >>>>> struct perf_branch_entry {
> >>>>> __u64 from;
> >>>>> __u64 to;
> >>>>> - __u64 mispred:1, /* target mispredicted */
> >>>>> - predicted:1,/* target predicted */
> >>>>> - in_tx:1, /* in transaction */
> >>>>> - abort:1, /* transaction abort */
> >>>>> - cycles:16, /* cycle count to last branch */
> >>>>> - type:4, /* branch type */
> >>>>> - reserved:40;
> >>>>> + union {
> >>>>> + __u64 val; /* to make it easier to clear all fields */
> >>>>> + struct {
> >>>>> + __u64 mispred:1, /* target mispredicted */
> >>>>> + predicted:1,/* target predicted */
> >>>>> + in_tx:1, /* in transaction */
> >>>>> + abort:1, /* transaction abort */
> >>>>> + cycles:16, /* cycle count to last branch */
> >>>>> + type:4, /* branch type */
> >>>>> + reserved:40;
> >>>>> + };
> >>>>> + };
> >>>>> };
> >>>>
> >>>> Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this
> >>>> one. Power folks, could you please have a look?
> >>> The bit number of each field changes between big and little endian, but
> >>> as long as kernel and userspace are the same endian, and both only
> >>> access values via the bitfields then it works.
> >> ...
> >>> It does look like we have a bug in perf tool though, if I take a
> >>> perf.data from a big endian system to a little endian one I don't see
> >>> any of the branch flags decoded. eg:
> >>>
> >>> BE:
> >>>
> >>> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
> >>> ... branch stack: nr:28
> >>> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles P 0
> >>>
> >>> LE:
> >>>
> >>> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
> >>> ... branch stack: nr:28
> >>> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles 0
> >>> ^
> >>> missing P
> >>>
> >>> I guess we're missing a byte swap somewhere.
> >> Ugh. We _do_ have a byte swap, but we also need a bit swap.
> >>
> >> That works for the single bit fields, not sure if it will for the
> >> multi-bit fields.
> >>
> >> So that's a bit of a mess :/
> >>
> > Based on what I see in perf_event.h for other structures, I think I
> > can make up what you would need for struct branch_entry. But Iit would
> > be easier if you could send me a patch that you would have verified on
> > your systems.
> > Thanks.
> Attached patch fixes the issue. Have tested both in both in BE and LE case.
>
> Maddy
>
> From f816ba2e6ef8d5975f78442d7ecb50d66c3c4326 Mon Sep 17 00:00:00 2001
> From: Madhavan Srinivasan <[email protected]>
> Date: Wed, 15 Sep 2021 22:29:09 +0530
> Subject: [RFC PATCH] tools/perf: Add reverse_64b macro
>
> branch_stack struct has bit field definition
> producing different bit ordering for big/little endian.
> Because of this, when branch_stack sample collected
> in a BE system viewed/reported in a LE system,
> bit fields of the branch stack are not presented
> properly. To address this issue, a reverse_64b
> macro is defined and introduced in evsel__parse_sample.
>
> Signed-off-by: Madhavan Srinivasan <[email protected]>
> ---
> tools/perf/util/evsel.c | 35 +++++++++++++++++++++++++++++++++--
> 1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index dbfeceb2546c..3151606e516e 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2221,6 +2221,9 @@ void __weak arch_perf_parse_sample_weight(struct
> perf_sample *data,
> data->weight = *array;
> }
>
> +#define reverse_64b(src, pos, size) \
> + (((src >> pos) & (( 1ull <<size) - 1)) << (63 - (pos + size - 1)))
> +
> int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
> struct perf_sample *data)
> {
> @@ -2408,6 +2411,8 @@ int evsel__parse_sample(struct evsel *evsel, union
> perf_event *event,
> if (type & PERF_SAMPLE_BRANCH_STACK) {
> const u64 max_branch_nr = UINT64_MAX /
> sizeof(struct branch_entry);
> + struct branch_entry *e;
> + unsigned i;
>
> OVERFLOW_CHECK_u64(array);
> data->branch_stack = (struct branch_stack *)array++;
> @@ -2416,10 +2421,36 @@ int evsel__parse_sample(struct evsel *evsel,
> union perf_event *event,
> return -EFAULT;
>
> sz = data->branch_stack->nr * sizeof(struct branch_entry);
> - if (evsel__has_branch_hw_idx(evsel))
> + if (evsel__has_branch_hw_idx(evsel)) {
> sz += sizeof(u64);
> - else
> + e = &data->branch_stack->entries[0];
> + } else {
> data->no_hw_idx = true;
> + e = (struct branch_entry *)&data->branch_stack->hw_idx;
> + }
> +
> + if (swapped) {
> + for (i = 0; i < data->branch_stack->nr; i++, e++) {
> + u64 new_val = 0;
> +
> + /* mispred:1 target mispredicted */
> + new_val = reverse_64b(e->flags.value, 0, 1);
> + /* predicted:1 target predicted */
> + new_val |= reverse_64b(e->flags.value, 1, 1);
> + /* in_tx:1 in transaction */
> + new_val |= reverse_64b(e->flags.value, 2, 1);
> + /* abort:1 transaction abort */
> + new_val |= reverse_64b(e->flags.value, 3, 1);
> + /* cycles:16 cycle count to last branch */
> + new_val |= reverse_64b(e->flags.value, 4, 16);
> + /* type:4 branch type */
> + new_val |= reverse_64b(e->flags.value, 20, 4);
> + /* reserved:40 */
> + new_val |= reverse_64b(e->flags.value, 24, 40);
> + e->flags.value = new_val;
> + }
> + }
> +
> OVERFLOW_CHECK(array, sz, max_size);
> array = (void *)array + sz;
> }
> --
> 2.31.1
>
>
Stephane Eranian <[email protected]> writes:
> Hi,
>
> Thanks for fixing this in the perf tool. But what about the struct
> branch_entry in the header?
I'm not sure what you mean.
We can't change the order of the fields in the header, without breaking
existing userspace on BE systems.
It's annoying that the bit numbers are different between LE & BE, but I
think it's too late to change that.
So nothing should change in the branch_entry definition in the header.
My comment on your patch was that adding the union with val, makes it
easier to misuse the bitfields, because now the values can be accessed
via the bitfields and also via val, but when using val you have to know
that the bit numbers differ between BE/LE.
Maybe that's over-paranoid on my part, but if we just want val for
clearing the values easily then I think the static inline I suggested is
preferable.
cheers
> On Thu, Sep 16, 2021 at 11:38 PM Madhavan Srinivasan
> <[email protected]> wrote:
>>
>>
>> On 9/15/21 11:33 AM, Stephane Eranian wrote:
>> > Michael,
>> >
>> >
>> > On Fri, Sep 10, 2021 at 7:16 AM Michael Ellerman <[email protected]> wrote:
>> >> Michael Ellerman <[email protected]> writes:
>> >>> Peter Zijlstra <[email protected]> writes:
>> >>>> On Thu, Sep 09, 2021 at 12:56:48AM -0700, Stephane Eranian wrote:
>> >>>>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>> >>>>> index f92880a15645..eb11f383f4be 100644
>> >>>>> --- a/include/uapi/linux/perf_event.h
>> >>>>> +++ b/include/uapi/linux/perf_event.h
>> >>>>> @@ -1329,13 +1329,18 @@ union perf_mem_data_src {
>> >>>>> struct perf_branch_entry {
>> >>>>> __u64 from;
>> >>>>> __u64 to;
>> >>>>> - __u64 mispred:1, /* target mispredicted */
>> >>>>> - predicted:1,/* target predicted */
>> >>>>> - in_tx:1, /* in transaction */
>> >>>>> - abort:1, /* transaction abort */
>> >>>>> - cycles:16, /* cycle count to last branch */
>> >>>>> - type:4, /* branch type */
>> >>>>> - reserved:40;
>> >>>>> + union {
>> >>>>> + __u64 val; /* to make it easier to clear all fields */
>> >>>>> + struct {
>> >>>>> + __u64 mispred:1, /* target mispredicted */
>> >>>>> + predicted:1,/* target predicted */
>> >>>>> + in_tx:1, /* in transaction */
>> >>>>> + abort:1, /* transaction abort */
>> >>>>> + cycles:16, /* cycle count to last branch */
>> >>>>> + type:4, /* branch type */
>> >>>>> + reserved:40;
>> >>>>> + };
>> >>>>> + };
>> >>>>> };
>> >>>>
>> >>>> Hurpmh... all other bitfields have ENDIAN_BITFIELD things except this
>> >>>> one. Power folks, could you please have a look?
>> >>> The bit number of each field changes between big and little endian, but
>> >>> as long as kernel and userspace are the same endian, and both only
>> >>> access values via the bitfields then it works.
>> >> ...
>> >>> It does look like we have a bug in perf tool though, if I take a
>> >>> perf.data from a big endian system to a little endian one I don't see
>> >>> any of the branch flags decoded. eg:
>> >>>
>> >>> BE:
>> >>>
>> >>> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
>> >>> ... branch stack: nr:28
>> >>> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles P 0
>> >>>
>> >>> LE:
>> >>>
>> >>> 2413132652524 0x1db8 [0x2d0]: PERF_RECORD_SAMPLE(IP, 0x1): 5279/5279: 0xc00000000045c028 period: 923003 addr: 0
>> >>> ... branch stack: nr:28
>> >>> ..... 0: c00000000045c028 -> c00000000dce7604 0 cycles 0
>> >>> ^
>> >>> missing P
>> >>>
>> >>> I guess we're missing a byte swap somewhere.
>> >> Ugh. We _do_ have a byte swap, but we also need a bit swap.
>> >>
>> >> That works for the single bit fields, not sure if it will for the
>> >> multi-bit fields.
>> >>
>> >> So that's a bit of a mess :/
>> >>
>> > Based on what I see in perf_event.h for other structures, I think I
>> > can make up what you would need for struct branch_entry. But Iit would
>> > be easier if you could send me a patch that you would have verified on
>> > your systems.
>> > Thanks.
>> Attached patch fixes the issue. Have tested both in both in BE and LE case.
>>
>> Maddy
>>
>> From f816ba2e6ef8d5975f78442d7ecb50d66c3c4326 Mon Sep 17 00:00:00 2001
>> From: Madhavan Srinivasan <[email protected]>
>> Date: Wed, 15 Sep 2021 22:29:09 +0530
>> Subject: [RFC PATCH] tools/perf: Add reverse_64b macro
>>
>> branch_stack struct has bit field definition
>> producing different bit ordering for big/little endian.
>> Because of this, when branch_stack sample collected
>> in a BE system viewed/reported in a LE system,
>> bit fields of the branch stack are not presented
>> properly. To address this issue, a reverse_64b
>> macro is defined and introduced in evsel__parse_sample.
>>
>> Signed-off-by: Madhavan Srinivasan <[email protected]>
>> ---
>> tools/perf/util/evsel.c | 35 +++++++++++++++++++++++++++++++++--
>> 1 file changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index dbfeceb2546c..3151606e516e 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -2221,6 +2221,9 @@ void __weak arch_perf_parse_sample_weight(struct
>> perf_sample *data,
>> data->weight = *array;
>> }
>>
>> +#define reverse_64b(src, pos, size) \
>> + (((src >> pos) & (( 1ull <<size) - 1)) << (63 - (pos + size - 1)))
>> +
>> int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
>> struct perf_sample *data)
>> {
>> @@ -2408,6 +2411,8 @@ int evsel__parse_sample(struct evsel *evsel, union
>> perf_event *event,
>> if (type & PERF_SAMPLE_BRANCH_STACK) {
>> const u64 max_branch_nr = UINT64_MAX /
>> sizeof(struct branch_entry);
>> + struct branch_entry *e;
>> + unsigned i;
>>
>> OVERFLOW_CHECK_u64(array);
>> data->branch_stack = (struct branch_stack *)array++;
>> @@ -2416,10 +2421,36 @@ int evsel__parse_sample(struct evsel *evsel,
>> union perf_event *event,
>> return -EFAULT;
>>
>> sz = data->branch_stack->nr * sizeof(struct branch_entry);
>> - if (evsel__has_branch_hw_idx(evsel))
>> + if (evsel__has_branch_hw_idx(evsel)) {
>> sz += sizeof(u64);
>> - else
>> + e = &data->branch_stack->entries[0];
>> + } else {
>> data->no_hw_idx = true;
>> + e = (struct branch_entry *)&data->branch_stack->hw_idx;
>> + }
>> +
>> + if (swapped) {
>> + for (i = 0; i < data->branch_stack->nr; i++, e++) {
>> + u64 new_val = 0;
>> +
>> + /* mispred:1 target mispredicted */
>> + new_val = reverse_64b(e->flags.value, 0, 1);
>> + /* predicted:1 target predicted */
>> + new_val |= reverse_64b(e->flags.value, 1, 1);
>> + /* in_tx:1 in transaction */
>> + new_val |= reverse_64b(e->flags.value, 2, 1);
>> + /* abort:1 transaction abort */
>> + new_val |= reverse_64b(e->flags.value, 3, 1);
>> + /* cycles:16 cycle count to last branch */
>> + new_val |= reverse_64b(e->flags.value, 4, 16);
>> + /* type:4 branch type */
>> + new_val |= reverse_64b(e->flags.value, 20, 4);
>> + /* reserved:40 */
>> + new_val |= reverse_64b(e->flags.value, 24, 40);
>> + e->flags.value = new_val;
>> + }
>> + }
>> +
>> OVERFLOW_CHECK(array, sz, max_size);
>> array = (void *)array + sz;
>> }
>> --
>> 2.31.1
>>
>>
Stephane Eranian <[email protected]> writes:
> Hi,
>
> On Fri, Sep 17, 2021 at 12:05 AM Michael Ellerman <[email protected]> wrote:
>>
>> Stephane Eranian <[email protected]> writes:
>> > Hi,
>> >
>> > Thanks for fixing this in the perf tool. But what about the struct
>> > branch_entry in the header?
>>
>> I'm not sure what you mean.
>>
>> We can't change the order of the fields in the header, without breaking
>> existing userspace on BE systems.
>>
> Ok, I think I had missed that. You are saying that the
> #ifdef (__BIG_ENDIAN_BITFIELD) vs __LITTLE_ENDIAN_BITFIELD
>
> is only added to kernel-only data structures?
No, we *should* have used __BIG/LITTLE_ENDIAN_BITFIELD for the uapi
definition, but we forgot.
>> It's annoying that the bit numbers are different between LE & BE, but I
>> think it's too late to change that.
>>
> I agree.
>
>> So nothing should change in the branch_entry definition in the header.
>>
>> My comment on your patch was that adding the union with val, makes it
>> easier to misuse the bitfields, because now the values can be accessed
>> via the bitfields and also via val, but when using val you have to know
>> that the bit numbers differ between BE/LE.
>>
> Ok, I get it now. We do not need to expose val to user. This is added
> for kernel code convenience only.
Yeah. Putting the union with val in the uapi encourages userspace to
misuse val to bypass the bitfields, and that risks causing endian bugs.
> But if we keep it in kernel, that may break some other rules about
> uapi headers.
I don't follow what you mean there.
We could use #ifdef __KERNEL__ in the uapi header to make the union
kernel-only, see below, but it's pretty gross.
struct perf_branch_entry {
__u64 from;
__u64 to;
#ifdef __KERNEL__
union {
__u64 val; /* to make it easier to clear all fields */
struct {
#endif
__u64 mispred:1, /* target mispredicted */
predicted:1,/* target predicted */
in_tx:1, /* in transaction */
abort:1, /* transaction abort */
cycles:16, /* cycle count to last branch */
type:4, /* branch type */
reserved:40;
#ifdef __KERNEL__
};
};
#endif
};
If we just do the inline I suggested we can clear the flags in a single
source line, and the generated code seems fine too, eg:
static inline void clear_perf_branch_entry_flags(struct perf_branch_entry *e)
{
e->mispred = 0;
e->predicted = 0;
e->in_tx = 0;
e->abort = 0;
e->cycles = 0;
e->type = 0;
e->reserved = 0;
}
cheers
On Fri, Sep 17, 2021 at 5:38 AM Michael Ellerman <[email protected]> wrote:
>
> Stephane Eranian <[email protected]> writes:
> > Hi,
> >
> > On Fri, Sep 17, 2021 at 12:05 AM Michael Ellerman <[email protected]> wrote:
> >>
> >> Stephane Eranian <[email protected]> writes:
> >> > Hi,
> >> >
> >> > Thanks for fixing this in the perf tool. But what about the struct
> >> > branch_entry in the header?
> >>
> >> I'm not sure what you mean.
> >>
> >> We can't change the order of the fields in the header, without breaking
> >> existing userspace on BE systems.
> >>
> > Ok, I think I had missed that. You are saying that the
> > #ifdef (__BIG_ENDIAN_BITFIELD) vs __LITTLE_ENDIAN_BITFIELD
> >
> > is only added to kernel-only data structures?
>
> No, we *should* have used __BIG/LITTLE_ENDIAN_BITFIELD for the uapi
> definition, but we forgot.
>
But are you suggesting it cannot be fixed?
> >> It's annoying that the bit numbers are different between LE & BE, but I
> >> think it's too late to change that.
> >>
> > I agree.
> >
> >> So nothing should change in the branch_entry definition in the header.
> >>
> >> My comment on your patch was that adding the union with val, makes it
> >> easier to misuse the bitfields, because now the values can be accessed
> >> via the bitfields and also via val, but when using val you have to know
> >> that the bit numbers differ between BE/LE.
> >>
> > Ok, I get it now. We do not need to expose val to user. This is added
> > for kernel code convenience only.
>
> Yeah. Putting the union with val in the uapi encourages userspace to
> misuse val to bypass the bitfields, and that risks causing endian bugs.
>
> > But if we keep it in kernel, that may break some other rules about
> > uapi headers.
>
> I don't follow what you mean there.
>
> We could use #ifdef __KERNEL__ in the uapi header to make the union
> kernel-only, see below, but it's pretty gross.
>
> struct perf_branch_entry {
> __u64 from;
> __u64 to;
> #ifdef __KERNEL__
> union {
> __u64 val; /* to make it easier to clear all fields */
> struct {
> #endif
> __u64 mispred:1, /* target mispredicted */
> predicted:1,/* target predicted */
> in_tx:1, /* in transaction */
> abort:1, /* transaction abort */
> cycles:16, /* cycle count to last branch */
> type:4, /* branch type */
> reserved:40;
> #ifdef __KERNEL__
> };
> };
> #endif
> };
>
>
> If we just do the inline I suggested we can clear the flags in a single
> source line, and the generated code seems fine too, eg:
>
> static inline void clear_perf_branch_entry_flags(struct perf_branch_entry *e)
> {
> e->mispred = 0;
> e->predicted = 0;
> e->in_tx = 0;
> e->abort = 0;
> e->cycles = 0;
> e->type = 0;
> e->reserved = 0;
> }
>
Ok, let's do the inline then. That looks like a cleaner solution to me
assuming the compiler does the right thing.
Stephane Eranian <[email protected]> writes:
> On Fri, Sep 17, 2021 at 5:38 AM Michael Ellerman <[email protected]> wrote:
>>
>> Stephane Eranian <[email protected]> writes:
>> > Hi,
>> >
>> > On Fri, Sep 17, 2021 at 12:05 AM Michael Ellerman <[email protected]> wrote:
>> >>
>> >> Stephane Eranian <[email protected]> writes:
>> >> > Hi,
>> >> >
>> >> > Thanks for fixing this in the perf tool. But what about the struct
>> >> > branch_entry in the header?
>> >>
>> >> I'm not sure what you mean.
>> >>
>> >> We can't change the order of the fields in the header, without breaking
>> >> existing userspace on BE systems.
>> >>
>> > Ok, I think I had missed that. You are saying that the
>> > #ifdef (__BIG_ENDIAN_BITFIELD) vs __LITTLE_ENDIAN_BITFIELD
>> >
>> > is only added to kernel-only data structures?
>>
>> No, we *should* have used __BIG/LITTLE_ENDIAN_BITFIELD for the uapi
>> definition, but we forgot.
>>
> But are you suggesting it cannot be fixed?
I'm not saying it *cannot* be fixed
But I don't think it's sufficiently broken to warrant fixing.
Just adding the __BIG/LITTLE_ENDIAN_BITFIELD ifdefs would break the ABI
for existing users.
So we'd have to continue to support the existing bitfield layout, and
then add a flag for userspace to request a new bitfield layout where the
bit numbers are stable across endian.
But that's way too much effort IMHO.
The existing definition works fine, *except* when perf.data files are
moved between machines of different endianness. That is pretty rare
these days, and can be handled in the perf tool easily enough.
>> >> It's annoying that the bit numbers are different between LE & BE, but I
>> >> think it's too late to change that.
>> >>
>> > I agree.
>> >
>> >> So nothing should change in the branch_entry definition in the header.
>> >>
>> >> My comment on your patch was that adding the union with val, makes it
>> >> easier to misuse the bitfields, because now the values can be accessed
>> >> via the bitfields and also via val, but when using val you have to know
>> >> that the bit numbers differ between BE/LE.
>> >>
>> > Ok, I get it now. We do not need to expose val to user. This is added
>> > for kernel code convenience only.
>>
>> Yeah. Putting the union with val in the uapi encourages userspace to
>> misuse val to bypass the bitfields, and that risks causing endian bugs.
>>
>> > But if we keep it in kernel, that may break some other rules about
>> > uapi headers.
>>
>> I don't follow what you mean there.
>>
>> We could use #ifdef __KERNEL__ in the uapi header to make the union
>> kernel-only, see below, but it's pretty gross.
>>
>> struct perf_branch_entry {
>> __u64 from;
>> __u64 to;
>> #ifdef __KERNEL__
>> union {
>> __u64 val; /* to make it easier to clear all fields */
>> struct {
>> #endif
>> __u64 mispred:1, /* target mispredicted */
>> predicted:1,/* target predicted */
>> in_tx:1, /* in transaction */
>> abort:1, /* transaction abort */
>> cycles:16, /* cycle count to last branch */
>> type:4, /* branch type */
>> reserved:40;
>> #ifdef __KERNEL__
>> };
>> };
>> #endif
>> };
>>
>>
>> If we just do the inline I suggested we can clear the flags in a single
>> source line, and the generated code seems fine too, eg:
>>
>> static inline void clear_perf_branch_entry_flags(struct perf_branch_entry *e)
>> {
>> e->mispred = 0;
>> e->predicted = 0;
>> e->in_tx = 0;
>> e->abort = 0;
>> e->cycles = 0;
>> e->type = 0;
>> e->reserved = 0;
>> }
>>
> Ok, let's do the inline then. That looks like a cleaner solution to me
> assuming the compiler does the right thing.
It did when I checked with GCC 10.
cheers