Messages in this thread | | | Date | Thu, 3 Aug 2023 10:22:46 -0400 | Subject | Re: [PATCH V2 2/6] perf: Add branch stack extension | From | "Liang, Kan" <> |
| |
On 2023-08-02 5:58 p.m., Peter Zijlstra wrote: > On Mon, May 22, 2023 at 04:30:36AM -0700, kan.liang@linux.intel.com wrote: >> From: Kan Liang <kan.liang@linux.intel.com> >> >> Currently, the extra information of a branch entry is stored in a u64 >> space. With more and more information added, the space is running out. >> For example, the information of occurrences of events will be added for >> each branch. >> >> Add an extension space to record the new information for each branch >> entry. The space is appended after the struct perf_branch_stack. >> >> Add a bit in struct perf_branch_entry to indicate whether the extra >> information is included. >> >> Reviewed-by: Andi Kleen <ak@linux.intel.com> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> Cc: Sandipan Das <sandipan.das@amd.com> >> Cc: Ravi Bangoria <ravi.bangoria@amd.com> >> Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com> >> --- >> >> New patch >> - Introduce a generic extension space which can be used to >> store the LBR event information for Intel. It can also be used by >> other ARCHs for the other purpose. >> - Add a new bit in struct perf_branch_entry to indicate whether the >> extra information is included. > > Bah.. I don't like this, also the actual format isn't clear to me. > > The uapi part is severely lacking, it just adds the ext:1 thing, but > doesn't describe what if anything happens when it's set. > > The internal perf_branch_stack_ext thing is just that, internal. > Additionally it contains a nr member, which seems to suggest it can be > different from the number of entries in the branch-stack itself -- which > would be odd indeed. > > So we have an 'ext' bit per branch entry to indicate the existance of > this extra data, this again suggests no 1:1 correspondence, but at most > one extra entry per set bit. > > Parsing this will be pretty horrible, no? > > So what we have now is: > > { u64 nr; > { u64 hw_idx; } && PERF_SAMPLE_BRANCH_HW_INDEX > { u64 from, to, flags; } lbr[nr]; > } && PERF_SAMPLE_BRANCH_STACK > > and AFAICT you're doing: > > { u64 nr; > { u64 hw_idx; } && PERF_SAMPLE_BRANCH_HW_INDEX > { u64 from, to, flags; } lbr[nr]; > + { u64 nr2; > + { u64 extra; } extra[nr2]; > + } && OR_i{lbr[i].flags.ext} > } && PERF_SAMPLE_BRANCH_STACK > > Which is pretty horrific, no? The straight forward:
I just tried to make the interface more flexible, since I had no idea how other ARCHs would use the extra space. But it seems such flexibility is not necessary. It is indeed not easy to be parsed.
> > { u64 nr; > { u64 hw_idx; } && PERF_SAMPLE_BRANCH_HW_INDEX > { u64 from, to, flags; } lbr[nr]; > + { u64 extra; } ext[nr] && SOMETHING > } && PERF_SAMPLE_BRANCH_STACK > > Or perhaps even: > > { u64 nr; > { u64 hw_idx; } && PERF_SAMPLE_BRANCH_HW_INDEX > { u64 from, to, flags; > + u64 extra; && SOMETHING > } lbr[nr]; > } && PERF_SAMPLE_BRANCH_STACK > > With the obvious question what 'SOMETHING' should be. I suppose > PERF_SAMPLE_BRANCH_EXTRA was considered and discarded?
Yes, it's considered. I once tried to reuse the existing space/structure as much as possible. So it's dropped.
Other than that, using a new sample type as an indicator should be a better way and much straight forward. I will use it in V3.
> > Implementing the last suggestion wouldn't even be too bad, since having > PERF_SAMPLE_BRANCH_EXTRA set, we know to allocate and cast the existing > perf_sample_data::br_stack to a convenient new type, something like: > > struct perf_branch_entry_ext { > __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 */ > spec:2, /* branch speculation info */ > new_type:4, /* additional branch type */ > priv:3, /* privilege level */ > reserved:31; > __u64 extra; > }; > > Except at that point I think I would suggest doing s/EXTRA/COUNTERS/g > and making it something like: > > union { > __u64 counters; > __u8 c[8]; > }; >
It's good enough for this feature and Intel LBR. My only concern is that it's only a 64 bit extra space. If we need more space later, we have to keep adding perf_branch_entry_ext2 and PERF_SAMPLE_BRANCH_EXTRA2. But I don't have such use case now. Maybe I'm just too paranoid. :)
I will use the suggested structure in V3. If anyone has other concerns, we can discuss them from there.
Thanks, Kan
> Or something daft like that. > > Wouldn't all that make *MUCH* more sense?
| |