Messages in this thread | | | Date | Mon, 2 Oct 2023 17:45:35 +0200 | From | Peter Zijlstra <> | Subject | Re: [RESEND PATCH V3 1/6] perf: Add branch stack extra |
| |
On Mon, Sep 11, 2023 at 08:48:17AM -0700, kan.liang@linux.intel.com wrote: > From: Kan Liang <kan.liang@linux.intel.com> > > Currently, the additional 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 a new branch sample type, PERF_SAMPLE_BRANCH_EXTRA, to indicate > whether to support an extra space. > > Two places were suggested to append the extra space. > https://lore.kernel.org/lkml/20230802215814.GH231007@hirez.programming.kicks-ass.net/ > One place is right after the flags of each branch entry. It changes the > existing struct perf_branch_entry. In the later Intel-specific > implementation, two separate spaces have to be created in the > struct cpu_hw_events to store different branch entry structures. That > duplicates space.
Well, something like so:
- struct perf_branch_entry lbr_entries[MAX_LBR_ENTRIES]; + + union { + struct perf_branch_entry lbr_entries[MAX_LBR_ENTRIES]; + struct perf_branch_entry_ext lbr_entries_ext[MAX_LBR_ENTRIES]; + };
would just do... you just have to be really careful to consistently pick the right one.
Something that might help would be to do make perf_branch_stack::entries a 'void *' and use:
struct perf_branch_entry_ext * perf_get_branch_entry(struct perf_sample_data *data, int idx) { if (data->sample_flags & PERF_SAMPLE_BRANCH_EXTRA) return (struct perf_branch_entry_ext *)data->br_stack->entries + idx; return (struct perf_branch_entry *)data->br_stack->entries + idx; }
> The other place is right after the entire struct perf_branch_stack. > Only adding the new extra space in the struct cpu_hw_event is necessary. > The disadvantage is that the pointer of the extra space has to be > recorded. The common interface perf_sample_save_brstack() has to be > updated as well.
Right.. probably easier.
> The latter requires less space and is much straight forward. It is > implemented in the patch.
Same amount of space either way around. 'n*x+n*y == n*(x+y)' and all that.
> Also, add a new branch sample type, PERF_SAMPLE_BRANCH_EVT_CNTRS, to > indicate whether include occurrences of events in branch info. The > information will be stored in the extra space.
This... why do we need two flags?
Also, I can't find this in the SDM, how wide are these counter deltas? ISTR they're saturating, but not how wide they are.
| |