* [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue
@ 2021-09-30 7:04 Madhavan Srinivasan
2021-09-30 7:04 ` [PATCH 2/2] tools/perf/test: Add endian test for struct branch_flags Madhavan Srinivasan
2021-10-07 17:42 ` [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue Jiri Olsa
0 siblings, 2 replies; 4+ messages in thread
From: Madhavan Srinivasan @ 2021-09-30 7:04 UTC (permalink / raw)
To: acme
Cc: michael, eranian, mark.rutland, namhyung, kjain, atrajeev, jolsa,
linux-perf-users, linux-kernel, Madhavan Srinivasan
branch_stack struct has bit field definition which
produces different bit ordering for big/little endian.
Because of this, when branch_stack sample is collected
in a BE system and viewed/reported in a LE system, bit
fields of the branch stack are not presented properly.
To address this issue, a evsel__reverse64_branch_stack_flags()
is defined and introduced in evsel__parse_sample.
Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
tools/perf/util/evsel.c | 64 +++++++++++++++++++++++++++++++++++++++--
tools/perf/util/evsel.h | 5 ++++
2 files changed, 67 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index dbfeceb2546c..007be66b69a2 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2221,6 +2221,46 @@ void __weak arch_perf_parse_sample_weight(struct perf_sample *data,
data->weight = *array;
}
+u64 evsel__reverse64_branch_stack_flags(u64 value)
+{
+ u64 new_val = 0;
+
+ /*
+ * branch_stack flag (u64)
+ * union {
+ * u64 values;
+ * struct {
+ * 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
+ * }
+ * }
+ */
+ if (bigendian()) {
+ new_val = reverse_64(value, 0, 1);
+ new_val |= reverse_64(value, 1, 1);
+ new_val |= reverse_64(value, 2, 1);
+ new_val |= reverse_64(value, 3, 1);
+ new_val |= reverse_64(value, 4, 16);
+ new_val |= reverse_64(value, 20, 4);
+ new_val |= reverse_64(value, 24, 40);
+ } else {
+ new_val = reverse_64(value, 63, 1);
+ new_val |= reverse_64(value, 62, 1);
+ new_val |= reverse_64(value, 61, 1);
+ new_val |= reverse_64(value, 60, 1);
+ new_val |= reverse_64(value, 44, 16);
+ new_val |= reverse_64(value, 40, 4);
+ new_val |= reverse_64(value, 0, 40);
+ }
+
+ return new_val;
+}
+
int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
struct perf_sample *data)
{
@@ -2408,6 +2448,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 int i;
OVERFLOW_CHECK_u64(array);
data->branch_stack = (struct branch_stack *)array++;
@@ -2416,10 +2458,28 @@ 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) {
+ /*
+ * struct branch_flag does not have endian specific
+ * bit field definition. And bswap will not resolve the
+ * issue, since these are bit fields.
+ *
+ * evsel__reverse64_branch_stack_flags() uses a reverse64
+ * macro to reverse the bit position based on the host
+ * endians.
+ */
+ for (i = 0; i < data->branch_stack->nr; i++, e++)
+ e->flags.value = evsel__reverse64_branch_stack_flags(e->flags.value);
+ }
+
OVERFLOW_CHECK(array, sz, max_size);
array = (void *)array + sz;
}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 1f7edfa8568a..1127c23710cf 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -482,4 +482,9 @@ struct evsel *evsel__leader(struct evsel *evsel);
bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
bool evsel__is_leader(struct evsel *evsel);
void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
+
+#define reverse_64(src, pos, size) \
+ ((((src) >> (pos)) & ((1ull << (size)) - 1)) << (63 - ((pos) + (size) - 1)))
+
+u64 evsel__reverse64_branch_stack_flags(u64 value);
#endif /* __PERF_EVSEL_H */
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] tools/perf/test: Add endian test for struct branch_flags
2021-09-30 7:04 [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue Madhavan Srinivasan
@ 2021-09-30 7:04 ` Madhavan Srinivasan
2021-10-07 17:42 ` [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue Jiri Olsa
1 sibling, 0 replies; 4+ messages in thread
From: Madhavan Srinivasan @ 2021-09-30 7:04 UTC (permalink / raw)
To: acme
Cc: michael, eranian, mark.rutland, namhyung, kjain, atrajeev, jolsa,
linux-perf-users, linux-kernel, Madhavan Srinivasan
Extend sample-parsing test to include branch_flag
endian-reverse test. Patch include "util/trace-event.h"
to sample-parsing for importing bigendian() and extends
samples_same() to include "needs_swap" to detect/enable
check for endian-reverse.
Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
---
tools/perf/tests/sample-parsing.c | 43 +++++++++++++++++++++++++++----
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/tools/perf/tests/sample-parsing.c b/tools/perf/tests/sample-parsing.c
index 8fd8a4ef97da..a67158c12773 100644
--- a/tools/perf/tests/sample-parsing.c
+++ b/tools/perf/tests/sample-parsing.c
@@ -13,6 +13,7 @@
#include "evsel.h"
#include "debug.h"
#include "util/synthetic-events.h"
+#include "util/trace-event.h"
#include "tests.h"
@@ -30,9 +31,18 @@
} \
} while (0)
+/*
+ * Hardcode the expected values for branch_entry flags.
+ * These are based on the input value (213) specified
+ * in branch_stack variable.
+ */
+#define BS_EXPECTED_BE 0xa00d000000000000
+#define BS_EXPECTED_LE 0xd5000000
+#define FLAG(s) s->branch_stack->entries[i].flags
+
static bool samples_same(const struct perf_sample *s1,
const struct perf_sample *s2,
- u64 type, u64 read_format)
+ u64 type, u64 read_format, bool needs_swap)
{
size_t i;
@@ -100,8 +110,14 @@ static bool samples_same(const struct perf_sample *s1,
if (type & PERF_SAMPLE_BRANCH_STACK) {
COMP(branch_stack->nr);
COMP(branch_stack->hw_idx);
- for (i = 0; i < s1->branch_stack->nr; i++)
- MCOMP(branch_stack->entries[i]);
+ for (i = 0; i < s1->branch_stack->nr; i++) {
+ if (needs_swap)
+ return ((bigendian()) ?
+ (FLAG(s2).value == BS_EXPECTED_BE) :
+ (FLAG(s2).value == BS_EXPECTED_LE));
+ else
+ MCOMP(branch_stack->entries[i]);
+ }
}
if (type & PERF_SAMPLE_REGS_USER) {
@@ -248,7 +264,7 @@ static int do_test(u64 sample_type, u64 sample_regs, u64 read_format)
},
};
struct sample_read_value values[] = {{1, 5}, {9, 3}, {2, 7}, {6, 4},};
- struct perf_sample sample_out;
+ struct perf_sample sample_out, sample_out_endian;
size_t i, sz, bufsz;
int err, ret = -1;
@@ -313,12 +329,29 @@ static int do_test(u64 sample_type, u64 sample_regs, u64 read_format)
goto out_free;
}
- if (!samples_same(&sample, &sample_out, sample_type, read_format)) {
+ if (!samples_same(&sample, &sample_out, sample_type, read_format, evsel.needs_swap)) {
pr_debug("parsing failed for sample_type %#"PRIx64"\n",
sample_type);
goto out_free;
}
+ if (sample_type == PERF_SAMPLE_BRANCH_STACK) {
+ evsel.needs_swap = true;
+ evsel.sample_size = __evsel__sample_size(sample_type);
+ err = evsel__parse_sample(&evsel, event, &sample_out_endian);
+ if (err) {
+ pr_debug("%s failed for sample_type %#"PRIx64", error %d\n",
+ "evsel__parse_sample", sample_type, err);
+ goto out_free;
+ }
+
+ if (!samples_same(&sample, &sample_out_endian, sample_type, read_format, evsel.needs_swap)) {
+ pr_debug("parsing failed for sample_type %#"PRIx64"\n",
+ sample_type);
+ goto out_free;
+ }
+ }
+
ret = 0;
out_free:
free(event);
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue
2021-09-30 7:04 [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue Madhavan Srinivasan
2021-09-30 7:04 ` [PATCH 2/2] tools/perf/test: Add endian test for struct branch_flags Madhavan Srinivasan
@ 2021-10-07 17:42 ` Jiri Olsa
2021-10-11 14:29 ` Madhavan Srinivasan
1 sibling, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2021-10-07 17:42 UTC (permalink / raw)
To: Madhavan Srinivasan
Cc: acme, michael, eranian, mark.rutland, namhyung, kjain, atrajeev,
linux-perf-users, linux-kernel
On Thu, Sep 30, 2021 at 12:34:09PM +0530, Madhavan Srinivasan wrote:
> branch_stack struct has bit field definition which
> produces different bit ordering for big/little endian.
> Because of this, when branch_stack sample is collected
> in a BE system and viewed/reported in a LE system, bit
> fields of the branch stack are not presented properly.
> To address this issue, a evsel__reverse64_branch_stack_flags()
> is defined and introduced in evsel__parse_sample.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> ---
> tools/perf/util/evsel.c | 64 +++++++++++++++++++++++++++++++++++++++--
> tools/perf/util/evsel.h | 5 ++++
> 2 files changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index dbfeceb2546c..007be66b69a2 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2221,6 +2221,46 @@ void __weak arch_perf_parse_sample_weight(struct perf_sample *data,
> data->weight = *array;
> }
>
> +u64 evsel__reverse64_branch_stack_flags(u64 value)
> +{
> + u64 new_val = 0;
> +
> + /*
> + * branch_stack flag (u64)
> + * union {
> + * u64 values;
> + * struct {
> + * 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
> + * }
> + * }
> + */
please describe in comment how the bitfield is swapped
> + if (bigendian()) {
> + new_val = reverse_64(value, 0, 1);
> + new_val |= reverse_64(value, 1, 1);
> + new_val |= reverse_64(value, 2, 1);
> + new_val |= reverse_64(value, 3, 1);
> + new_val |= reverse_64(value, 4, 16);
> + new_val |= reverse_64(value, 20, 4);
> + new_val |= reverse_64(value, 24, 40);
> + } else {
> + new_val = reverse_64(value, 63, 1);
> + new_val |= reverse_64(value, 62, 1);
> + new_val |= reverse_64(value, 61, 1);
> + new_val |= reverse_64(value, 60, 1);
> + new_val |= reverse_64(value, 44, 16);
> + new_val |= reverse_64(value, 40, 4);
> + new_val |= reverse_64(value, 0, 40);
> + }
> +
> + return new_val;
> +}
> +
> int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
> struct perf_sample *data)
> {
> @@ -2408,6 +2448,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 int i;
>
> OVERFLOW_CHECK_u64(array);
> data->branch_stack = (struct branch_stack *)array++;
> @@ -2416,10 +2458,28 @@ 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;
hum, why do we convert hw_idx? it's the same struct as entries?
please explain this in comment as well
> + }
> +
> + if (swapped) {
> + /*
> + * struct branch_flag does not have endian specific
> + * bit field definition. And bswap will not resolve the
> + * issue, since these are bit fields.
> + *
> + * evsel__reverse64_branch_stack_flags() uses a reverse64
> + * macro to reverse the bit position based on the host
> + * endians.
> + */
> + for (i = 0; i < data->branch_stack->nr; i++, e++)
> + e->flags.value = evsel__reverse64_branch_stack_flags(e->flags.value);
> + }
> +
> OVERFLOW_CHECK(array, sz, max_size);
> array = (void *)array + sz;
> }
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 1f7edfa8568a..1127c23710cf 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -482,4 +482,9 @@ struct evsel *evsel__leader(struct evsel *evsel);
> bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
> bool evsel__is_leader(struct evsel *evsel);
> void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
> +
> +#define reverse_64(src, pos, size) \
> + ((((src) >> (pos)) & ((1ull << (size)) - 1)) << (63 - ((pos) + (size) - 1)))
hum, is this reversing anything?
could you please add comment describing what this is doing?
thanks,
jirka
> +
> +u64 evsel__reverse64_branch_stack_flags(u64 value);
> #endif /* __PERF_EVSEL_H */
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue
2021-10-07 17:42 ` [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue Jiri Olsa
@ 2021-10-11 14:29 ` Madhavan Srinivasan
0 siblings, 0 replies; 4+ messages in thread
From: Madhavan Srinivasan @ 2021-10-11 14:29 UTC (permalink / raw)
To: Jiri Olsa
Cc: acme, michael, eranian, mark.rutland, namhyung, kjain, atrajeev,
linux-perf-users, linux-kernel
On 10/7/21 11:12 PM, Jiri Olsa wrote:
> On Thu, Sep 30, 2021 at 12:34:09PM +0530, Madhavan Srinivasan wrote:
>> branch_stack struct has bit field definition which
>> produces different bit ordering for big/little endian.
>> Because of this, when branch_stack sample is collected
>> in a BE system and viewed/reported in a LE system, bit
>> fields of the branch stack are not presented properly.
>> To address this issue, a evsel__reverse64_branch_stack_flags()
>> is defined and introduced in evsel__parse_sample.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
>> ---
>> tools/perf/util/evsel.c | 64 +++++++++++++++++++++++++++++++++++++++--
>> tools/perf/util/evsel.h | 5 ++++
>> 2 files changed, 67 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index dbfeceb2546c..007be66b69a2 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -2221,6 +2221,46 @@ void __weak arch_perf_parse_sample_weight(struct perf_sample *data,
>> data->weight = *array;
>> }
>>
>> +u64 evsel__reverse64_branch_stack_flags(u64 value)
>> +{
>> + u64 new_val = 0;
>> +
>> + /*
>> + * branch_stack flag (u64)
>> + * union {
>> + * u64 values;
>> + * struct {
>> + * 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
>> + * }
>> + * }
>> + */
> please describe in comment how the bitfield is swapped
Sure will do.
>> + if (bigendian()) {
>> + new_val = reverse_64(value, 0, 1);
>> + new_val |= reverse_64(value, 1, 1);
>> + new_val |= reverse_64(value, 2, 1);
>> + new_val |= reverse_64(value, 3, 1);
>> + new_val |= reverse_64(value, 4, 16);
>> + new_val |= reverse_64(value, 20, 4);
>> + new_val |= reverse_64(value, 24, 40);
>> + } else {
>> + new_val = reverse_64(value, 63, 1);
>> + new_val |= reverse_64(value, 62, 1);
>> + new_val |= reverse_64(value, 61, 1);
>> + new_val |= reverse_64(value, 60, 1);
>> + new_val |= reverse_64(value, 44, 16);
>> + new_val |= reverse_64(value, 40, 4);
>> + new_val |= reverse_64(value, 0, 40);
>> + }
>> +
>> + return new_val;
>> +}
>> +
>> int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
>> struct perf_sample *data)
>> {
>> @@ -2408,6 +2448,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 int i;
>>
>> OVERFLOW_CHECK_u64(array);
>> data->branch_stack = (struct branch_stack *)array++;
>> @@ -2416,10 +2458,28 @@ 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;
> hum, why do we convert hw_idx? it's the same struct as entries?
> please explain this in comment as well
No. IIUC, hw_idx is valid only if PERF_SAMPLE_BRANCH_HW_INDEX is applied.
If not, then only nr and entries[] will be output-ed by kernel
42bbabed09ce ('perf tools: Add hw_idx in struct branch_stack')
>
>> + }
>> +
>> + if (swapped) {
>> + /*
>> + * struct branch_flag does not have endian specific
>> + * bit field definition. And bswap will not resolve the
>> + * issue, since these are bit fields.
>> + *
>> + * evsel__reverse64_branch_stack_flags() uses a reverse64
>> + * macro to reverse the bit position based on the host
>> + * endians.
>> + */
>> + for (i = 0; i < data->branch_stack->nr; i++, e++)
>> + e->flags.value = evsel__reverse64_branch_stack_flags(e->flags.value);
>> + }
>> +
>> OVERFLOW_CHECK(array, sz, max_size);
>> array = (void *)array + sz;
>> }
>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> index 1f7edfa8568a..1127c23710cf 100644
>> --- a/tools/perf/util/evsel.h
>> +++ b/tools/perf/util/evsel.h
>> @@ -482,4 +482,9 @@ struct evsel *evsel__leader(struct evsel *evsel);
>> bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
>> bool evsel__is_leader(struct evsel *evsel);
>> void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
>> +
>> +#define reverse_64(src, pos, size) \
>> + ((((src) >> (pos)) & ((1ull << (size)) - 1)) << (63 - ((pos) + (size) - 1)))
> hum, is this reversing anything?
> could you please add comment describing what this is doing?
Sure will do.
Thanks for review
Maddy
> thanks, jirka
>> +
>> +u64 evsel__reverse64_branch_stack_flags(u64 value);
>> #endif /* __PERF_EVSEL_H */
>> --
>> 2.31.1
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-11 14:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30 7:04 [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue Madhavan Srinivasan
2021-09-30 7:04 ` [PATCH 2/2] tools/perf/test: Add endian test for struct branch_flags Madhavan Srinivasan
2021-10-07 17:42 ` [PATCH 1/2] tools/perf: Add reverse_fn to handle branch_stack endian issue Jiri Olsa
2021-10-11 14:29 ` Madhavan Srinivasan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.