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 <[email protected]>
> ---
> 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;
>
On Thu, Mar 7, 2024 at 6:36 AM Liang, Kan <[email protected]> wrote:
>
>
>
> 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, will fix in v2. It does highlight we lack a struct pmu in pmus
for legacy types like hardware, hence switching to faults as a
software event that has a PMU. Perhaps we should just make legacy PMUs
anyway, to avoid special case logic. Anyway, not in these changes.
Thanks,
Ian
> 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 <[email protected]>
> > ---
> > 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;
> >