Re: [PATCH V2 7/8] perf/tools: Extend branch type classification

[James Clark]

On 09/03/2022 03:36, Anshuman Khandual wrote:
> This updates the perf tool with generic branch type classification with new
> ABI extender place holder i.e PERF_BR_EXTEND_ABI, the new 4 bit branch type
> field i.e perf_branch_entry.new_type, new generic page fault related branch
> types and some arch specific branch types as added earlier in the kernel.
> 
> 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: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 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>
> ---
>  tools/include/uapi/linux/perf_event.h | 16 +++++++++++++++-
>  tools/perf/util/branch.c              |  3 ++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
> index 26d8f0b5ac0d..d29280adc3c4 100644
> --- a/tools/include/uapi/linux/perf_event.h
> +++ b/tools/include/uapi/linux/perf_event.h
> @@ -255,9 +255,22 @@ enum {
>  	PERF_BR_IRQ		= 12,	/* irq */
>  	PERF_BR_SERROR		= 13,	/* system error */
>  	PERF_BR_NO_TX		= 14,	/* not in transaction */
> +	PERF_BR_EXTEND_ABI	= 15,	/* extend ABI */
>  	PERF_BR_MAX,
>  };
>  
> +enum {
> +	PERF_BR_NEW_FAULT_ALGN		= 0,    /* Alignment fault */
> +	PERF_BR_NEW_FAULT_DATA		= 1,    /* Data fault */
> +	PERF_BR_NEW_FAULT_INST		= 2,    /* Inst fault */
> +	PERF_BR_NEW_ARCH_1		= 3,    /* Architecture specific */
> +	PERF_BR_NEW_ARCH_2		= 4,    /* Architecture specific */
> +	PERF_BR_NEW_ARCH_3		= 5,    /* Architecture specific */
> +	PERF_BR_NEW_ARCH_4		= 6,    /* Architecture specific */
> +	PERF_BR_NEW_ARCH_5		= 7,    /* Architecture specific */
> +	PERF_BR_NEW_MAX,
> +};
> +
>  #define PERF_SAMPLE_BRANCH_PLM_ALL \
>  	(PERF_SAMPLE_BRANCH_USER|\
>  	 PERF_SAMPLE_BRANCH_KERNEL|\
> @@ -1372,7 +1385,8 @@ struct perf_branch_entry {
>  		abort:1,    /* transaction abort */
>  		cycles:16,  /* cycle count to last branch */
>  		type:4,     /* branch type */
> -		reserved:40;
> +		new_type:4, /* additional branch type */
> +		reserved:36;
>  };
>  
>  union perf_sample_weight {
> diff --git a/tools/perf/util/branch.c b/tools/perf/util/branch.c
> index abc673347bee..4bd52de0527c 100644
> --- a/tools/perf/util/branch.c
> +++ b/tools/perf/util/branch.c
> @@ -53,7 +53,8 @@ const char *branch_type_name(int type)
>  		"ERET",
>  		"IRQ",
>  		"SERROR",
> -		"NO_TX"
> +		"NO_TX",
> +		"EXTEND_ABI"


Shouldn't we hide this implementation detail from users? They just want to know
the branch type, they don't want to see the extend_abi stuff.

It should be possible to fix all the perf internals so that it's transparent and
any code using or printing the branch type has some accessor that works out what
the final type is rather than having to re-implement that logic everywhere.

So I don't think adding the string "EXTEND_ABI" would be needed because it would
never be shown.

If we just want to discuss the new extend ABI kernel side changes then I think this
patch can be dropped and we can do the full perf tool side implementation
in a more complete way later.

James

>  	};
>  
>  	if (type >= 0 && type < PERF_BR_MAX)