Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <ce16c6c1-9bdc-35e2-fe85-155bd7edbaa4@linux.intel.com>
Date:   Mon, 2 Oct 2023 15:19:04 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [RESEND PATCH V3 1/6] perf: Add branch stack extra
Content-Language: en-US
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     mingo@redhat.com, acme@kernel.org, linux-kernel@vger.kernel.org,
        mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
        jolsa@kernel.org, namhyung@kernel.org, irogers@google.com,
        adrian.hunter@intel.com, ak@linux.intel.com, eranian@google.com,
        alexey.v.bayduraev@linux.intel.com, tinghao.zhang@intel.com,
        Sandipan Das <sandipan.das@amd.com>,
        Ravi Bangoria <ravi.bangoria@amd.com>,
        Athira Rajeev <atrajeev@linux.vnet.ibm.com>
References: <20230911154822.2559213-1-kan.liang@linux.intel.com>
 <20231002154535.GB35785@noisy.programming.kicks-ass.net>
From:   "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <20231002154535.GB35785@noisy.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2023-10-02 11:45 a.m., Peter Zijlstra wrote:
> 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;
> }

I tried to avoid the above extra calculation (although it should be
tiny), since it's in a NMI handler. So I once planned to add an extra
struct perf_branch_entry_ext    lbr_entries_ext[MAX_LBR_ENTRIES]; which
doubles the space.
But yes, it should be doable.

> 
>> 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.

I don't see big drawbacks to it. Easier to understand and implement, so
should be easier to maintain as well.
I guess I will still use the latter, if no objection.

> 
>> 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?

Users may only collect the occurrences of some events in a group. The
EVT_CNTRS flag is used to indicate those events. E.g.,
perf record -e "{cpu/branch-instructions,branch_type=call/,
cpu/branch-misses,branch_type=event/}"

Only the occurrences of the branch-misses event is collected in LBR and
finally dumped into the extra buffer.

While the first flag, PERF_SAMPLE_BRANCH_EXTRA, only tells that the
extra space is required.

> 
> 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.

Now, it's documented in the Intel® Architecture Instruction Set
Extensions and Future Features, Chapter 8, 8.6 LBR ENHANCEMENTS. It
should be moved to SDM later.
https://cdrdv2.intel.com/v1/dl/getContent/671368

Only 2 bits for each counter. Saturating at a value of 3.

Thanks,
Kan