Re: [PATCH v2 1/2] tools/perf: Add bitfield_swap to handle branch_stack endian issue

From: Arnaldo Carvalho de Melo
Date: Wed Oct 27 2021 - 20:16:59 EST


Em Sat, Oct 16, 2021 at 06:20:58PM +0530, Madhavan Srinivasan escreveu:
> 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__bitfield_swap_branch_stack()
> is defined and introduced in evsel__parse_sample.
>
> Signed-off-by: Madhavan Srinivasan <maddy@xxxxxxxxxxxxx>
> ---
> Changelog v1:
> - Renamed function and macro
> - Added comments in code

[acme@quaco perf]$ perf test python
19: 'import perf' in python : FAILED!
[acme@quaco perf]$ perf test -v python
Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc
19: 'import perf' in python :
--- start ---
test child forked, pid 991284
python usage test: "echo "import sys ; sys.path.append('/tmp/build/perf/python'); import perf" | '/usr/bin/python3' "
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ImportError: /tmp/build/perf/python/perf.cpython-39-x86_64-linux-gnu.so: undefined symbol: bigendian
test child finished with -1
---- end ----
'import perf' in python: FAILED!
[acme@quaco perf]$

> tools/perf/util/evsel.c | 74 +++++++++++++++++++++++++++++++++++++++--
> tools/perf/util/evsel.h | 13 ++++++++
> 2 files changed, 85 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index dbfeceb2546c..746e642d4d32 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2221,6 +2221,51 @@ void __weak arch_perf_parse_sample_weight(struct perf_sample *data,
> data->weight = *array;
> }
>
> +u64 evsel__bitfield_swap_branch_flags(u64 value)
> +{
> + u64 new_val = 0;
> +
> + /*
> + * branch_flags
> + * 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
> + * }
> + * }
> + *
> + * Avoid bswap64() the entire branch_flag.value,
> + * as it has variable bit-field sizes. Instead the
> + * macro takes the bit-field position/size,
> + * swaps it based on the host endianness.
> + */
> + if (bigendian()) {
> + new_val = bitfield_swap(value, 0, 1);
> + new_val |= bitfield_swap(value, 1, 1);
> + new_val |= bitfield_swap(value, 2, 1);
> + new_val |= bitfield_swap(value, 3, 1);
> + new_val |= bitfield_swap(value, 4, 16);
> + new_val |= bitfield_swap(value, 20, 4);
> + new_val |= bitfield_swap(value, 24, 40);
> + } else {
> + new_val = bitfield_swap(value, 63, 1);
> + new_val |= bitfield_swap(value, 62, 1);
> + new_val |= bitfield_swap(value, 61, 1);
> + new_val |= bitfield_swap(value, 60, 1);
> + new_val |= bitfield_swap(value, 44, 16);
> + new_val |= bitfield_swap(value, 40, 4);
> + new_val |= bitfield_swap(value, 0, 40);
> + }
> +
> + return new_val;
> +}
> +
> int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
> struct perf_sample *data)
> {
> @@ -2408,6 +2453,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 +2463,33 @@ 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;
> + /*
> + * if the PERF_SAMPLE_BRANCH_HW_INDEX is not applied,
> + * only nr and entries[] will be output by kernel.
> + */
> + 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__bitfield_swap_branch_flags() uses a
> + * bitfield_swap macro to swap the bit position
> + * based on the host endians.
> + */
> + for (i = 0; i < data->branch_stack->nr; i++, e++)
> + e->flags.value = evsel__bitfield_swap_branch_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..2e82cdbe2c08 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -482,4 +482,17 @@ 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);
> +
> +/*
> + * Macro to swap the bit-field postition and size.
> + * Used when,
> + * - dont need to swap the entire u64 &&
> + * - when u64 has variable bit-field sizes &&
> + * - when presented in a host endian which is different
> + * than the source endian of the perf.data file
> + */
> +#define bitfield_swap(src, pos, size) \
> + ((((src) >> (pos)) & ((1ull << (size)) - 1)) << (63 - ((pos) + (size) - 1)))
> +
> +u64 evsel__bitfield_swap_branch_flags(u64 value);
> #endif /* __PERF_EVSEL_H */
> --
> 2.31.1

--

- Arnaldo