linux-kernel - Re: [RESEND PATCH V3 1/6] perf: Add branch stack extra

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 2 Oct 2023 17:45:35 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     kan.liang@...ux.intel.com
Cc:     mingo@...hat.com, acme@...nel.org, linux-kernel@...r.kernel.org,
        mark.rutland@....com, alexander.shishkin@...ux.intel.com,
        jolsa@...nel.org, namhyung@...nel.org, irogers@...gle.com,
        adrian.hunter@...el.com, ak@...ux.intel.com, eranian@...gle.com,
        alexey.v.bayduraev@...ux.intel.com, tinghao.zhang@...el.com,
        Sandipan Das <sandipan.das@....com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
Subject: Re: [RESEND PATCH V3 1/6] perf: Add branch stack extra

On Mon, Sep 11, 2023 at 08:48:17AM -0700, kan.liang@...ux.intel.com wrote:
> From: Kan Liang <kan.liang@...ux.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.