[PATCH v2 13/16] perf parse-events: Improvements to modifier parsing

Ian Rogers irogers at google.com
Thu Apr 18 23:22:18 PDT 2024


On Thu, Apr 18, 2024 at 1:32 PM Liang, Kan <kan.liang at 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 at 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;
> >  }
> >  |



More information about the linux-riscv mailing list