Re: [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map
From: Ian Rogers
Date: Mon Sep 04 2023 - 10:56:46 EST
On Mon, Sep 4, 2023 at 4:02 AM James Clark <james.clark@xxxxxxx> wrote:
>
>
>
> On 28/07/2023 01:12, Ian Rogers wrote:
> > This reverts commit e571e029bdbf ("perf tools: Enable indices setting
> > syntax for BPF map").
> >
> > The reverted commit added a notion of arrays that could be set as
> > event terms for BPF events. The parsing hasn't worked over multiple
> > Linux releases. Given the broken nature of the parsing it appears the
> > code isn't in use, nor could I find a way for it to be used to add a
> > test.
> >
> > The original commit contains a test in the commit message,
> > however, running it yields:
> > ```
> > $ perf record -e './test_bpf_map_3.c/map:channel.value[0,1,2,3...5]=101/' usleep 2
> > event syntax error: '..pf_map_3.c/map:channel.value[0,1,2,3...5]=101/'
> > \___ parser error
> > Run 'perf list' for a list of valid events
> >
> > Usage: perf record [<options>] [<command>]
> > or: perf record [<options>] -- <command> [<options>]
> >
> > -e, --event <event> event selector. use 'perf list' to list available events
> > ```
> >
> > Given the code can't be used this commit reverts and removes it.
> >
>
> Hi Ian,
>
> Unfortunately this revert breaks Coresight sink argument parsing.
>
> Before:
>
> $ perf record -e cs_etm/@tmc_etr0/ -- true
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 4.008 MB perf.data ]
>
> After:
>
> $ perf record -e cs_etm/@tmc_etr0/ -- true
> event syntax error: 'cs_etm/@tmc_etr0/'
> \___ parser error
>
> I can't really see how it's related to the array syntax that the commit
> messages mention, but it could either be that the revert wasn't applied
> cleanly or just some unintended side effect.
>
> We should probably add a cross platform parsing test for Coresight
> arguments, but I don't know whether we should just blindly revert the
> revert for now, or work on a new change that explicitly fixes the
> Coresight case.
Agreed, I'll take a look. Any chance you could post the full error
message? I suspect there's a first error hiding in there too.
Thanks,
Ian
> Thanks
> James
>
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> > tools/perf/util/parse-events.c | 8 +--
> > tools/perf/util/parse-events.l | 11 ---
> > tools/perf/util/parse-events.y | 122 ---------------------------------
> > 3 files changed, 1 insertion(+), 140 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 02647313c918..0e2004511cf5 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -800,13 +800,7 @@ parse_events_config_bpf(struct parse_events_state *parse_state,
> >
> > parse_events_error__handle(parse_state->error, idx,
> > strdup(errbuf),
> > - strdup(
> > -"Hint:\tValid config terms:\n"
> > -" \tmap:[<arraymap>].value<indices>=[value]\n"
> > -" \tmap:[<eventmap>].event<indices>=[event]\n"
> > -"\n"
> > -" \twhere <indices> is something like [0,3...5] or [all]\n"
> > -" \t(add -v to see detail)"));
> > + NULL);
> > return err;
> > }
> > }
> > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > index 99335ec586ae..d7d084cc4140 100644
> > --- a/tools/perf/util/parse-events.l
> > +++ b/tools/perf/util/parse-events.l
> > @@ -175,7 +175,6 @@ do { \
> > %x mem
> > %s config
> > %x event
> > -%x array
> >
> > group [^,{}/]*[{][^}]*[}][^,{}/]*
> > event_pmu [^,{}/]+[/][^/]*[/][^,{}/]*
> > @@ -251,14 +250,6 @@ non_digit [^0-9]
> > }
> > }
> >
> > -<array>{
> > -"]" { BEGIN(config); return ']'; }
> > -{num_dec} { return value(yyscanner, 10); }
> > -{num_hex} { return value(yyscanner, 16); }
> > -, { return ','; }
> > -"\.\.\." { return PE_ARRAY_RANGE; }
> > -}
> > -
> > <config>{
> > /*
> > * Please update config_term_names when new static term is added.
> > @@ -302,8 +293,6 @@ r0x{num_raw_hex} { return str(yyscanner, PE_RAW); }
> > {lc_type}-{lc_op_result} { return lc_str(yyscanner, _parse_state); }
> > {lc_type}-{lc_op_result}-{lc_op_result} { return lc_str(yyscanner, _parse_state); }
> > {name_minus} { return str(yyscanner, PE_NAME); }
> > -\[all\] { return PE_ARRAY_ALL; }
> > -"[" { BEGIN(array); return '['; }
> > @{drv_cfg_term} { return drv_str(yyscanner, PE_DRV_CFG_TERM); }
> > }
> >
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 454577f7aff6..5a90e7874c59 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -64,7 +64,6 @@ static void free_list_evsel(struct list_head* list_evsel)
> > %token PE_LEGACY_CACHE
> > %token PE_PREFIX_MEM
> > %token PE_ERROR
> > -%token PE_ARRAY_ALL PE_ARRAY_RANGE
> > %token PE_DRV_CFG_TERM
> > %token PE_TERM_HW
> > %type <num> PE_VALUE
> > @@ -108,11 +107,6 @@ static void free_list_evsel(struct list_head* list_evsel)
> > %type <list_evsel> groups
> > %destructor { free_list_evsel ($$); } <list_evsel>
> > %type <tracepoint_name> tracepoint_name
> > -%destructor { free ($$.sys); free ($$.event); } <tracepoint_name>
> > -%type <array> array
> > -%type <array> array_term
> > -%type <array> array_terms
> > -%destructor { free ($$.ranges); } <array>
> > %type <hardware_term> PE_TERM_HW
> > %destructor { free ($$.str); } <hardware_term>
> >
> > @@ -127,7 +121,6 @@ static void free_list_evsel(struct list_head* list_evsel)
> > char *sys;
> > char *event;
> > } tracepoint_name;
> > - struct parse_events_array array;
> > struct hardware_term {
> > char *str;
> > u64 num;
> > @@ -878,121 +871,6 @@ PE_TERM
> >
> > $$ = term;
> > }
> > -|
> > -name_or_raw array '=' name_or_legacy
> > -{
> > - struct parse_events_term *term;
> > - int err = parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_USER, $1, $4, &@1, &@4);
> > -
> > - if (err) {
> > - free($1);
> > - free($4);
> > - free($2.ranges);
> > - PE_ABORT(err);
> > - }
> > - term->array = $2;
> > - $$ = term;
> > -}
> > -|
> > -name_or_raw array '=' PE_VALUE
> > -{
> > - struct parse_events_term *term;
> > - int err = parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER, $1, $4, false, &@1, &@4);
> > -
> > - if (err) {
> > - free($1);
> > - free($2.ranges);
> > - PE_ABORT(err);
> > - }
> > - term->array = $2;
> > - $$ = term;
> > -}
> > -|
> > -PE_DRV_CFG_TERM
> > -{
> > - struct parse_events_term *term;
> > - char *config = strdup($1);
> > - int err;
> > -
> > - if (!config)
> > - YYNOMEM;
> > - err = parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG, config, $1, &@1, NULL);
> > - if (err) {
> > - free($1);
> > - free(config);
> > - PE_ABORT(err);
> > - }
> > - $$ = term;
> > -}
> > -
> > -array:
> > -'[' array_terms ']'
> > -{
> > - $$ = $2;
> > -}
> > -|
> > -PE_ARRAY_ALL
> > -{
> > - $$.nr_ranges = 0;
> > - $$.ranges = NULL;
> > -}
> > -
> > -array_terms:
> > -array_terms ',' array_term
> > -{
> > - struct parse_events_array new_array;
> > -
> > - new_array.nr_ranges = $1.nr_ranges + $3.nr_ranges;
> > - new_array.ranges = realloc($1.ranges,
> > - sizeof(new_array.ranges[0]) *
> > - new_array.nr_ranges);
> > - if (!new_array.ranges)
> > - YYNOMEM;
> > - memcpy(&new_array.ranges[$1.nr_ranges], $3.ranges,
> > - $3.nr_ranges * sizeof(new_array.ranges[0]));
> > - free($3.ranges);
> > - $$ = new_array;
> > -}
> > -|
> > -array_term
> > -
> > -array_term:
> > -PE_VALUE
> > -{
> > - struct parse_events_array array;
> > -
> > - array.nr_ranges = 1;
> > - array.ranges = malloc(sizeof(array.ranges[0]));
> > - if (!array.ranges)
> > - YYNOMEM;
> > - array.ranges[0].start = $1;
> > - array.ranges[0].length = 1;
> > - $$ = array;
> > -}
> > -|
> > -PE_VALUE PE_ARRAY_RANGE PE_VALUE
> > -{
> > - struct parse_events_array array;
> > -
> > - if ($3 < $1) {
> > - struct parse_events_state *parse_state = _parse_state;
> > - struct parse_events_error *error = parse_state->error;
> > - char *err_str;
> > -
> > - if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0)
> > - err_str = NULL;
> > -
> > - parse_events_error__handle(error, @1.first_column, err_str, NULL);
> > - YYABORT;
> > - }
> > - array.nr_ranges = 1;
> > - array.ranges = malloc(sizeof(array.ranges[0]));
> > - if (!array.ranges)
> > - YYNOMEM;
> > - array.ranges[0].start = $1;
> > - array.ranges[0].length = $3 - $1 + 1;
> > - $$ = array;
> > -}
> >
> > sep_dc: ':' |
> >