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

[Madhavan Srinivasan]

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@xxxxxxxxxxxxx>
> > ---
> >   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