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

[Mark Rutland]

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.

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

* I don't think the debug state entry/exits make sense as generic branch types,
  since those are somewhat specific to the ARM architecutre, and it might make
  sense to define generic PERF_BR_ARCH* definitions instead.

* 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.
 
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-perf-users@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> ---
>  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.

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

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

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
>