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

[Liang, Kan]

On 2023-10-02 5:37 p.m., Peter Zijlstra wrote:
> On Mon, Oct 02, 2023 at 03:19:04PM -0400, Liang, Kan wrote:
> 
>>>> 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.
> 
> Or have it implicit, I reallt don't see the point of having two bits
> here.

Perf has to traverse the whole group to decide whether using the extra
space. But It should be possible to use an internal flag to avoid the
traverse every time. Let me have a try.

> 
>>> 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.
> 
> Urgh, this ISE document is shite, that thing don't say how many
> IA32_LBR_INFO.PMCx_CNT fields there are, I think your later patch says
> 4, right? And is this for arch LBR or the other thing?
>

It's for Arch LBR. Yes, the current CPUID enumeration implies that only
4 counters.

"Per-counter support for LBR Event Logging is indicated by the “Event
Logging Supported” bitmap in CPUID.(EAX=01CH, ECX=0).ECX[19:16]"

> (Also, what is IA32_LER_x_INFO ?)

Last Event Record (LER). It records the last taken branch preceding the
last exception, hardware interrupt, or software interrupt.
Linux doesn't have it supported.

> 
> This is then a grant total of 8 bits.
> 
> And we still have 31 spare bits in perf_branch_entry.
> 
> Why again do we need the extra u64 ?!?
>

The first version utilizes the spare bits in perf_branch_entry.
https://lore.kernel.org/lkml/20230414145324.GB761523@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

To address the similar concern (what should we do if more counters and a
wider bits are added later), I changed it to the extra space method
since V2.

Another consideration is that the 'events' field in the
perf_branch_entry from V1 is Intel specific. The u64 extra space is more
generic. Other ARCHs can utilize it to store other extra information if
they want.

Please let me know if I'm overthinking. I can switch back to the
'events' field of V1.

> More specifically, this interface is pretty crap -- suppose the next
> generation of things feels that 2 bits aint' enough and goes and gives
> us 4. Then what do we do?
>

The current LBR is an architectural feature. The existed fields of 2
bits 4 counters should not be changed.
But yes, it's possible to add more bits and counters into the reserved
bits. The reserved bits of the IA32_LBR_x_INFO are only 31 now. The u64
extra space should be good enough.
If more information is introduced later (e.g., a brand new
LBR_x_INFO_2), then we can add a extra_2 space.

But I don't see there is a plan to extend the IA32_LBR_x_INFO again in
the near future.

> Did I already say that the ISE document raises more questions than it
> provides answers?

Yes. Would an improved CPUID enumeration can address the questions? For
example, the CPUID enumeration can give the maximum number of counters
and supported width? I think we can discuss it with the architect.

Thanks,
Kan