Re: [PATCH v1 5/6] perf tools: Use pmus to describe type from attribute

[Liang, Kan]

On 2024-03-07 3:14 a.m., Ian Rogers wrote:
> When dumping a perf_event_attr, use pmus to find the PMU and its name
> by the type number. This allows dynamically added PMUs to be described.
> 
> Before:
> ```
> $ perf stat -vv -e data_read true
> ...
> perf_event_attr:
>   type                             24
>   size                             136
>   config                           0x20ff
>   sample_type                      IDENTIFIER
>   read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>   disabled                         1
>   inherit                          1
>   exclude_guest                    1
> ...
> ```
> 
> After:
> ```
> $ perf stat -vv -e data_read true
> ...
> perf_event_attr:
>   type                             24 (uncore_imc_free_running_0)
>   size                             136
>   config                           0x20ff
>   sample_type                      IDENTIFIER
>   read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>   disabled                         1
>   inherit                          1
>   exclude_guest                    1
> ...
> ```
> 
> However, it also means that when we have a PMU name we prefer it to a
> hard coded name:
> 
> Before:
> ```
> $ perf stat -vv -e cycles true

faults?

Thanks,
Kan
> ...
> perf_event_attr:
>   type                             1 (PERF_TYPE_SOFTWARE)
>   size                             136
>   config                           0x2 (PERF_COUNT_SW_PAGE_FAULTS)
>   sample_type                      IDENTIFIER
>   read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>   disabled                         1
>   inherit                          1
>   enable_on_exec                   1
>   exclude_guest                    1
> ...
> ```
> 
> After:
> ```
> $ perf stat -vv -e faults true
> ...
> perf_event_attr:
>   type                             1 (software)
>   size                             136
>   config                           0x2 (PERF_COUNT_SW_PAGE_FAULTS)
>   sample_type                      IDENTIFIER
>   read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>   disabled                         1
>   inherit                          1
>   enable_on_exec                   1
>   exclude_guest                    1
> ...
> ```
> 
> It feels more consistent to do this, rather than only prefer a PMU
> name when a hard coded name isn't available.
> 
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>  tools/perf/util/perf_event_attr_fprintf.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/perf_event_attr_fprintf.c b/tools/perf/util/perf_event_attr_fprintf.c
> index 8f04d3b7f3ec..29e66835da3a 100644
> --- a/tools/perf/util/perf_event_attr_fprintf.c
> +++ b/tools/perf/util/perf_event_attr_fprintf.c
> @@ -7,6 +7,8 @@
>  #include <linux/types.h>
>  #include <linux/perf_event.h>
>  #include "util/evsel_fprintf.h"
> +#include "util/pmu.h"
> +#include "util/pmus.h"
>  #include "trace-event.h"
>  
>  struct bit_names {
> @@ -75,9 +77,12 @@ static void __p_read_format(char *buf, size_t size, u64 value)
>  }
>  
>  #define ENUM_ID_TO_STR_CASE(x) case x: return (#x);
> -static const char *stringify_perf_type_id(u64 value)
> +static const char *stringify_perf_type_id(struct perf_pmu *pmu, u32 type)
>  {
> -	switch (value) {
> +	if (pmu)
> +		return pmu->name;
> +
> +	switch (type) {
>  	ENUM_ID_TO_STR_CASE(PERF_TYPE_HARDWARE)
>  	ENUM_ID_TO_STR_CASE(PERF_TYPE_SOFTWARE)
>  	ENUM_ID_TO_STR_CASE(PERF_TYPE_TRACEPOINT)
> @@ -175,9 +180,9 @@ do {								\
>  #define print_id_unsigned(_s)	PRINT_ID(_s, "%"PRIu64)
>  #define print_id_hex(_s)	PRINT_ID(_s, "%#"PRIx64)
>  
> -static void __p_type_id(char *buf, size_t size, u64 value)
> +static void __p_type_id(struct perf_pmu *pmu, char *buf, size_t size, u64 value)
>  {
> -	print_id_unsigned(stringify_perf_type_id(value));
> +	print_id_unsigned(stringify_perf_type_id(pmu, value));
>  }
>  
>  static void __p_config_hw_id(char *buf, size_t size, u64 value)
> @@ -246,7 +251,7 @@ static void __p_config_id(char *buf, size_t size, u32 type, u64 value)
>  #define p_sample_type(val)	__p_sample_type(buf, BUF_SIZE, val)
>  #define p_branch_sample_type(val) __p_branch_sample_type(buf, BUF_SIZE, val)
>  #define p_read_format(val)	__p_read_format(buf, BUF_SIZE, val)
> -#define p_type_id(val)		__p_type_id(buf, BUF_SIZE, val)
> +#define p_type_id(val)		__p_type_id(pmu, buf, BUF_SIZE, val)
>  #define p_config_id(val)	__p_config_id(buf, BUF_SIZE, attr->type, val)
>  
>  #define PRINT_ATTRn(_n, _f, _p, _a)			\
> @@ -262,6 +267,7 @@ do {							\
>  int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
>  			     attr__fprintf_f attr__fprintf, void *priv)
>  {
> +	struct perf_pmu *pmu = perf_pmus__find_by_type(attr->type);
>  	char buf[BUF_SIZE];
>  	int ret = 0;
>