| Message ID | 20240416061533.921723-14-irogers@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Consistently prefer sysfs/json events | expand |
On 2024-04-16 2:15 a.m., Ian Rogers wrote: > Use a struct/bitmap rather than a copied string from lexer. > > In lexer give improved error message when too many precise flags are > given or repeated modifiers. > > Before: > ``` > $ perf stat -e 'cycles:kuk' true > event syntax error: 'cycles:kuk' > \___ Bad modifier > ... > $ perf stat -e 'cycles:pppp' true > event syntax error: 'cycles:pppp' > \___ Bad modifier > ... > $ perf stat -e '{instructions:p,cycles:pp}:pp' -a true > event syntax error: '..cycles:pp}:pp' > \___ Bad modifier > ... > ``` > After: > ``` > $ perf stat -e 'cycles:kuk' true > event syntax error: 'cycles:kuk' > \___ Duplicate modifier 'k' (kernel) > ... > $ perf stat -e 'cycles:pppp' true > event syntax error: 'cycles:pppp' > \___ Maximum precise value is 3 > ... > $ perf stat -e '{instructions:p,cycles:pp}:pp' true > event syntax error: '..cycles:pp}:pp' > \___ Maximum combined precise value is 3, adding precision to "cycles:pp" > ... > ``` > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/parse-events.c | 250 ++++++++++++--------------------- > tools/perf/util/parse-events.h | 23 ++- > tools/perf/util/parse-events.l | 75 +++++++++- > tools/perf/util/parse-events.y | 28 +--- > 4 files changed, 194 insertions(+), 182 deletions(-) > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index ebada37ef98a..3ab533d0e653 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -1700,12 +1700,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state > return -EINVAL; > } > > -int parse_events__modifier_group(struct list_head *list, > - char *event_mod) > -{ > - return parse_events__modifier_event(list, event_mod, true); > -} > - > void parse_events__set_leader(char *name, struct list_head *list) > { > struct evsel *leader; > @@ -1720,183 +1714,125 @@ void parse_events__set_leader(char *name, struct list_head *list) > leader->group_name = name; > } > > -struct event_modifier { > - int eu; > - int ek; > - int eh; > - int eH; > - int eG; > - int eI; > - int precise; > - int precise_max; > - int exclude_GH; > - int sample_read; > - int pinned; > - int weak; > - int exclusive; > - int bpf_counter; > -}; > +static int parse_events__modifier_list(struct parse_events_state *parse_state, > + YYLTYPE *loc, > + struct list_head *list, > + struct parse_events_modifier mod, > + bool group) > +{ > + struct evsel *evsel; > > -static int get_event_modifier(struct event_modifier *mod, char *str, > - struct evsel *evsel) > -{ > - int eu = evsel ? evsel->core.attr.exclude_user : 0; > - int ek = evsel ? evsel->core.attr.exclude_kernel : 0; > - int eh = evsel ? evsel->core.attr.exclude_hv : 0; > - int eH = evsel ? evsel->core.attr.exclude_host : 0; > - int eG = evsel ? evsel->core.attr.exclude_guest : 0; > - int eI = evsel ? evsel->core.attr.exclude_idle : 0; > - int precise = evsel ? evsel->core.attr.precise_ip : 0; > - int precise_max = 0; > - int sample_read = 0; > - int pinned = evsel ? evsel->core.attr.pinned : 0; > - int exclusive = evsel ? evsel->core.attr.exclusive : 0; > - > - int exclude = eu | ek | eh; > - int exclude_GH = evsel ? evsel->exclude_GH : 0; > - int weak = 0; > - int bpf_counter = 0; > - > - memset(mod, 0, sizeof(*mod)); > - > - while (*str) { > - if (*str == 'u') { > + if (!group && mod.weak) { > + parse_events_error__handle(parse_state->error, loc->first_column, > + strdup("Weak modifier is for use with groups"), NULL); > + return -EINVAL; > + } > + > + __evlist__for_each_entry(list, evsel) { > + /* Translate modifiers into the equivalent evsel excludes. */ > + int eu = group ? evsel->core.attr.exclude_user : 0; > + int ek = group ? evsel->core.attr.exclude_kernel : 0; > + int eh = group ? evsel->core.attr.exclude_hv : 0; > + int eH = group ? evsel->core.attr.exclude_host : 0; > + int eG = group ? evsel->core.attr.exclude_guest : 0; > + int exclude = eu | ek | eh; > + int exclude_GH = group ? evsel->exclude_GH : 0; > + > + if (mod.precise) { > + /* use of precise requires exclude_guest */ > + eG = 1; > + } > + if (mod.user) { > if (!exclude) > exclude = eu = ek = eh = 1; > if (!exclude_GH && !perf_guest) > eG = 1; > eu = 0; > - } else if (*str == 'k') { > + } > + if (mod.kernel) { > if (!exclude) > exclude = eu = ek = eh = 1; > ek = 0; > - } else if (*str == 'h') { > + } > + if (mod.hypervisor) { > if (!exclude) > exclude = eu = ek = eh = 1; > eh = 0; > - } else if (*str == 'G') { > + } > + if (mod.guest) { > if (!exclude_GH) > exclude_GH = eG = eH = 1; > eG = 0; > - } else if (*str == 'H') { > + } > + if (mod.host) { > if (!exclude_GH) > exclude_GH = eG = eH = 1; > eH = 0; > - } else if (*str == 'I') { > - eI = 1; > - } else if (*str == 'p') { > - precise++; > - /* use of precise requires exclude_guest */ > - if (!exclude_GH) > - eG = 1; > - } else if (*str == 'P') { > - precise_max = 1; > - } else if (*str == 'S') { > - sample_read = 1; > - } else if (*str == 'D') { > - pinned = 1; > - } else if (*str == 'e') { > - exclusive = 1; > - } else if (*str == 'W') { > - weak = 1; > - } else if (*str == 'b') { > - bpf_counter = 1; > - } else > - break; > - > - ++str; > + } > + evsel->core.attr.exclude_user = eu; > + evsel->core.attr.exclude_kernel = ek; > + evsel->core.attr.exclude_hv = eh; > + evsel->core.attr.exclude_host = eH; > + evsel->core.attr.exclude_guest = eG; > + evsel->exclude_GH = exclude_GH; > + > + /* Simple modifiers copied to the evsel. */ > + if (mod.precise) { > + u8 precise = evsel->core.attr.precise_ip + mod.precise; > + /* > + * precise ip: > + * > + * 0 - SAMPLE_IP can have arbitrary skid > + * 1 - SAMPLE_IP must have constant skid > + * 2 - SAMPLE_IP requested to have 0 skid > + * 3 - SAMPLE_IP must have 0 skid > + * > + * See also PERF_RECORD_MISC_EXACT_IP > + */ > + if (precise > 3) { The pmu_max_precise() should return the max precise the current kernel supports. It checks the /sys/devices/cpu/caps/max_precise. I think we should use that value rather than hard code it to 3. Thanks, Kan > + char *help; > + > + if (asprintf(&help, > + "Maximum combined precise value is 3, adding precision to \"%s\"", > + evsel__name(evsel)) > 0) { > + parse_events_error__handle(parse_state->error, > + loc->first_column, > + help, NULL); > + } > + return -EINVAL; > + } > + evsel->core.attr.precise_ip = precise; > + } > + if (mod.precise_max) > + evsel->precise_max = 1; > + if (mod.non_idle) > + evsel->core.attr.exclude_idle = 1; > + if (mod.sample_read) > + evsel->sample_read = 1; > + if (mod.pinned && evsel__is_group_leader(evsel)) > + evsel->core.attr.pinned = 1; > + if (mod.exclusive && evsel__is_group_leader(evsel)) > + evsel->core.attr.exclusive = 1; > + if (mod.weak) > + evsel->weak_group = true; > + if (mod.bpf) > + evsel->bpf_counter = true; > } > - > - /* > - * precise ip: > - * > - * 0 - SAMPLE_IP can have arbitrary skid > - * 1 - SAMPLE_IP must have constant skid > - * 2 - SAMPLE_IP requested to have 0 skid > - * 3 - SAMPLE_IP must have 0 skid > - * > - * See also PERF_RECORD_MISC_EXACT_IP > - */ > - if (precise > 3) > - return -EINVAL; > - > - mod->eu = eu; > - mod->ek = ek; > - mod->eh = eh; > - mod->eH = eH; > - mod->eG = eG; > - mod->eI = eI; > - mod->precise = precise; > - mod->precise_max = precise_max; > - mod->exclude_GH = exclude_GH; > - mod->sample_read = sample_read; > - mod->pinned = pinned; > - mod->weak = weak; > - mod->bpf_counter = bpf_counter; > - mod->exclusive = exclusive; > - > return 0; > } > > -/* > - * Basic modifier sanity check to validate it contains only one > - * instance of any modifier (apart from 'p') present. > - */ > -static int check_modifier(char *str) > +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc, > + struct list_head *list, > + struct parse_events_modifier mod) > { > - char *p = str; > - > - /* The sizeof includes 0 byte as well. */ > - if (strlen(str) > (sizeof("ukhGHpppPSDIWeb") - 1)) > - return -1; > - > - while (*p) { > - if (*p != 'p' && strchr(p + 1, *p)) > - return -1; > - p++; > - } > - > - return 0; > + return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/true); > } > > -int parse_events__modifier_event(struct list_head *list, char *str, bool add) > +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc, > + struct list_head *list, > + struct parse_events_modifier mod) > { > - struct evsel *evsel; > - struct event_modifier mod; > - > - if (str == NULL) > - return 0; > - > - if (check_modifier(str)) > - return -EINVAL; > - > - if (!add && get_event_modifier(&mod, str, NULL)) > - return -EINVAL; > - > - __evlist__for_each_entry(list, evsel) { > - if (add && get_event_modifier(&mod, str, evsel)) > - return -EINVAL; > - > - evsel->core.attr.exclude_user = mod.eu; > - evsel->core.attr.exclude_kernel = mod.ek; > - evsel->core.attr.exclude_hv = mod.eh; > - evsel->core.attr.precise_ip = mod.precise; > - evsel->core.attr.exclude_host = mod.eH; > - evsel->core.attr.exclude_guest = mod.eG; > - evsel->core.attr.exclude_idle = mod.eI; > - evsel->exclude_GH = mod.exclude_GH; > - evsel->sample_read = mod.sample_read; > - evsel->precise_max = mod.precise_max; > - evsel->weak_group = mod.weak; > - evsel->bpf_counter = mod.bpf_counter; > - > - if (evsel__is_group_leader(evsel)) { > - evsel->core.attr.pinned = mod.pinned; > - evsel->core.attr.exclusive = mod.exclusive; > - } > - } > - > - return 0; > + return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/false); > } > > int parse_events_name(struct list_head *list, const char *name) > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h > index 290ae6c72ec5..f104faef1a78 100644 > --- a/tools/perf/util/parse-events.h > +++ b/tools/perf/util/parse-events.h > @@ -186,8 +186,27 @@ void parse_events_terms__init(struct parse_events_terms *terms); > void parse_events_terms__exit(struct parse_events_terms *terms); > int parse_events_terms(struct parse_events_terms *terms, const char *str, FILE *input); > int parse_events_terms__to_strbuf(const struct parse_events_terms *terms, struct strbuf *sb); > -int parse_events__modifier_event(struct list_head *list, char *str, bool add); > -int parse_events__modifier_group(struct list_head *list, char *event_mod); > + > +struct parse_events_modifier { > + u8 precise; /* Number of repeated 'p' for precision. */ > + bool precise_max : 1; /* 'P' */ > + bool non_idle : 1; /* 'I' */ > + bool sample_read : 1; /* 'S' */ > + bool pinned : 1; /* 'D' */ > + bool exclusive : 1; /* 'e' */ > + bool weak : 1; /* 'W' */ > + bool bpf : 1; /* 'b' */ > + bool user : 1; /* 'u' */ > + bool kernel : 1; /* 'k' */ > + bool hypervisor : 1; /* 'h' */ > + bool guest : 1; /* 'G' */ > + bool host : 1; /* 'H' */ > +}; > + > +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc, > + struct list_head *list, struct parse_events_modifier mod); > +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc, > + struct list_head *list, struct parse_events_modifier mod); > int parse_events_name(struct list_head *list, const char *name); > int parse_events_add_tracepoint(struct list_head *list, int *idx, > const char *sys, const char *event, > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l > index 0cd68c9f0d4f..4aaf0c53d9b6 100644 > --- a/tools/perf/util/parse-events.l > +++ b/tools/perf/util/parse-events.l > @@ -142,6 +142,77 @@ static int hw(yyscan_t scanner, int config) > return PE_TERM_HW; > } > > +static void modifiers_error(struct parse_events_state *parse_state, yyscan_t scanner, > + int pos, char mod_char, const char *mod_name) > +{ > + struct parse_events_error *error = parse_state->error; > + char *help = NULL; > + > + if (asprintf(&help, "Duplicate modifier '%c' (%s)", mod_char, mod_name) > 0) > + parse_events_error__handle(error, get_column(scanner) + pos, help , NULL); > +} > + > +static int modifiers(struct parse_events_state *parse_state, yyscan_t scanner) > +{ > + YYSTYPE *yylval = parse_events_get_lval(scanner); > + char *text = parse_events_get_text(scanner); > + struct parse_events_modifier mod = { .precise = 0, }; > + > + for (size_t i = 0, n = strlen(text); i < n; i++) { > +#define CASE(c, field) \ > + case c: \ > + if (mod.field) { \ > + modifiers_error(parse_state, scanner, i, c, #field); \ > + return PE_ERROR; \ > + } \ > + mod.field = true; \ > + break > + > + switch (text[i]) { > + CASE('u', user); > + CASE('k', kernel); > + CASE('h', hypervisor); > + CASE('I', non_idle); > + CASE('G', guest); > + CASE('H', host); > + case 'p': > + mod.precise++; > + /* > + * precise ip: > + * > + * 0 - SAMPLE_IP can have arbitrary skid > + * 1 - SAMPLE_IP must have constant skid > + * 2 - SAMPLE_IP requested to have 0 skid > + * 3 - SAMPLE_IP must have 0 skid > + * > + * See also PERF_RECORD_MISC_EXACT_IP > + */ > + if (mod.precise > 3) { > + struct parse_events_error *error = parse_state->error; > + char *help = strdup("Maximum precise value is 3"); > + > + if (help) { > + parse_events_error__handle(error, get_column(scanner) + i, > + help , NULL); > + } > + return PE_ERROR; > + } > + break; > + CASE('P', precise_max); > + CASE('S', sample_read); > + CASE('D', pinned); > + CASE('W', weak); > + CASE('e', exclusive); > + CASE('b', bpf); > + default: > + return PE_ERROR; > + } > +#undef CASE > + } > + yylval->mod = mod; > + return PE_MODIFIER_EVENT; > +} > + > #define YY_USER_ACTION \ > do { \ > yylloc->last_column = yylloc->first_column; \ > @@ -174,7 +245,7 @@ drv_cfg_term [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)? > * If you add a modifier you need to update check_modifier(). > * Also, the letters in modifier_event must not be in modifier_bp. > */ > -modifier_event [ukhpPGHSDIWeb]+ > +modifier_event [ukhpPGHSDIWeb]{1,15} > modifier_bp [rwx]{1,3} > lc_type (L1-dcache|l1-d|l1d|L1-data|L1-icache|l1-i|l1i|L1-instruction|LLC|L2|dTLB|d-tlb|Data-TLB|iTLB|i-tlb|Instruction-TLB|branch|branches|bpu|btb|bpc|node) > lc_op_result (load|loads|read|store|stores|write|prefetch|prefetches|speculative-read|speculative-load|refs|Reference|ops|access|misses|miss) > @@ -341,7 +412,7 @@ r{num_raw_hex} { return str(yyscanner, PE_RAW); } > {num_dec} { return value(_parse_state, yyscanner, 10); } > {num_hex} { return value(_parse_state, yyscanner, 16); } > > -{modifier_event} { return str(yyscanner, PE_MODIFIER_EVENT); } > +{modifier_event} { return modifiers(_parse_state, yyscanner); } > {name} { return str(yyscanner, PE_NAME); } > {name_tag} { return str(yyscanner, PE_NAME); } > "/" { BEGIN(config); return '/'; } > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > index 2c4817e126c1..79f254189be6 100644 > --- a/tools/perf/util/parse-events.y > +++ b/tools/perf/util/parse-events.y > @@ -68,11 +68,11 @@ static void free_list_evsel(struct list_head* list_evsel) > %type <num> PE_VALUE > %type <num> PE_VALUE_SYM_SW > %type <num> PE_VALUE_SYM_TOOL > +%type <mod> PE_MODIFIER_EVENT > %type <term_type> PE_TERM > %type <str> PE_RAW > %type <str> PE_NAME > %type <str> PE_LEGACY_CACHE > -%type <str> PE_MODIFIER_EVENT > %type <str> PE_MODIFIER_BP > %type <str> PE_EVENT_NAME > %type <str> PE_DRV_CFG_TERM > @@ -110,6 +110,7 @@ static void free_list_evsel(struct list_head* list_evsel) > { > char *str; > u64 num; > + struct parse_events_modifier mod; > enum parse_events__term_type term_type; > struct list_head *list_evsel; > struct parse_events_terms *list_terms; > @@ -175,20 +176,13 @@ event > group: > group_def ':' PE_MODIFIER_EVENT > { > + /* Apply the modifier to the events in the group_def. */ > struct list_head *list = $1; > int err; > > - err = parse_events__modifier_group(list, $3); > - free($3); > - if (err) { > - struct parse_events_state *parse_state = _parse_state; > - struct parse_events_error *error = parse_state->error; > - > - parse_events_error__handle(error, @3.first_column, > - strdup("Bad modifier"), NULL); > - free_list_evsel(list); > + err = parse_events__modifier_group(_parse_state, &@3, list, $3); > + if (err) > YYABORT; > - } > $$ = list; > } > | > @@ -238,17 +232,9 @@ event_name PE_MODIFIER_EVENT > * (there could be more events added for multiple tracepoint > * definitions via '*?'. > */ > - err = parse_events__modifier_event(list, $2, false); > - free($2); > - if (err) { > - struct parse_events_state *parse_state = _parse_state; > - struct parse_events_error *error = parse_state->error; > - > - parse_events_error__handle(error, @2.first_column, > - strdup("Bad modifier"), NULL); > - free_list_evsel(list); > + err = parse_events__modifier_event(_parse_state, &@2, list, $2); > + if (err) > YYABORT; > - } > $$ = list; > } > |
On Thu, Apr 18, 2024 at 1:32 PM Liang, Kan <kan.liang@linux.intel.com> wrote: > > > > On 2024-04-16 2:15 a.m., Ian Rogers wrote: > > Use a struct/bitmap rather than a copied string from lexer. > > > > In lexer give improved error message when too many precise flags are > > given or repeated modifiers. > > > > Before: > > ``` > > $ perf stat -e 'cycles:kuk' true > > event syntax error: 'cycles:kuk' > > \___ Bad modifier > > ... > > $ perf stat -e 'cycles:pppp' true > > event syntax error: 'cycles:pppp' > > \___ Bad modifier > > ... > > $ perf stat -e '{instructions:p,cycles:pp}:pp' -a true > > event syntax error: '..cycles:pp}:pp' > > \___ Bad modifier > > ... > > ``` > > After: > > ``` > > $ perf stat -e 'cycles:kuk' true > > event syntax error: 'cycles:kuk' > > \___ Duplicate modifier 'k' (kernel) > > ... > > $ perf stat -e 'cycles:pppp' true > > event syntax error: 'cycles:pppp' > > \___ Maximum precise value is 3 > > ... > > $ perf stat -e '{instructions:p,cycles:pp}:pp' true > > event syntax error: '..cycles:pp}:pp' > > \___ Maximum combined precise value is 3, adding precision to "cycles:pp" > > ... > > ``` > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/parse-events.c | 250 ++++++++++++--------------------- > > tools/perf/util/parse-events.h | 23 ++- > > tools/perf/util/parse-events.l | 75 +++++++++- > > tools/perf/util/parse-events.y | 28 +--- > > 4 files changed, 194 insertions(+), 182 deletions(-) > > > > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > > index ebada37ef98a..3ab533d0e653 100644 > > --- a/tools/perf/util/parse-events.c > > +++ b/tools/perf/util/parse-events.c > > @@ -1700,12 +1700,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state > > return -EINVAL; > > } > > > > -int parse_events__modifier_group(struct list_head *list, > > - char *event_mod) > > -{ > > - return parse_events__modifier_event(list, event_mod, true); > > -} > > - > > void parse_events__set_leader(char *name, struct list_head *list) > > { > > struct evsel *leader; > > @@ -1720,183 +1714,125 @@ void parse_events__set_leader(char *name, struct list_head *list) > > leader->group_name = name; > > } > > > > -struct event_modifier { > > - int eu; > > - int ek; > > - int eh; > > - int eH; > > - int eG; > > - int eI; > > - int precise; > > - int precise_max; > > - int exclude_GH; > > - int sample_read; > > - int pinned; > > - int weak; > > - int exclusive; > > - int bpf_counter; > > -}; > > +static int parse_events__modifier_list(struct parse_events_state *parse_state, > > + YYLTYPE *loc, > > + struct list_head *list, > > + struct parse_events_modifier mod, > > + bool group) > > +{ > > + struct evsel *evsel; > > > > -static int get_event_modifier(struct event_modifier *mod, char *str, > > - struct evsel *evsel) > > -{ > > - int eu = evsel ? evsel->core.attr.exclude_user : 0; > > - int ek = evsel ? evsel->core.attr.exclude_kernel : 0; > > - int eh = evsel ? evsel->core.attr.exclude_hv : 0; > > - int eH = evsel ? evsel->core.attr.exclude_host : 0; > > - int eG = evsel ? evsel->core.attr.exclude_guest : 0; > > - int eI = evsel ? evsel->core.attr.exclude_idle : 0; > > - int precise = evsel ? evsel->core.attr.precise_ip : 0; > > - int precise_max = 0; > > - int sample_read = 0; > > - int pinned = evsel ? evsel->core.attr.pinned : 0; > > - int exclusive = evsel ? evsel->core.attr.exclusive : 0; > > - > > - int exclude = eu | ek | eh; > > - int exclude_GH = evsel ? evsel->exclude_GH : 0; > > - int weak = 0; > > - int bpf_counter = 0; > > - > > - memset(mod, 0, sizeof(*mod)); > > - > > - while (*str) { > > - if (*str == 'u') { > > + if (!group && mod.weak) { > > + parse_events_error__handle(parse_state->error, loc->first_column, > > + strdup("Weak modifier is for use with groups"), NULL); > > + return -EINVAL; > > + } > > + > > + __evlist__for_each_entry(list, evsel) { > > + /* Translate modifiers into the equivalent evsel excludes. */ > > + int eu = group ? evsel->core.attr.exclude_user : 0; > > + int ek = group ? evsel->core.attr.exclude_kernel : 0; > > + int eh = group ? evsel->core.attr.exclude_hv : 0; > > + int eH = group ? evsel->core.attr.exclude_host : 0; > > + int eG = group ? evsel->core.attr.exclude_guest : 0; > > + int exclude = eu | ek | eh; > > + int exclude_GH = group ? evsel->exclude_GH : 0; > > + > > + if (mod.precise) { > > + /* use of precise requires exclude_guest */ > > + eG = 1; > > + } > > + if (mod.user) { > > if (!exclude) > > exclude = eu = ek = eh = 1; > > if (!exclude_GH && !perf_guest) > > eG = 1; > > eu = 0; > > - } else if (*str == 'k') { > > + } > > + if (mod.kernel) { > > if (!exclude) > > exclude = eu = ek = eh = 1; > > ek = 0; > > - } else if (*str == 'h') { > > + } > > + if (mod.hypervisor) { > > if (!exclude) > > exclude = eu = ek = eh = 1; > > eh = 0; > > - } else if (*str == 'G') { > > + } > > + if (mod.guest) { > > if (!exclude_GH) > > exclude_GH = eG = eH = 1; > > eG = 0; > > - } else if (*str == 'H') { > > + } > > + if (mod.host) { > > if (!exclude_GH) > > exclude_GH = eG = eH = 1; > > eH = 0; > > - } else if (*str == 'I') { > > - eI = 1; > > - } else if (*str == 'p') { > > - precise++; > > - /* use of precise requires exclude_guest */ > > - if (!exclude_GH) > > - eG = 1; > > - } else if (*str == 'P') { > > - precise_max = 1; > > - } else if (*str == 'S') { > > - sample_read = 1; > > - } else if (*str == 'D') { > > - pinned = 1; > > - } else if (*str == 'e') { > > - exclusive = 1; > > - } else if (*str == 'W') { > > - weak = 1; > > - } else if (*str == 'b') { > > - bpf_counter = 1; > > - } else > > - break; > > - > > - ++str; > > + } > > + evsel->core.attr.exclude_user = eu; > > + evsel->core.attr.exclude_kernel = ek; > > + evsel->core.attr.exclude_hv = eh; > > + evsel->core.attr.exclude_host = eH; > > + evsel->core.attr.exclude_guest = eG; > > + evsel->exclude_GH = exclude_GH; > > + > > + /* Simple modifiers copied to the evsel. */ > > + if (mod.precise) { > > + u8 precise = evsel->core.attr.precise_ip + mod.precise; > > + /* > > + * precise ip: > > + * > > + * 0 - SAMPLE_IP can have arbitrary skid > > + * 1 - SAMPLE_IP must have constant skid > > + * 2 - SAMPLE_IP requested to have 0 skid > > + * 3 - SAMPLE_IP must have 0 skid > > + * > > + * See also PERF_RECORD_MISC_EXACT_IP > > + */ > > + if (precise > 3) { > > The pmu_max_precise() should return the max precise the current kernel > supports. It checks the /sys/devices/cpu/caps/max_precise. > > I think we should use that value rather than hard code it to 3. I'll add an extra patch to do that. I'm a bit concerned it may break event parsing on platforms not supporting max_precise of 3. Thanks, Ian > Thanks, > Kan > > > + char *help; > > + > > + if (asprintf(&help, > > + "Maximum combined precise value is 3, adding precision to \"%s\"", > > + evsel__name(evsel)) > 0) { > > + parse_events_error__handle(parse_state->error, > > + loc->first_column, > > + help, NULL); > > + } > > + return -EINVAL; > > + } > > + evsel->core.attr.precise_ip = precise; > > + } > > + if (mod.precise_max) > > + evsel->precise_max = 1; > > + if (mod.non_idle) > > + evsel->core.attr.exclude_idle = 1; > > + if (mod.sample_read) > > + evsel->sample_read = 1; > > + if (mod.pinned && evsel__is_group_leader(evsel)) > > + evsel->core.attr.pinned = 1; > > + if (mod.exclusive && evsel__is_group_leader(evsel)) > > + evsel->core.attr.exclusive = 1; > > + if (mod.weak) > > + evsel->weak_group = true; > > + if (mod.bpf) > > + evsel->bpf_counter = true; > > } > > - > > - /* > > - * precise ip: > > - * > > - * 0 - SAMPLE_IP can have arbitrary skid > > - * 1 - SAMPLE_IP must have constant skid > > - * 2 - SAMPLE_IP requested to have 0 skid > > - * 3 - SAMPLE_IP must have 0 skid > > - * > > - * See also PERF_RECORD_MISC_EXACT_IP > > - */ > > - if (precise > 3) > > - return -EINVAL; > > - > > - mod->eu = eu; > > - mod->ek = ek; > > - mod->eh = eh; > > - mod->eH = eH; > > - mod->eG = eG; > > - mod->eI = eI; > > - mod->precise = precise; > > - mod->precise_max = precise_max; > > - mod->exclude_GH = exclude_GH; > > - mod->sample_read = sample_read; > > - mod->pinned = pinned; > > - mod->weak = weak; > > - mod->bpf_counter = bpf_counter; > > - mod->exclusive = exclusive; > > - > > return 0; > > } > > > > -/* > > - * Basic modifier sanity check to validate it contains only one > > - * instance of any modifier (apart from 'p') present. > > - */ > > -static int check_modifier(char *str) > > +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc, > > + struct list_head *list, > > + struct parse_events_modifier mod) > > { > > - char *p = str; > > - > > - /* The sizeof includes 0 byte as well. */ > > - if (strlen(str) > (sizeof("ukhGHpppPSDIWeb") - 1)) > > - return -1; > > - > > - while (*p) { > > - if (*p != 'p' && strchr(p + 1, *p)) > > - return -1; > > - p++; > > - } > > - > > - return 0; > > + return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/true); > > } > > > > -int parse_events__modifier_event(struct list_head *list, char *str, bool add) > > +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc, > > + struct list_head *list, > > + struct parse_events_modifier mod) > > { > > - struct evsel *evsel; > > - struct event_modifier mod; > > - > > - if (str == NULL) > > - return 0; > > - > > - if (check_modifier(str)) > > - return -EINVAL; > > - > > - if (!add && get_event_modifier(&mod, str, NULL)) > > - return -EINVAL; > > - > > - __evlist__for_each_entry(list, evsel) { > > - if (add && get_event_modifier(&mod, str, evsel)) > > - return -EINVAL; > > - > > - evsel->core.attr.exclude_user = mod.eu; > > - evsel->core.attr.exclude_kernel = mod.ek; > > - evsel->core.attr.exclude_hv = mod.eh; > > - evsel->core.attr.precise_ip = mod.precise; > > - evsel->core.attr.exclude_host = mod.eH; > > - evsel->core.attr.exclude_guest = mod.eG; > > - evsel->core.attr.exclude_idle = mod.eI; > > - evsel->exclude_GH = mod.exclude_GH; > > - evsel->sample_read = mod.sample_read; > > - evsel->precise_max = mod.precise_max; > > - evsel->weak_group = mod.weak; > > - evsel->bpf_counter = mod.bpf_counter; > > - > > - if (evsel__is_group_leader(evsel)) { > > - evsel->core.attr.pinned = mod.pinned; > > - evsel->core.attr.exclusive = mod.exclusive; > > - } > > - } > > - > > - return 0; > > + return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/false); > > } > > > > int parse_events_name(struct list_head *list, const char *name) > > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h > > index 290ae6c72ec5..f104faef1a78 100644 > > --- a/tools/perf/util/parse-events.h > > +++ b/tools/perf/util/parse-events.h > > @@ -186,8 +186,27 @@ void parse_events_terms__init(struct parse_events_terms *terms); > > void parse_events_terms__exit(struct parse_events_terms *terms); > > int parse_events_terms(struct parse_events_terms *terms, const char *str, FILE *input); > > int parse_events_terms__to_strbuf(const struct parse_events_terms *terms, struct strbuf *sb); > > -int parse_events__modifier_event(struct list_head *list, char *str, bool add); > > -int parse_events__modifier_group(struct list_head *list, char *event_mod); > > + > > +struct parse_events_modifier { > > + u8 precise; /* Number of repeated 'p' for precision. */ > > + bool precise_max : 1; /* 'P' */ > > + bool non_idle : 1; /* 'I' */ > > + bool sample_read : 1; /* 'S' */ > > + bool pinned : 1; /* 'D' */ > > + bool exclusive : 1; /* 'e' */ > > + bool weak : 1; /* 'W' */ > > + bool bpf : 1; /* 'b' */ > > + bool user : 1; /* 'u' */ > > + bool kernel : 1; /* 'k' */ > > + bool hypervisor : 1; /* 'h' */ > > + bool guest : 1; /* 'G' */ > > + bool host : 1; /* 'H' */ > > +}; > > + > > +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc, > > + struct list_head *list, struct parse_events_modifier mod); > > +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc, > > + struct list_head *list, struct parse_events_modifier mod); > > int parse_events_name(struct list_head *list, const char *name); > > int parse_events_add_tracepoint(struct list_head *list, int *idx, > > const char *sys, const char *event, > > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l > > index 0cd68c9f0d4f..4aaf0c53d9b6 100644 > > --- a/tools/perf/util/parse-events.l > > +++ b/tools/perf/util/parse-events.l > > @@ -142,6 +142,77 @@ static int hw(yyscan_t scanner, int config) > > return PE_TERM_HW; > > } > > > > +static void modifiers_error(struct parse_events_state *parse_state, yyscan_t scanner, > > + int pos, char mod_char, const char *mod_name) > > +{ > > + struct parse_events_error *error = parse_state->error; > > + char *help = NULL; > > + > > + if (asprintf(&help, "Duplicate modifier '%c' (%s)", mod_char, mod_name) > 0) > > + parse_events_error__handle(error, get_column(scanner) + pos, help , NULL); > > +} > > + > > +static int modifiers(struct parse_events_state *parse_state, yyscan_t scanner) > > +{ > > + YYSTYPE *yylval = parse_events_get_lval(scanner); > > + char *text = parse_events_get_text(scanner); > > + struct parse_events_modifier mod = { .precise = 0, }; > > + > > + for (size_t i = 0, n = strlen(text); i < n; i++) { > > +#define CASE(c, field) \ > > + case c: \ > > + if (mod.field) { \ > > + modifiers_error(parse_state, scanner, i, c, #field); \ > > + return PE_ERROR; \ > > + } \ > > + mod.field = true; \ > > + break > > + > > + switch (text[i]) { > > + CASE('u', user); > > + CASE('k', kernel); > > + CASE('h', hypervisor); > > + CASE('I', non_idle); > > + CASE('G', guest); > > + CASE('H', host); > > + case 'p': > > + mod.precise++; > > + /* > > + * precise ip: > > + * > > + * 0 - SAMPLE_IP can have arbitrary skid > > + * 1 - SAMPLE_IP must have constant skid > > + * 2 - SAMPLE_IP requested to have 0 skid > > + * 3 - SAMPLE_IP must have 0 skid > > + * > > + * See also PERF_RECORD_MISC_EXACT_IP > > + */ > > + if (mod.precise > 3) { > > + struct parse_events_error *error = parse_state->error; > > + char *help = strdup("Maximum precise value is 3"); > > + > > + if (help) { > > + parse_events_error__handle(error, get_column(scanner) + i, > > + help , NULL); > > + } > > + return PE_ERROR; > > + } > > + break; > > + CASE('P', precise_max); > > + CASE('S', sample_read); > > + CASE('D', pinned); > > + CASE('W', weak); > > + CASE('e', exclusive); > > + CASE('b', bpf); > > + default: > > + return PE_ERROR; > > + } > > +#undef CASE > > + } > > + yylval->mod = mod; > > + return PE_MODIFIER_EVENT; > > +} > > + > > #define YY_USER_ACTION \ > > do { \ > > yylloc->last_column = yylloc->first_column; \ > > @@ -174,7 +245,7 @@ drv_cfg_term [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)? > > * If you add a modifier you need to update check_modifier(). > > * Also, the letters in modifier_event must not be in modifier_bp. > > */ > > -modifier_event [ukhpPGHSDIWeb]+ > > +modifier_event [ukhpPGHSDIWeb]{1,15} > > modifier_bp [rwx]{1,3} > > lc_type (L1-dcache|l1-d|l1d|L1-data|L1-icache|l1-i|l1i|L1-instruction|LLC|L2|dTLB|d-tlb|Data-TLB|iTLB|i-tlb|Instruction-TLB|branch|branches|bpu|btb|bpc|node) > > lc_op_result (load|loads|read|store|stores|write|prefetch|prefetches|speculative-read|speculative-load|refs|Reference|ops|access|misses|miss) > > @@ -341,7 +412,7 @@ r{num_raw_hex} { return str(yyscanner, PE_RAW); } > > {num_dec} { return value(_parse_state, yyscanner, 10); } > > {num_hex} { return value(_parse_state, yyscanner, 16); } > > > > -{modifier_event} { return str(yyscanner, PE_MODIFIER_EVENT); } > > +{modifier_event} { return modifiers(_parse_state, yyscanner); } > > {name} { return str(yyscanner, PE_NAME); } > > {name_tag} { return str(yyscanner, PE_NAME); } > > "/" { BEGIN(config); return '/'; } > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > > index 2c4817e126c1..79f254189be6 100644 > > --- a/tools/perf/util/parse-events.y > > +++ b/tools/perf/util/parse-events.y > > @@ -68,11 +68,11 @@ static void free_list_evsel(struct list_head* list_evsel) > > %type <num> PE_VALUE > > %type <num> PE_VALUE_SYM_SW > > %type <num> PE_VALUE_SYM_TOOL > > +%type <mod> PE_MODIFIER_EVENT > > %type <term_type> PE_TERM > > %type <str> PE_RAW > > %type <str> PE_NAME > > %type <str> PE_LEGACY_CACHE > > -%type <str> PE_MODIFIER_EVENT > > %type <str> PE_MODIFIER_BP > > %type <str> PE_EVENT_NAME > > %type <str> PE_DRV_CFG_TERM > > @@ -110,6 +110,7 @@ static void free_list_evsel(struct list_head* list_evsel) > > { > > char *str; > > u64 num; > > + struct parse_events_modifier mod; > > enum parse_events__term_type term_type; > > struct list_head *list_evsel; > > struct parse_events_terms *list_terms; > > @@ -175,20 +176,13 @@ event > > group: > > group_def ':' PE_MODIFIER_EVENT > > { > > + /* Apply the modifier to the events in the group_def. */ > > struct list_head *list = $1; > > int err; > > > > - err = parse_events__modifier_group(list, $3); > > - free($3); > > - if (err) { > > - struct parse_events_state *parse_state = _parse_state; > > - struct parse_events_error *error = parse_state->error; > > - > > - parse_events_error__handle(error, @3.first_column, > > - strdup("Bad modifier"), NULL); > > - free_list_evsel(list); > > + err = parse_events__modifier_group(_parse_state, &@3, list, $3); > > + if (err) > > YYABORT; > > - } > > $$ = list; > > } > > | > > @@ -238,17 +232,9 @@ event_name PE_MODIFIER_EVENT > > * (there could be more events added for multiple tracepoint > > * definitions via '*?'. > > */ > > - err = parse_events__modifier_event(list, $2, false); > > - free($2); > > - if (err) { > > - struct parse_events_state *parse_state = _parse_state; > > - struct parse_events_error *error = parse_state->error; > > - > > - parse_events_error__handle(error, @2.first_column, > > - strdup("Bad modifier"), NULL); > > - free_list_evsel(list); > > + err = parse_events__modifier_event(_parse_state, &@2, list, $2); > > + if (err) > > YYABORT; > > - } > > $$ = list; > > } > > |
On 2024-04-19 2:22 a.m., Ian Rogers wrote: >>> + /* Simple modifiers copied to the evsel. */ >>> + if (mod.precise) { >>> + u8 precise = evsel->core.attr.precise_ip + mod.precise; >>> + /* >>> + * precise ip: >>> + * >>> + * 0 - SAMPLE_IP can have arbitrary skid >>> + * 1 - SAMPLE_IP must have constant skid >>> + * 2 - SAMPLE_IP requested to have 0 skid >>> + * 3 - SAMPLE_IP must have 0 skid >>> + * >>> + * See also PERF_RECORD_MISC_EXACT_IP >>> + */ >>> + if (precise > 3) { >> The pmu_max_precise() should return the max precise the current kernel >> supports. It checks the /sys/devices/cpu/caps/max_precise. >> >> I think we should use that value rather than hard code it to 3. > I'll add an extra patch to do that. I'm a bit concerned it may break > event parsing on platforms not supporting max_precise of 3. The kernel already rejects the precise_ip > max_precise (using the same x86_pmu_max_precise()). It should be fine to apply the same logic in the tool. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/core.c#n566 Will the extra patch be sent separately? Thanks, Kan
On Fri, Apr 19, 2024 at 6:20 AM Liang, Kan <kan.liang@linux.intel.com> wrote: > > > > On 2024-04-19 2:22 a.m., Ian Rogers wrote: > >>> + /* Simple modifiers copied to the evsel. */ > >>> + if (mod.precise) { > >>> + u8 precise = evsel->core.attr.precise_ip + mod.precise; > >>> + /* > >>> + * precise ip: > >>> + * > >>> + * 0 - SAMPLE_IP can have arbitrary skid > >>> + * 1 - SAMPLE_IP must have constant skid > >>> + * 2 - SAMPLE_IP requested to have 0 skid > >>> + * 3 - SAMPLE_IP must have 0 skid > >>> + * > >>> + * See also PERF_RECORD_MISC_EXACT_IP > >>> + */ > >>> + if (precise > 3) { > >> The pmu_max_precise() should return the max precise the current kernel > >> supports. It checks the /sys/devices/cpu/caps/max_precise. > >> > >> I think we should use that value rather than hard code it to 3. > > I'll add an extra patch to do that. I'm a bit concerned it may break > > event parsing on platforms not supporting max_precise of 3. > > The kernel already rejects the precise_ip > max_precise (using the same > x86_pmu_max_precise()). It should be fine to apply the same logic in the > tool. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/core.c#n566 > > Will the extra patch be sent separately? Let's do it separately. I'm concerned about the behavior on AMD (and possibly similar architectures) where certain events support precision like cycles, as they detour to the IBS PMU, but not all events support it. The max_precise should reflect that AMD's Zen core PMU does support precision as a consequence of detouring to IBS, but maybe things in sysfs aren't set up correctly. Thanks, Ian > Thanks, > Kan
On 2024-04-24 11:18 a.m., Ian Rogers wrote: > On Fri, Apr 19, 2024 at 6:20 AM Liang, Kan <kan.liang@linux.intel.com> wrote: >> >> >> >> On 2024-04-19 2:22 a.m., Ian Rogers wrote: >>>>> + /* Simple modifiers copied to the evsel. */ >>>>> + if (mod.precise) { >>>>> + u8 precise = evsel->core.attr.precise_ip + mod.precise; >>>>> + /* >>>>> + * precise ip: >>>>> + * >>>>> + * 0 - SAMPLE_IP can have arbitrary skid >>>>> + * 1 - SAMPLE_IP must have constant skid >>>>> + * 2 - SAMPLE_IP requested to have 0 skid >>>>> + * 3 - SAMPLE_IP must have 0 skid >>>>> + * >>>>> + * See also PERF_RECORD_MISC_EXACT_IP >>>>> + */ >>>>> + if (precise > 3) { >>>> The pmu_max_precise() should return the max precise the current kernel >>>> supports. It checks the /sys/devices/cpu/caps/max_precise. >>>> >>>> I think we should use that value rather than hard code it to 3. >>> I'll add an extra patch to do that. I'm a bit concerned it may break >>> event parsing on platforms not supporting max_precise of 3. >> >> The kernel already rejects the precise_ip > max_precise (using the same >> x86_pmu_max_precise()). It should be fine to apply the same logic in the >> tool. >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/events/core.c#n566 >> >> Will the extra patch be sent separately? > > Let's do it separately. I'm concerned about the behavior on AMD (and > possibly similar architectures) where certain events support precision > like cycles, as they detour to the IBS PMU, but not all events support > it. The max_precise should reflect that AMD's Zen core PMU does > support precision as a consequence of detouring to IBS, but maybe > things in sysfs aren't set up correctly. > The x86_pmu_max_precise() is a generic function for X86. It should apply to AMD as well. A separate patch looks good to me. Thanks, Kan
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index ebada37ef98a..3ab533d0e653 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -1700,12 +1700,6 @@ int parse_events_multi_pmu_add_or_add_pmu(struct parse_events_state *parse_state return -EINVAL; } -int parse_events__modifier_group(struct list_head *list, - char *event_mod) -{ - return parse_events__modifier_event(list, event_mod, true); -} - void parse_events__set_leader(char *name, struct list_head *list) { struct evsel *leader; @@ -1720,183 +1714,125 @@ void parse_events__set_leader(char *name, struct list_head *list) leader->group_name = name; } -struct event_modifier { - int eu; - int ek; - int eh; - int eH; - int eG; - int eI; - int precise; - int precise_max; - int exclude_GH; - int sample_read; - int pinned; - int weak; - int exclusive; - int bpf_counter; -}; +static int parse_events__modifier_list(struct parse_events_state *parse_state, + YYLTYPE *loc, + struct list_head *list, + struct parse_events_modifier mod, + bool group) +{ + struct evsel *evsel; -static int get_event_modifier(struct event_modifier *mod, char *str, - struct evsel *evsel) -{ - int eu = evsel ? evsel->core.attr.exclude_user : 0; - int ek = evsel ? evsel->core.attr.exclude_kernel : 0; - int eh = evsel ? evsel->core.attr.exclude_hv : 0; - int eH = evsel ? evsel->core.attr.exclude_host : 0; - int eG = evsel ? evsel->core.attr.exclude_guest : 0; - int eI = evsel ? evsel->core.attr.exclude_idle : 0; - int precise = evsel ? evsel->core.attr.precise_ip : 0; - int precise_max = 0; - int sample_read = 0; - int pinned = evsel ? evsel->core.attr.pinned : 0; - int exclusive = evsel ? evsel->core.attr.exclusive : 0; - - int exclude = eu | ek | eh; - int exclude_GH = evsel ? evsel->exclude_GH : 0; - int weak = 0; - int bpf_counter = 0; - - memset(mod, 0, sizeof(*mod)); - - while (*str) { - if (*str == 'u') { + if (!group && mod.weak) { + parse_events_error__handle(parse_state->error, loc->first_column, + strdup("Weak modifier is for use with groups"), NULL); + return -EINVAL; + } + + __evlist__for_each_entry(list, evsel) { + /* Translate modifiers into the equivalent evsel excludes. */ + int eu = group ? evsel->core.attr.exclude_user : 0; + int ek = group ? evsel->core.attr.exclude_kernel : 0; + int eh = group ? evsel->core.attr.exclude_hv : 0; + int eH = group ? evsel->core.attr.exclude_host : 0; + int eG = group ? evsel->core.attr.exclude_guest : 0; + int exclude = eu | ek | eh; + int exclude_GH = group ? evsel->exclude_GH : 0; + + if (mod.precise) { + /* use of precise requires exclude_guest */ + eG = 1; + } + if (mod.user) { if (!exclude) exclude = eu = ek = eh = 1; if (!exclude_GH && !perf_guest) eG = 1; eu = 0; - } else if (*str == 'k') { + } + if (mod.kernel) { if (!exclude) exclude = eu = ek = eh = 1; ek = 0; - } else if (*str == 'h') { + } + if (mod.hypervisor) { if (!exclude) exclude = eu = ek = eh = 1; eh = 0; - } else if (*str == 'G') { + } + if (mod.guest) { if (!exclude_GH) exclude_GH = eG = eH = 1; eG = 0; - } else if (*str == 'H') { + } + if (mod.host) { if (!exclude_GH) exclude_GH = eG = eH = 1; eH = 0; - } else if (*str == 'I') { - eI = 1; - } else if (*str == 'p') { - precise++; - /* use of precise requires exclude_guest */ - if (!exclude_GH) - eG = 1; - } else if (*str == 'P') { - precise_max = 1; - } else if (*str == 'S') { - sample_read = 1; - } else if (*str == 'D') { - pinned = 1; - } else if (*str == 'e') { - exclusive = 1; - } else if (*str == 'W') { - weak = 1; - } else if (*str == 'b') { - bpf_counter = 1; - } else - break; - - ++str; + } + evsel->core.attr.exclude_user = eu; + evsel->core.attr.exclude_kernel = ek; + evsel->core.attr.exclude_hv = eh; + evsel->core.attr.exclude_host = eH; + evsel->core.attr.exclude_guest = eG; + evsel->exclude_GH = exclude_GH; + + /* Simple modifiers copied to the evsel. */ + if (mod.precise) { + u8 precise = evsel->core.attr.precise_ip + mod.precise; + /* + * precise ip: + * + * 0 - SAMPLE_IP can have arbitrary skid + * 1 - SAMPLE_IP must have constant skid + * 2 - SAMPLE_IP requested to have 0 skid + * 3 - SAMPLE_IP must have 0 skid + * + * See also PERF_RECORD_MISC_EXACT_IP + */ + if (precise > 3) { + char *help; + + if (asprintf(&help, + "Maximum combined precise value is 3, adding precision to \"%s\"", + evsel__name(evsel)) > 0) { + parse_events_error__handle(parse_state->error, + loc->first_column, + help, NULL); + } + return -EINVAL; + } + evsel->core.attr.precise_ip = precise; + } + if (mod.precise_max) + evsel->precise_max = 1; + if (mod.non_idle) + evsel->core.attr.exclude_idle = 1; + if (mod.sample_read) + evsel->sample_read = 1; + if (mod.pinned && evsel__is_group_leader(evsel)) + evsel->core.attr.pinned = 1; + if (mod.exclusive && evsel__is_group_leader(evsel)) + evsel->core.attr.exclusive = 1; + if (mod.weak) + evsel->weak_group = true; + if (mod.bpf) + evsel->bpf_counter = true; } - - /* - * precise ip: - * - * 0 - SAMPLE_IP can have arbitrary skid - * 1 - SAMPLE_IP must have constant skid - * 2 - SAMPLE_IP requested to have 0 skid - * 3 - SAMPLE_IP must have 0 skid - * - * See also PERF_RECORD_MISC_EXACT_IP - */ - if (precise > 3) - return -EINVAL; - - mod->eu = eu; - mod->ek = ek; - mod->eh = eh; - mod->eH = eH; - mod->eG = eG; - mod->eI = eI; - mod->precise = precise; - mod->precise_max = precise_max; - mod->exclude_GH = exclude_GH; - mod->sample_read = sample_read; - mod->pinned = pinned; - mod->weak = weak; - mod->bpf_counter = bpf_counter; - mod->exclusive = exclusive; - return 0; } -/* - * Basic modifier sanity check to validate it contains only one - * instance of any modifier (apart from 'p') present. - */ -static int check_modifier(char *str) +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc, + struct list_head *list, + struct parse_events_modifier mod) { - char *p = str; - - /* The sizeof includes 0 byte as well. */ - if (strlen(str) > (sizeof("ukhGHpppPSDIWeb") - 1)) - return -1; - - while (*p) { - if (*p != 'p' && strchr(p + 1, *p)) - return -1; - p++; - } - - return 0; + return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/true); } -int parse_events__modifier_event(struct list_head *list, char *str, bool add) +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc, + struct list_head *list, + struct parse_events_modifier mod) { - struct evsel *evsel; - struct event_modifier mod; - - if (str == NULL) - return 0; - - if (check_modifier(str)) - return -EINVAL; - - if (!add && get_event_modifier(&mod, str, NULL)) - return -EINVAL; - - __evlist__for_each_entry(list, evsel) { - if (add && get_event_modifier(&mod, str, evsel)) - return -EINVAL; - - evsel->core.attr.exclude_user = mod.eu; - evsel->core.attr.exclude_kernel = mod.ek; - evsel->core.attr.exclude_hv = mod.eh; - evsel->core.attr.precise_ip = mod.precise; - evsel->core.attr.exclude_host = mod.eH; - evsel->core.attr.exclude_guest = mod.eG; - evsel->core.attr.exclude_idle = mod.eI; - evsel->exclude_GH = mod.exclude_GH; - evsel->sample_read = mod.sample_read; - evsel->precise_max = mod.precise_max; - evsel->weak_group = mod.weak; - evsel->bpf_counter = mod.bpf_counter; - - if (evsel__is_group_leader(evsel)) { - evsel->core.attr.pinned = mod.pinned; - evsel->core.attr.exclusive = mod.exclusive; - } - } - - return 0; + return parse_events__modifier_list(parse_state, loc, list, mod, /*group=*/false); } int parse_events_name(struct list_head *list, const char *name) diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h index 290ae6c72ec5..f104faef1a78 100644 --- a/tools/perf/util/parse-events.h +++ b/tools/perf/util/parse-events.h @@ -186,8 +186,27 @@ void parse_events_terms__init(struct parse_events_terms *terms); void parse_events_terms__exit(struct parse_events_terms *terms); int parse_events_terms(struct parse_events_terms *terms, const char *str, FILE *input); int parse_events_terms__to_strbuf(const struct parse_events_terms *terms, struct strbuf *sb); -int parse_events__modifier_event(struct list_head *list, char *str, bool add); -int parse_events__modifier_group(struct list_head *list, char *event_mod); + +struct parse_events_modifier { + u8 precise; /* Number of repeated 'p' for precision. */ + bool precise_max : 1; /* 'P' */ + bool non_idle : 1; /* 'I' */ + bool sample_read : 1; /* 'S' */ + bool pinned : 1; /* 'D' */ + bool exclusive : 1; /* 'e' */ + bool weak : 1; /* 'W' */ + bool bpf : 1; /* 'b' */ + bool user : 1; /* 'u' */ + bool kernel : 1; /* 'k' */ + bool hypervisor : 1; /* 'h' */ + bool guest : 1; /* 'G' */ + bool host : 1; /* 'H' */ +}; + +int parse_events__modifier_event(struct parse_events_state *parse_state, void *loc, + struct list_head *list, struct parse_events_modifier mod); +int parse_events__modifier_group(struct parse_events_state *parse_state, void *loc, + struct list_head *list, struct parse_events_modifier mod); int parse_events_name(struct list_head *list, const char *name); int parse_events_add_tracepoint(struct list_head *list, int *idx, const char *sys, const char *event, diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l index 0cd68c9f0d4f..4aaf0c53d9b6 100644 --- a/tools/perf/util/parse-events.l +++ b/tools/perf/util/parse-events.l @@ -142,6 +142,77 @@ static int hw(yyscan_t scanner, int config) return PE_TERM_HW; } +static void modifiers_error(struct parse_events_state *parse_state, yyscan_t scanner, + int pos, char mod_char, const char *mod_name) +{ + struct parse_events_error *error = parse_state->error; + char *help = NULL; + + if (asprintf(&help, "Duplicate modifier '%c' (%s)", mod_char, mod_name) > 0) + parse_events_error__handle(error, get_column(scanner) + pos, help , NULL); +} + +static int modifiers(struct parse_events_state *parse_state, yyscan_t scanner) +{ + YYSTYPE *yylval = parse_events_get_lval(scanner); + char *text = parse_events_get_text(scanner); + struct parse_events_modifier mod = { .precise = 0, }; + + for (size_t i = 0, n = strlen(text); i < n; i++) { +#define CASE(c, field) \ + case c: \ + if (mod.field) { \ + modifiers_error(parse_state, scanner, i, c, #field); \ + return PE_ERROR; \ + } \ + mod.field = true; \ + break + + switch (text[i]) { + CASE('u', user); + CASE('k', kernel); + CASE('h', hypervisor); + CASE('I', non_idle); + CASE('G', guest); + CASE('H', host); + case 'p': + mod.precise++; + /* + * precise ip: + * + * 0 - SAMPLE_IP can have arbitrary skid + * 1 - SAMPLE_IP must have constant skid + * 2 - SAMPLE_IP requested to have 0 skid + * 3 - SAMPLE_IP must have 0 skid + * + * See also PERF_RECORD_MISC_EXACT_IP + */ + if (mod.precise > 3) { + struct parse_events_error *error = parse_state->error; + char *help = strdup("Maximum precise value is 3"); + + if (help) { + parse_events_error__handle(error, get_column(scanner) + i, + help , NULL); + } + return PE_ERROR; + } + break; + CASE('P', precise_max); + CASE('S', sample_read); + CASE('D', pinned); + CASE('W', weak); + CASE('e', exclusive); + CASE('b', bpf); + default: + return PE_ERROR; + } +#undef CASE + } + yylval->mod = mod; + return PE_MODIFIER_EVENT; +} + #define YY_USER_ACTION \ do { \ yylloc->last_column = yylloc->first_column; \ @@ -174,7 +245,7 @@ drv_cfg_term [a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)? * If you add a modifier you need to update check_modifier(). * Also, the letters in modifier_event must not be in modifier_bp. */ -modifier_event [ukhpPGHSDIWeb]+ +modifier_event [ukhpPGHSDIWeb]{1,15} modifier_bp [rwx]{1,3} lc_type (L1-dcache|l1-d|l1d|L1-data|L1-icache|l1-i|l1i|L1-instruction|LLC|L2|dTLB|d-tlb|Data-TLB|iTLB|i-tlb|Instruction-TLB|branch|branches|bpu|btb|bpc|node) lc_op_result (load|loads|read|store|stores|write|prefetch|prefetches|speculative-read|speculative-load|refs|Reference|ops|access|misses|miss) @@ -341,7 +412,7 @@ r{num_raw_hex} { return str(yyscanner, PE_RAW); } {num_dec} { return value(_parse_state, yyscanner, 10); } {num_hex} { return value(_parse_state, yyscanner, 16); } -{modifier_event} { return str(yyscanner, PE_MODIFIER_EVENT); } +{modifier_event} { return modifiers(_parse_state, yyscanner); } {name} { return str(yyscanner, PE_NAME); } {name_tag} { return str(yyscanner, PE_NAME); } "/" { BEGIN(config); return '/'; } diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y index 2c4817e126c1..79f254189be6 100644 --- a/tools/perf/util/parse-events.y +++ b/tools/perf/util/parse-events.y @@ -68,11 +68,11 @@ static void free_list_evsel(struct list_head* list_evsel) %type <num> PE_VALUE %type <num> PE_VALUE_SYM_SW %type <num> PE_VALUE_SYM_TOOL +%type <mod> PE_MODIFIER_EVENT %type <term_type> PE_TERM %type <str> PE_RAW %type <str> PE_NAME %type <str> PE_LEGACY_CACHE -%type <str> PE_MODIFIER_EVENT %type <str> PE_MODIFIER_BP %type <str> PE_EVENT_NAME %type <str> PE_DRV_CFG_TERM @@ -110,6 +110,7 @@ static void free_list_evsel(struct list_head* list_evsel) { char *str; u64 num; + struct parse_events_modifier mod; enum parse_events__term_type term_type; struct list_head *list_evsel; struct parse_events_terms *list_terms; @@ -175,20 +176,13 @@ event group: group_def ':' PE_MODIFIER_EVENT { + /* Apply the modifier to the events in the group_def. */ struct list_head *list = $1; int err; - err = parse_events__modifier_group(list, $3); - free($3); - if (err) { - struct parse_events_state *parse_state = _parse_state; - struct parse_events_error *error = parse_state->error; - - parse_events_error__handle(error, @3.first_column, - strdup("Bad modifier"), NULL); - free_list_evsel(list); + err = parse_events__modifier_group(_parse_state, &@3, list, $3); + if (err) YYABORT; - } $$ = list; } | @@ -238,17 +232,9 @@ event_name PE_MODIFIER_EVENT * (there could be more events added for multiple tracepoint * definitions via '*?'. */ - err = parse_events__modifier_event(list, $2, false); - free($2); - if (err) { - struct parse_events_state *parse_state = _parse_state; - struct parse_events_error *error = parse_state->error; - - parse_events_error__handle(error, @2.first_column, - strdup("Bad modifier"), NULL); - free_list_evsel(list); + err = parse_events__modifier_event(_parse_state, &@2, list, $2); + if (err) YYABORT; - } $$ = list; } |
Use a struct/bitmap rather than a copied string from lexer. In lexer give improved error message when too many precise flags are given or repeated modifiers. Before: ``` $ perf stat -e 'cycles:kuk' true event syntax error: 'cycles:kuk' \___ Bad modifier ... $ perf stat -e 'cycles:pppp' true event syntax error: 'cycles:pppp' \___ Bad modifier ... $ perf stat -e '{instructions:p,cycles:pp}:pp' -a true event syntax error: '..cycles:pp}:pp' \___ Bad modifier ... ``` After: ``` $ perf stat -e 'cycles:kuk' true event syntax error: 'cycles:kuk' \___ Duplicate modifier 'k' (kernel) ... $ perf stat -e 'cycles:pppp' true event syntax error: 'cycles:pppp' \___ Maximum precise value is 3 ... $ perf stat -e '{instructions:p,cycles:pp}:pp' true event syntax error: '..cycles:pp}:pp' \___ Maximum combined precise value is 3, adding precision to "cycles:pp" ... ``` Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/parse-events.c | 250 ++++++++++++--------------------- tools/perf/util/parse-events.h | 23 ++- tools/perf/util/parse-events.l | 75 +++++++++- tools/perf/util/parse-events.y | 28 +--- 4 files changed, 194 insertions(+), 182 deletions(-)