Messages in this thread | | | Subject | Re: [PATCH v2 1/2] tools/perf: Add bitfield_swap to handle branch_stack endian issue | From | Madhavan Srinivasan <> | Date | Thu, 28 Oct 2021 12:49:45 +0530 |
| |
On 10/28/21 8:07 AM, Arnaldo Carvalho de Melo wrote: > > On October 27, 2021 9:16:51 PM GMT-03:00, Arnaldo Carvalho de Melo <acme@kernel.org> wrote: >> 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@linux.ibm.com> >>> --- >>> Changelog v1: >>> - Renamed function and macro >>> - Added comments in code > Please, run 'perf test' before submitting patches, it's not that difficult, please do so. > > - Arnaldo > >> [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
using tep_is_bigendian() instead of bigendian() fixes it. Will send a V3 with that changes.
Maddy
>> ---- 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
| |