Re: [PATCH 1/2] perf: Add more generic branch types

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 2/2/22 5:28 PM, Mark Rutland wrote:
> Hi Anshuman,
> 
> On Fri, Jan 28, 2022 at 11:14:12AM +0530, Anshuman Khandual wrote:
>> This expands generic branch type classification by adding some more entries
>> , that can still be represented with the existing 4 bit 'type' field. While

    ^
>> here this also updates the x86 implementation with these new branch types.
> 
> It looks like there's some whitespace damage here.

Are you referring the above ? I will have a look.

> 
>>From a quick scan of the below, I think the "exception return" and "IRQ
> exception" types are somewhat generic, and could be added now (aside from any
> bikeshedding over names), but:
> 
> * For IRQ vs FIQ, we might just want to have a top-level "asynchronous
>   exception" type, and then further divide that with a separate field. That way
>   it's easier to extend in future if new exceptions are added.

Okay. But that might lead to a hierarchical bit fields design where as the
current one is just linear.

> 
> * I don't think the debug state entry/exits make sense as generic branch types,

From BRBE perspective, a branch is any control flow change including exception
level change, debug enter, debug exit etc. If exception and its return can be
classified as 'branches' why not debug state change ? Are there no similar
debug states transition on other platforms ?

>   since those are somewhat specific to the ARM architecutre, and it might make
>   sense to define generic PERF_BR_ARCH* definitions instead.

Makes sense but corresponding bit field layout change in branch_sample_type
will remain a challenge.

> 
> * Given the next patch extends the field, and therei are potential ABI problems
>   with that, we might need to reserve a value for ABI extensibility purposes,
>   and I suspect we need to do that *first*. More comments on the subsequent
>   patch.

Sure, understood.

>  
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-perf-users@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/x86/events/intel/lbr.c           | 4 ++--
>>  include/uapi/linux/perf_event.h       | 5 +++++
>>  tools/include/uapi/linux/perf_event.h | 5 +++++
>>  tools/perf/util/branch.c              | 7 ++++++-
>>  4 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
>> index 8043213b75a5..9f86fac8c6a5 100644
>> --- a/arch/x86/events/intel/lbr.c
>> +++ b/arch/x86/events/intel/lbr.c
>> @@ -1336,10 +1336,10 @@ static int branch_map[X86_BR_TYPE_MAP_MAX] = {
>>  	PERF_BR_SYSCALL,	/* X86_BR_SYSCALL */
>>  	PERF_BR_SYSRET,		/* X86_BR_SYSRET */
>>  	PERF_BR_UNKNOWN,	/* X86_BR_INT */
>> -	PERF_BR_UNKNOWN,	/* X86_BR_IRET */
>> +	PERF_BR_EXPT_RET,	/* X86_BR_IRET */
>>  	PERF_BR_COND,		/* X86_BR_JCC */
>>  	PERF_BR_UNCOND,		/* X86_BR_JMP */
>> -	PERF_BR_UNKNOWN,	/* X86_BR_IRQ */
>> +	PERF_BR_IRQ,		/* X86_BR_IRQ */
>>  	PERF_BR_IND_CALL,	/* X86_BR_IND_CALL */
>>  	PERF_BR_UNKNOWN,	/* X86_BR_ABORT */
>>  	PERF_BR_UNKNOWN,	/* X86_BR_IN_TX */
> 
> This presumably changes the values reported to userspace, so the commit message
> should mention that and explain why that is not a problem.

Okay.

> 
>> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
>> index 1b65042ab1db..b91d0f575d0c 100644
>> --- a/include/uapi/linux/perf_event.h
>> +++ b/include/uapi/linux/perf_event.h
>> @@ -251,6 +251,11 @@ enum {
>>  	PERF_BR_SYSRET		= 8,	/* syscall return */
>>  	PERF_BR_COND_CALL	= 9,	/* conditional function call */
>>  	PERF_BR_COND_RET	= 10,	/* conditional function return */
>> +	PERF_BR_EXPT_RET	= 11,	/* exception return */
> 
> We don't use 'EXPT' anywhere else, so it might be better to just use 'ERET'.
> IIUC that's the naming the x86 FRED stuff is going to use anyhow.

Sure, will change.

> 
>> +	PERF_BR_IRQ		= 12,	/* irq */
> 
> This looks somewhat generic, so adding it now makes sense to me, but ...
> 
>> +	PERF_BR_FIQ		= 13,	/* fiq */
> 
> ... this is arguably just a idfferent class of interrupt from the PoV of Linux,
> and the naming is ARM-specific, so I don't think this makes sense to add *now*.

I assume 'now' --> without ABI extension.

> As above, maybe it would be better to have a generic "aynchronous exception" or
> "interrupt" type, and a separate field to distinguish specific types of those.
> 
>> +	PERF_BR_DEBUG_HALT	= 14,	/* debug halt */
>> +	PERF_BR_DEBUG_EXIT	= 15,	/* debug exit */
> 
> For the benefit of those not familiar with the ARM architecture, "debug halt"
> and "debug exit" usually refer to "debug state", which is what an external
> (e.g. JTAG) debugger uses rather than the usual self-hosted debug stuff that
> Linux uses.
> 
> Given that, I'm not sure these are very generic, and I suspect it would be
> better to have more generic PERF_BR_ARCH_* entries for things like this.

Sure, will try and come up with something similar.

> 
> Thanks,
> Mark.
> 
>>  	PERF_BR_MAX,
>>  };
>>  
>> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
>> index 4cd39aaccbe7..1882054e8684 100644
>> --- a/tools/include/uapi/linux/perf_event.h
>> +++ b/tools/include/uapi/linux/perf_event.h
>> @@ -251,6 +251,11 @@ enum {
>>  	PERF_BR_SYSRET		= 8,	/* syscall return */
>>  	PERF_BR_COND_CALL	= 9,	/* conditional function call */
>>  	PERF_BR_COND_RET	= 10,	/* conditional function return */
>> +	PERF_BR_EXPT_RET	= 11,	/* exception return */
>> +	PERF_BR_IRQ		= 12,	/* irq */
>> +	PERF_BR_FIQ		= 13,	/* fiq */
>> +	PERF_BR_DEBUG_HALT	= 14,	/* debug halt */
>> +	PERF_BR_DEBUG_EXIT	= 15,	/* debug exit */
>>  	PERF_BR_MAX,
>>  };
>>  
>> diff --git a/tools/perf/util/branch.c b/tools/perf/util/branch.c
>> index 2285b1eb3128..74e5e67b1779 100644
>> --- a/tools/perf/util/branch.c
>> +++ b/tools/perf/util/branch.c
>> @@ -49,7 +49,12 @@ const char *branch_type_name(int type)
>>  		"SYSCALL",
>>  		"SYSRET",
>>  		"COND_CALL",
>> -		"COND_RET"
>> +		"COND_RET",
>> +		"EXPT_RET",
>> +		"IRQ",
>> +		"FIQ",
>> +		"DEBUG_HALT",
>> +		"DEBUG_EXIT"
>>  	};
>>  
>>  	if (type >= 0 && type < PERF_BR_MAX)
>> -- 
>> 2.25.1
>>