perf parse-events: Improvements to modifier parsing
authorIan Rogers <irogers@google.com>
Tue, 16 Apr 2024 06:15:29 +0000 (23:15 -0700)
committerArnaldo Carvalho de Melo <acme@redhat.com>
Sat, 27 Apr 2024 01:07:20 +0000 (22:07 -0300)
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>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Tested-by: Atish Patra <atishp@rivosinc.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Beeman Strong <beeman@rivosinc.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: James Clark <james.clark@arm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20240416061533.921723-14-irogers@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
tools/perf/util/parse-events.c
tools/perf/util/parse-events.h
tools/perf/util/parse-events.l
tools/perf/util/parse-events.y

index ebada37ef98ad21618de5b6a81181b029868a56a..3ab533d0e653a829871382f6dd122dc5e3d2d13e 100644 (file)
@@ -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)
index 290ae6c72ec5a1106e48e3a4c78d54c6948b17c9..f104faef1a78710ff61da1b8afe1b69ee300b208 100644 (file)
@@ -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,
index 0cd68c9f0d4fd95ac01283a729be6f54b8d6f272..4aaf0c53d9b6dceff689bf12b5747fca42f849ed 100644 (file)
@@ -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 '/'; }
index 2c4817e126c1b44841449a3a3b3f0d894b0c085b..79f254189be6e66a10cc02f03ed7a5ca63fcdf87 100644 (file)
@@ -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;
 }
 |