Re: [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map

From: James Clark
Date: Mon Sep 04 2023 - 11:09:48 EST

Next message: Peter Zijlstra: "Re: [PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning""
Previous message: Uros Bizjak: "[PATCH] mm/vmstat: Use this_cpu_try_cmpxchg in mod_{zone,node}_state"
In reply to: Ian Rogers: "Re: [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 04/09/2023 15:56, Ian Rogers wrote:
> 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
>

No that seems to be the whole thing, running with -vvv and pasting
everything:

$ perf record -e cs_etm/@tmc_etr0/ -vvv -- true
event syntax error: 'cs_etm/@tmc_etr0/'
\___ 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

I previously mentioned that this is a Coresight thing, but I saw that it
may actually be the more generic "drv_str". Unless nothing other than
Coresight is using it, then it's just a Coresight thing.

>> 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: ':' |
>>>

Next message: Peter Zijlstra: "Re: [PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning""
Previous message: Uros Bizjak: "[PATCH] mm/vmstat: Use this_cpu_try_cmpxchg in mod_{zone,node}_state"
In reply to: Ian Rogers: "Re: [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]