On Fri, Jan 10, 2025 at 11:40 AM Namhyung Kim <namhyung@xxxxxxxxxx> wrote: > > On Thu, Jan 09, 2025 at 02:21:09PM -0800, Ian Rogers wrote: > > Originally posted and merged from: > > https://lore.kernel.org/r/20240416061533.921723-10-irogers@xxxxxxxxxx > > This reverts commit 4f1b067359ac8364cdb7f9fda41085fa85789d0f although > > the patch is now smaller due to related fixes being applied in commit > > 22a4db3c3603 ("perf evsel: Add alternate_hw_config and use in > > evsel__match"). > > The original commit message was: > > > > It was requested that RISC-V be able to add events to the perf tool so > > the PMU driver didn't need to map legacy events to config encodings: > > https://lore.kernel.org/lkml/20240217005738.3744121-1-atishp@xxxxxxxxxxxx/ > > > > This change makes the priority of events specified without a PMU the > > same as those specified with a PMU, namely sysfs and JSON events are > > checked first before using the legacy encoding. > > I'm still not convinced why we need this change despite of these > troubles. If it's because RISC-V cannot define the lagacy hardware > events in the kernel driver, why not using a different name in JSON and When the discussion happened a year back. we tried to avoid defining the legacy hardware events in the kernel driver. However, we agreed that we have to define it anyways for other reasons (legacy usage + virtualization) as described here[1]. I have improved the driver in such a way that it can handle both legacy events from the driver or json file (via this patch) if available. If this patch is available, a platform vendor can choose to encode the legacy events in json. Otherwise, it has to specify them in the driver. I will try to send the series today/tomorrow. This patch will help avoid proliferation of usage of legacy events in the long run. But it is no longer absolutely necessary for RISC-V. If this patch is accepted, there is a hope that we can get rid of the specifying encodings in the driver in the distant future. However, we have to define them in the driver for reasons described in[1]. [1] https://lore.kernel.org/lkml/20241026121758.143259-1-irogers@xxxxxxxxxx/T/#m653a6b98919a365a361a698032502bd26af9f6ba > ask users to use the name specifically? Something like: > > $ perf record -e riscv-cycles ... > That was the first alternative I proposed back in 2022 plumbers :). But it was concluded that we don't want users to learn new ways of running perf in RISC-V which makes sense to me as well. > Thanks, > Namhyung > > > > > The hw_term is made more generic as a hardware_event that encodes a > > pair of string and int value, allowing parse_events_multi_pmu_add to > > fall back on a known encoding when the sysfs/JSON adding fails for > > core events. As this covers PE_VALUE_SYM_HW, that token is removed and > > related code simplified. > > > > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx> > > Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx> > > Tested-by: Atish Patra <atishp@xxxxxxxxxxxx> > > Tested-by: James Clark <james.clark@xxxxxxxxxx> > > Tested-by: Leo Yan <leo.yan@xxxxxxx> > > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx> > > Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx> > > Cc: Beeman Strong <beeman@xxxxxxxxxxxx> > > Cc: Ingo Molnar <mingo@xxxxxxxxxx> > > Cc: Jiri Olsa <jolsa@xxxxxxxxxx> > > Cc: Mark Rutland <mark.rutland@xxxxxxx> > > Cc: Namhyung Kim <namhyung@xxxxxxxxxx> > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> > > --- > > tools/perf/util/parse-events.c | 26 +++++++++--- > > tools/perf/util/parse-events.l | 76 +++++++++++++++++----------------- > > tools/perf/util/parse-events.y | 60 ++++++++++++++++++--------- > > 3 files changed, 98 insertions(+), 64 deletions(-) > > > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > > index 1e23faa364b1..3a60fca53cfa 100644 > > --- a/tools/perf/util/parse-events.c > > +++ b/tools/perf/util/parse-events.c > > @@ -1545,8 +1545,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, > > struct list_head *list = NULL; > > struct perf_pmu *pmu = NULL; > > YYLTYPE *loc = loc_; > > - int ok = 0; > > - const char *config; > > + int ok = 0, core_ok = 0; > > + const char *tmp; > > struct parse_events_terms parsed_terms; > > > > *listp = NULL; > > @@ -1559,15 +1559,15 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, > > return ret; > > } > > > > - config = strdup(event_name); > > - if (!config) > > + tmp = strdup(event_name); > > + if (!tmp) > > goto out_err; > > > > if (parse_events_term__num(&term, > > PARSE_EVENTS__TERM_TYPE_USER, > > - config, /*num=*/1, /*novalue=*/true, > > + tmp, /*num=*/1, /*novalue=*/true, > > loc, /*loc_val=*/NULL) < 0) { > > - zfree(&config); > > + zfree(&tmp); > > goto out_err; > > } > > list_add_tail(&term->list, &parsed_terms.terms); > > @@ -1598,6 +1598,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, > > pr_debug("%s -> %s/%s/\n", event_name, pmu->name, sb.buf); > > strbuf_release(&sb); > > ok++; > > + if (pmu->is_core) > > + core_ok++; > > } > > } > > > > @@ -1614,6 +1616,18 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, > > } > > } > > > > + if (hw_config != PERF_COUNT_HW_MAX && !core_ok) { > > + /* > > + * The event wasn't found on core PMUs but it has a hardware > > + * config version to try. > > + */ > > + if (!parse_events_add_numeric(parse_state, list, > > + PERF_TYPE_HARDWARE, hw_config, > > + const_parsed_terms, > > + /*wildcard=*/true)) > > + ok++; > > + } > > + > > out_err: > > parse_events_terms__exit(&parsed_terms); > > if (ok) > > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l > > index bf7f73548605..29082a22ccc9 100644 > > --- a/tools/perf/util/parse-events.l > > +++ b/tools/perf/util/parse-events.l > > @@ -113,12 +113,12 @@ do { \ > > yyless(0); \ > > } while (0) > > > > -static int sym(yyscan_t scanner, int type, int config) > > +static int sym(yyscan_t scanner, int config) > > { > > YYSTYPE *yylval = parse_events_get_lval(scanner); > > > > - yylval->num = (type << 16) + config; > > - return type == PERF_TYPE_HARDWARE ? PE_VALUE_SYM_HW : PE_VALUE_SYM_SW; > > + yylval->num = config; > > + return PE_VALUE_SYM_SW; > > } > > > > static int term(yyscan_t scanner, enum parse_events__term_type type) > > @@ -129,13 +129,13 @@ static int term(yyscan_t scanner, enum parse_events__term_type type) > > return PE_TERM; > > } > > > > -static int hw_term(yyscan_t scanner, int config) > > +static int hw(yyscan_t scanner, int config) > > { > > YYSTYPE *yylval = parse_events_get_lval(scanner); > > char *text = parse_events_get_text(scanner); > > > > - yylval->hardware_term.str = strdup(text); > > - yylval->hardware_term.num = PERF_TYPE_HARDWARE + config; > > + yylval->hardware_event.str = strdup(text); > > + yylval->hardware_event.num = config; > > return PE_TERM_HW; > > } > > > > @@ -324,16 +324,16 @@ aux-output { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT); } > > aux-action { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_ACTION); } > > aux-sample-size { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE); } > > metric-id { return term(yyscanner, PARSE_EVENTS__TERM_TYPE_METRIC_ID); } > > -cpu-cycles|cycles { return hw_term(yyscanner, PERF_COUNT_HW_CPU_CYCLES); } > > -stalled-cycles-frontend|idle-cycles-frontend { return hw_term(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND); } > > -stalled-cycles-backend|idle-cycles-backend { return hw_term(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_BACKEND); } > > -instructions { return hw_term(yyscanner, PERF_COUNT_HW_INSTRUCTIONS); } > > -cache-references { return hw_term(yyscanner, PERF_COUNT_HW_CACHE_REFERENCES); } > > -cache-misses { return hw_term(yyscanner, PERF_COUNT_HW_CACHE_MISSES); } > > -branch-instructions|branches { return hw_term(yyscanner, PERF_COUNT_HW_BRANCH_INSTRUCTIONS); } > > -branch-misses { return hw_term(yyscanner, PERF_COUNT_HW_BRANCH_MISSES); } > > -bus-cycles { return hw_term(yyscanner, PERF_COUNT_HW_BUS_CYCLES); } > > -ref-cycles { return hw_term(yyscanner, PERF_COUNT_HW_REF_CPU_CYCLES); } > > +cpu-cycles|cycles { return hw(yyscanner, PERF_COUNT_HW_CPU_CYCLES); } > > +stalled-cycles-frontend|idle-cycles-frontend { return hw(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND); } > > +stalled-cycles-backend|idle-cycles-backend { return hw(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_BACKEND); } > > +instructions { return hw(yyscanner, PERF_COUNT_HW_INSTRUCTIONS); } > > +cache-references { return hw(yyscanner, PERF_COUNT_HW_CACHE_REFERENCES); } > > +cache-misses { return hw(yyscanner, PERF_COUNT_HW_CACHE_MISSES); } > > +branch-instructions|branches { return hw(yyscanner, PERF_COUNT_HW_BRANCH_INSTRUCTIONS); } > > +branch-misses { return hw(yyscanner, PERF_COUNT_HW_BRANCH_MISSES); } > > +bus-cycles { return hw(yyscanner, PERF_COUNT_HW_BUS_CYCLES); } > > +ref-cycles { return hw(yyscanner, PERF_COUNT_HW_REF_CPU_CYCLES); } > > r{num_raw_hex} { return str(yyscanner, PE_RAW); } > > r0x{num_raw_hex} { return str(yyscanner, PE_RAW); } > > , { return ','; } > > @@ -377,28 +377,28 @@ r0x{num_raw_hex} { return str(yyscanner, PE_RAW); } > > <<EOF>> { BEGIN(INITIAL); } > > } > > > > -cpu-cycles|cycles { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_CPU_CYCLES); } > > -stalled-cycles-frontend|idle-cycles-frontend { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND); } > > -stalled-cycles-backend|idle-cycles-backend { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_STALLED_CYCLES_BACKEND); } > > -instructions { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_INSTRUCTIONS); } > > -cache-references { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_CACHE_REFERENCES); } > > -cache-misses { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_CACHE_MISSES); } > > -branch-instructions|branches { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_BRANCH_INSTRUCTIONS); } > > -branch-misses { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_BRANCH_MISSES); } > > -bus-cycles { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_BUS_CYCLES); } > > -ref-cycles { return sym(yyscanner, PERF_TYPE_HARDWARE, PERF_COUNT_HW_REF_CPU_CYCLES); } > > -cpu-clock { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_CPU_CLOCK); } > > -task-clock { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_TASK_CLOCK); } > > -page-faults|faults { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_PAGE_FAULTS); } > > -minor-faults { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_PAGE_FAULTS_MIN); } > > -major-faults { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_PAGE_FAULTS_MAJ); } > > -context-switches|cs { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_CONTEXT_SWITCHES); } > > -cpu-migrations|migrations { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_CPU_MIGRATIONS); } > > -alignment-faults { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_ALIGNMENT_FAULTS); } > > -emulation-faults { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_EMULATION_FAULTS); } > > -dummy { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_DUMMY); } > > -bpf-output { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_BPF_OUTPUT); } > > -cgroup-switches { return sym(yyscanner, PERF_TYPE_SOFTWARE, PERF_COUNT_SW_CGROUP_SWITCHES); } > > +cpu-cycles|cycles { return hw(yyscanner, PERF_COUNT_HW_CPU_CYCLES); } > > +stalled-cycles-frontend|idle-cycles-frontend { return hw(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_FRONTEND); } > > +stalled-cycles-backend|idle-cycles-backend { return hw(yyscanner, PERF_COUNT_HW_STALLED_CYCLES_BACKEND); } > > +instructions { return hw(yyscanner, PERF_COUNT_HW_INSTRUCTIONS); } > > +cache-references { return hw(yyscanner, PERF_COUNT_HW_CACHE_REFERENCES); } > > +cache-misses { return hw(yyscanner, PERF_COUNT_HW_CACHE_MISSES); } > > +branch-instructions|branches { return hw(yyscanner, PERF_COUNT_HW_BRANCH_INSTRUCTIONS); } > > +branch-misses { return hw(yyscanner, PERF_COUNT_HW_BRANCH_MISSES); } > > +bus-cycles { return hw(yyscanner, PERF_COUNT_HW_BUS_CYCLES); } > > +ref-cycles { return hw(yyscanner, PERF_COUNT_HW_REF_CPU_CYCLES); } > > +cpu-clock { return sym(yyscanner, PERF_COUNT_SW_CPU_CLOCK); } > > +task-clock { return sym(yyscanner, PERF_COUNT_SW_TASK_CLOCK); } > > +page-faults|faults { return sym(yyscanner, PERF_COUNT_SW_PAGE_FAULTS); } > > +minor-faults { return sym(yyscanner, PERF_COUNT_SW_PAGE_FAULTS_MIN); } > > +major-faults { return sym(yyscanner, PERF_COUNT_SW_PAGE_FAULTS_MAJ); } > > +context-switches|cs { return sym(yyscanner, PERF_COUNT_SW_CONTEXT_SWITCHES); } > > +cpu-migrations|migrations { return sym(yyscanner, PERF_COUNT_SW_CPU_MIGRATIONS); } > > +alignment-faults { return sym(yyscanner, PERF_COUNT_SW_ALIGNMENT_FAULTS); } > > +emulation-faults { return sym(yyscanner, PERF_COUNT_SW_EMULATION_FAULTS); } > > +dummy { return sym(yyscanner, PERF_COUNT_SW_DUMMY); } > > +bpf-output { return sym(yyscanner, PERF_COUNT_SW_BPF_OUTPUT); } > > +cgroup-switches { return sym(yyscanner, PERF_COUNT_SW_CGROUP_SWITCHES); } > > > > {lc_type} { return str(yyscanner, PE_LEGACY_CACHE); } > > {lc_type}-{lc_op_result} { return str(yyscanner, PE_LEGACY_CACHE); } > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > > index f888cbb076d6..d2ef1890007e 100644 > > --- a/tools/perf/util/parse-events.y > > +++ b/tools/perf/util/parse-events.y > > @@ -55,7 +55,7 @@ static void free_list_evsel(struct list_head* list_evsel) > > %} > > > > %token PE_START_EVENTS PE_START_TERMS > > -%token PE_VALUE PE_VALUE_SYM_HW PE_VALUE_SYM_SW PE_TERM > > +%token PE_VALUE PE_VALUE_SYM_SW PE_TERM > > %token PE_EVENT_NAME > > %token PE_RAW PE_NAME > > %token PE_MODIFIER_EVENT PE_MODIFIER_BP PE_BP_COLON PE_BP_SLASH > > @@ -65,11 +65,9 @@ static void free_list_evsel(struct list_head* list_evsel) > > %token PE_DRV_CFG_TERM > > %token PE_TERM_HW > > %type <num> PE_VALUE > > -%type <num> PE_VALUE_SYM_HW > > %type <num> PE_VALUE_SYM_SW > > %type <mod> PE_MODIFIER_EVENT > > %type <term_type> PE_TERM > > -%type <num> value_sym > > %type <str> PE_RAW > > %type <str> PE_NAME > > %type <str> PE_LEGACY_CACHE > > @@ -85,6 +83,7 @@ static void free_list_evsel(struct list_head* list_evsel) > > %type <list_terms> opt_pmu_config > > %destructor { parse_events_terms__delete ($$); } <list_terms> > > %type <list_evsel> event_pmu > > +%type <list_evsel> event_legacy_hardware > > %type <list_evsel> event_legacy_symbol > > %type <list_evsel> event_legacy_cache > > %type <list_evsel> event_legacy_mem > > @@ -102,8 +101,8 @@ static void free_list_evsel(struct list_head* list_evsel) > > %destructor { free_list_evsel ($$); } <list_evsel> > > %type <tracepoint_name> tracepoint_name > > %destructor { free ($$.sys); free ($$.event); } <tracepoint_name> > > -%type <hardware_term> PE_TERM_HW > > -%destructor { free ($$.str); } <hardware_term> > > +%type <hardware_event> PE_TERM_HW > > +%destructor { free ($$.str); } <hardware_event> > > > > %union > > { > > @@ -118,10 +117,10 @@ static void free_list_evsel(struct list_head* list_evsel) > > char *sys; > > char *event; > > } tracepoint_name; > > - struct hardware_term { > > + struct hardware_event { > > char *str; > > u64 num; > > - } hardware_term; > > + } hardware_event; > > } > > %% > > > > @@ -264,6 +263,7 @@ PE_EVENT_NAME event_def > > event_def > > > > event_def: event_pmu | > > + event_legacy_hardware | > > event_legacy_symbol | > > event_legacy_cache sep_dc | > > event_legacy_mem sep_dc | > > @@ -306,24 +306,45 @@ PE_NAME sep_dc > > $$ = list; > > } > > > > -value_sym: > > -PE_VALUE_SYM_HW > > +event_legacy_hardware: > > +PE_TERM_HW opt_pmu_config > > +{ > > + /* List of created evsels. */ > > + struct list_head *list = NULL; > > + int err = parse_events_multi_pmu_add(_parse_state, $1.str, $1.num, $2, &list, &@1); > > + > > + free($1.str); > > + parse_events_terms__delete($2); > > + if (err) > > + PE_ABORT(err); > > + > > + $$ = list; > > +} > > | > > -PE_VALUE_SYM_SW > > +PE_TERM_HW sep_dc > > +{ > > + struct list_head *list; > > + int err; > > + > > + err = parse_events_multi_pmu_add(_parse_state, $1.str, $1.num, NULL, &list, &@1); > > + free($1.str); > > + if (err) > > + PE_ABORT(err); > > + $$ = list; > > +} > > > > event_legacy_symbol: > > -value_sym '/' event_config '/' > > +PE_VALUE_SYM_SW '/' event_config '/' > > { > > struct list_head *list; > > - int type = $1 >> 16; > > - int config = $1 & 255; > > int err; > > - bool wildcard = (type == PERF_TYPE_HARDWARE || type == PERF_TYPE_HW_CACHE); > > > > list = alloc_list(); > > if (!list) > > YYNOMEM; > > - err = parse_events_add_numeric(_parse_state, list, type, config, $3, wildcard); > > + err = parse_events_add_numeric(_parse_state, list, > > + /*type=*/PERF_TYPE_SOFTWARE, /*config=*/$1, > > + $3, /*wildcard=*/false); > > parse_events_terms__delete($3); > > if (err) { > > free_list_evsel(list); > > @@ -332,18 +353,17 @@ value_sym '/' event_config '/' > > $$ = list; > > } > > | > > -value_sym sep_slash_slash_dc > > +PE_VALUE_SYM_SW sep_slash_slash_dc > > { > > struct list_head *list; > > - int type = $1 >> 16; > > - int config = $1 & 255; > > - bool wildcard = (type == PERF_TYPE_HARDWARE || type == PERF_TYPE_HW_CACHE); > > int err; > > > > list = alloc_list(); > > if (!list) > > YYNOMEM; > > - err = parse_events_add_numeric(_parse_state, list, type, config, /*head_config=*/NULL, wildcard); > > + err = parse_events_add_numeric(_parse_state, list, > > + /*type=*/PERF_TYPE_SOFTWARE, /*config=*/$1, > > + /*head_config=*/NULL, /*wildcard=*/false); > > if (err) > > PE_ABORT(err); > > $$ = list; > > -- > > 2.47.1.613.gc27f4b7a9f-goog > >
