* [PATCH v1 1/3] perf parse-event: Avoid BPF test segv
2023-07-28 0:12 [PATCH v1 0/3] Remove BPF arrays from perf event parsing Ian Rogers
@ 2023-07-28 0:12 ` Ian Rogers
2023-07-28 0:12 ` [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map Ian Rogers
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Ian Rogers @ 2023-07-28 0:12 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Eduard Zingerman, Kan Liang, Rob Herring,
linux-perf-users, linux-kernel, bpf, Wang Nan, Wang ShaoBo,
YueHaibing, He Kuang
Cc: Ian Rogers
loc is passed as NULL in tools/perf/tests/bpf.c do_test, meaning
errors trigger a segv when trying to access. Add the missing NULL
check.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/parse-events.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 926d3ac97324..02647313c918 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -758,7 +758,7 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state,
return 0;
errout:
- parse_events_error__handle(parse_state->error, param.loc->first_column,
+ parse_events_error__handle(parse_state->error, param.loc ? param.loc->first_column : 0,
strdup(errbuf), strdup("(add -v to see detail)"));
return err;
}
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map
2023-07-28 0:12 [PATCH v1 0/3] Remove BPF arrays from perf event parsing Ian Rogers
2023-07-28 0:12 ` [PATCH v1 1/3] perf parse-event: Avoid BPF test segv Ian Rogers
@ 2023-07-28 0:12 ` Ian Rogers
2023-09-04 11:02 ` James Clark
2023-07-28 0:12 ` [PATCH v1 3/3] perf parse-events: Remove array remnants Ian Rogers
2023-07-28 14:29 ` [PATCH v1 0/3] Remove BPF arrays from perf event parsing Arnaldo Carvalho de Melo
3 siblings, 1 reply; 9+ messages in thread
From: Ian Rogers @ 2023-07-28 0:12 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Eduard Zingerman, Kan Liang, Rob Herring,
linux-perf-users, linux-kernel, bpf, Wang Nan, Wang ShaoBo,
YueHaibing, He Kuang
Cc: Ian Rogers
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.
Signed-off-by: Ian Rogers <irogers@google.com>
---
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: ':' |
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map
2023-07-28 0:12 ` [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map Ian Rogers
@ 2023-09-04 11:02 ` James Clark
2023-09-04 14:56 ` Ian Rogers
0 siblings, 1 reply; 9+ messages in thread
From: James Clark @ 2023-09-04 11:02 UTC (permalink / raw)
To: Ian Rogers, Arnaldo Carvalho de Melo, coresight@lists.linaro.org
Cc: Alexander Shishkin, Jiri Olsa, Namhyung Kim, Adrian Hunter,
Eduard Zingerman, Kan Liang, Rob Herring, linux-perf-users,
linux-kernel, bpf, Wang Nan, Wang ShaoBo, YueHaibing, He Kuang,
Peter Zijlstra, Ingo Molnar, Mark Rutland
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.
Thanks
James
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> 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: ':' |
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map
2023-09-04 11:02 ` James Clark
@ 2023-09-04 14:56 ` Ian Rogers
2023-09-04 15:09 ` James Clark
0 siblings, 1 reply; 9+ messages in thread
From: Ian Rogers @ 2023-09-04 14:56 UTC (permalink / raw)
To: James Clark
Cc: Arnaldo Carvalho de Melo, coresight@lists.linaro.org,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Adrian Hunter,
Eduard Zingerman, Kan Liang, Rob Herring, linux-perf-users,
linux-kernel, bpf, Wang Nan, Wang ShaoBo, YueHaibing, He Kuang,
Peter Zijlstra, Ingo Molnar, Mark Rutland
On Mon, Sep 4, 2023 at 4:02 AM James Clark <james.clark@arm.com> 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@google.com>
> > ---
> > 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: ':' |
> >
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map
2023-09-04 14:56 ` Ian Rogers
@ 2023-09-04 15:09 ` James Clark
0 siblings, 0 replies; 9+ messages in thread
From: James Clark @ 2023-09-04 15:09 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, coresight@lists.linaro.org,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Adrian Hunter,
Eduard Zingerman, Kan Liang, Rob Herring, linux-perf-users,
linux-kernel, bpf, Wang Nan, Wang ShaoBo, YueHaibing, He Kuang,
Peter Zijlstra, Ingo Molnar, Mark Rutland
On 04/09/2023 15:56, Ian Rogers wrote:
> On Mon, Sep 4, 2023 at 4:02 AM James Clark <james.clark@arm.com> 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@google.com>
>>> ---
>>> 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: ':' |
>>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 3/3] perf parse-events: Remove array remnants
2023-07-28 0:12 [PATCH v1 0/3] Remove BPF arrays from perf event parsing Ian Rogers
2023-07-28 0:12 ` [PATCH v1 1/3] perf parse-event: Avoid BPF test segv Ian Rogers
2023-07-28 0:12 ` [PATCH v1 2/3] perf tools: Revert enable indices setting syntax for BPF map Ian Rogers
@ 2023-07-28 0:12 ` Ian Rogers
2023-07-28 14:29 ` [PATCH v1 0/3] Remove BPF arrays from perf event parsing Arnaldo Carvalho de Melo
3 siblings, 0 replies; 9+ messages in thread
From: Ian Rogers @ 2023-07-28 0:12 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Adrian Hunter, Eduard Zingerman, Kan Liang, Rob Herring,
linux-perf-users, linux-kernel, bpf, Wang Nan, Wang ShaoBo,
YueHaibing, He Kuang
Cc: Ian Rogers
parse_events_array was set up by event term parsing, which no longer
exists. Remove this struct and references to it.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/bpf-loader.c | 101 ---------------------------------
tools/perf/util/parse-events.c | 8 ---
tools/perf/util/parse-events.h | 10 ----
3 files changed, 119 deletions(-)
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 44cde27d6389..108eca9ddad4 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -1091,16 +1091,12 @@ enum bpf_map_op_type {
enum bpf_map_key_type {
BPF_MAP_KEY_ALL,
- BPF_MAP_KEY_RANGES,
};
struct bpf_map_op {
struct list_head list;
enum bpf_map_op_type op_type;
enum bpf_map_key_type key_type;
- union {
- struct parse_events_array array;
- } k;
union {
u64 value;
struct evsel *evsel;
@@ -1116,8 +1112,6 @@ bpf_map_op__delete(struct bpf_map_op *op)
{
if (!list_empty(&op->list))
list_del_init(&op->list);
- if (op->key_type == BPF_MAP_KEY_RANGES)
- parse_events__clear_array(&op->k.array);
free(op);
}
@@ -1196,18 +1190,6 @@ bpf_map_op_setkey(struct bpf_map_op *op, struct parse_events_term *term)
if (!term)
return 0;
- if (term->array.nr_ranges) {
- size_t memsz = term->array.nr_ranges *
- sizeof(op->k.array.ranges[0]);
-
- op->k.array.ranges = memdup(term->array.ranges, memsz);
- if (!op->k.array.ranges) {
- pr_debug("Not enough memory to alloc indices for map\n");
- return -ENOMEM;
- }
- op->key_type = BPF_MAP_KEY_RANGES;
- op->k.array.nr_ranges = term->array.nr_ranges;
- }
return 0;
}
@@ -1244,18 +1226,6 @@ bpf_map_op__clone(struct bpf_map_op *op)
}
INIT_LIST_HEAD(&newop->list);
- if (op->key_type == BPF_MAP_KEY_RANGES) {
- size_t memsz = op->k.array.nr_ranges *
- sizeof(op->k.array.ranges[0]);
-
- newop->k.array.ranges = memdup(op->k.array.ranges, memsz);
- if (!newop->k.array.ranges) {
- pr_debug("Failed to alloc indices for map\n");
- free(newop);
- return NULL;
- }
- }
-
return newop;
}
@@ -1456,40 +1426,6 @@ struct bpf_obj_config__map_func bpf_obj_config__map_funcs[] = {
{"event", bpf_map__config_event},
};
-static int
-config_map_indices_range_check(struct parse_events_term *term,
- struct bpf_map *map,
- const char *map_name)
-{
- struct parse_events_array *array = &term->array;
- unsigned int i;
-
- if (!array->nr_ranges)
- return 0;
- if (!array->ranges) {
- pr_debug("ERROR: map %s: array->nr_ranges is %d but range array is NULL\n",
- map_name, (int)array->nr_ranges);
- return -BPF_LOADER_ERRNO__INTERNAL;
- }
-
- if (!map) {
- pr_debug("Map '%s' is invalid\n", map_name);
- return -BPF_LOADER_ERRNO__INTERNAL;
- }
-
- for (i = 0; i < array->nr_ranges; i++) {
- unsigned int start = array->ranges[i].start;
- size_t length = array->ranges[i].length;
- unsigned int idx = start + length - 1;
-
- if (idx >= bpf_map__max_entries(map)) {
- pr_debug("ERROR: index %d too large\n", idx);
- return -BPF_LOADER_ERRNO__OBJCONF_MAP_IDX2BIG;
- }
- }
- return 0;
-}
-
static int
bpf__obj_config_map(struct bpf_object *obj,
struct parse_events_term *term,
@@ -1525,12 +1461,6 @@ bpf__obj_config_map(struct bpf_object *obj,
goto out;
}
- *key_scan_pos += strlen(map_opt);
- err = config_map_indices_range_check(term, map, map_name);
- if (err)
- goto out;
- *key_scan_pos -= strlen(map_opt);
-
for (i = 0; i < ARRAY_SIZE(bpf_obj_config__map_funcs); i++) {
struct bpf_obj_config__map_func *func =
&bpf_obj_config__map_funcs[i];
@@ -1579,7 +1509,6 @@ typedef int (*map_config_func_t)(const char *name, int map_fd,
const struct bpf_map *map,
struct bpf_map_op *op,
void *pkey, void *arg);
-
static int
foreach_key_array_all(map_config_func_t func,
void *arg, const char *name,
@@ -1600,32 +1529,6 @@ foreach_key_array_all(map_config_func_t func,
return 0;
}
-static int
-foreach_key_array_ranges(map_config_func_t func, void *arg,
- const char *name, int map_fd,
- const struct bpf_map *map,
- struct bpf_map_op *op)
-{
- unsigned int i, j;
- int err;
-
- for (i = 0; i < op->k.array.nr_ranges; i++) {
- unsigned int start = op->k.array.ranges[i].start;
- size_t length = op->k.array.ranges[i].length;
-
- for (j = 0; j < length; j++) {
- unsigned int idx = start + j;
-
- err = func(name, map_fd, map, op, &idx, arg);
- if (err) {
- pr_debug("ERROR: failed to insert value to %s[%u]\n",
- name, idx);
- return err;
- }
- }
- }
- return 0;
-}
static int
bpf_map_config_foreach_key(struct bpf_map *map,
@@ -1666,10 +1569,6 @@ bpf_map_config_foreach_key(struct bpf_map *map,
err = foreach_key_array_all(func, arg, name,
map_fd, map, op);
break;
- case BPF_MAP_KEY_RANGES:
- err = foreach_key_array_ranges(func, arg, name,
- map_fd, map, op);
- break;
default:
pr_debug("ERROR: keytype for map '%s' invalid\n",
name);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 0e2004511cf5..70841b0febf3 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2717,9 +2717,6 @@ int parse_events_term__clone(struct parse_events_term **new,
void parse_events_term__delete(struct parse_events_term *term)
{
- if (term->array.nr_ranges)
- zfree(&term->array.ranges);
-
if (term->type_val != PARSE_EVENTS__TERM_TYPE_NUM)
zfree(&term->val.str);
@@ -2770,11 +2767,6 @@ void parse_events_terms__delete(struct list_head *terms)
free(terms);
}
-void parse_events__clear_array(struct parse_events_array *a)
-{
- zfree(&a->ranges);
-}
-
void parse_events_evlist_error(struct parse_events_state *parse_state,
int idx, const char *str)
{
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index e59b33805886..b77ff619a623 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -81,17 +81,8 @@ enum {
__PARSE_EVENTS__TERM_TYPE_NR,
};
-struct parse_events_array {
- size_t nr_ranges;
- struct {
- unsigned int start;
- size_t length;
- } *ranges;
-};
-
struct parse_events_term {
char *config;
- struct parse_events_array array;
union {
char *str;
u64 num;
@@ -162,7 +153,6 @@ int parse_events_term__clone(struct parse_events_term **new,
void parse_events_term__delete(struct parse_events_term *term);
void parse_events_terms__delete(struct list_head *terms);
void parse_events_terms__purge(struct list_head *terms);
-void parse_events__clear_array(struct parse_events_array *a);
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);
int parse_events_name(struct list_head *list, const char *name);
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 0/3] Remove BPF arrays from perf event parsing
2023-07-28 0:12 [PATCH v1 0/3] Remove BPF arrays from perf event parsing Ian Rogers
` (2 preceding siblings ...)
2023-07-28 0:12 ` [PATCH v1 3/3] perf parse-events: Remove array remnants Ian Rogers
@ 2023-07-28 14:29 ` Arnaldo Carvalho de Melo
2023-07-28 15:45 ` Ian Rogers
3 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-07-28 14:29 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Eduard Zingerman,
Kan Liang, Rob Herring, linux-perf-users, linux-kernel, bpf,
Wang Nan, Wang ShaoBo, YueHaibing, He Kuang
Em Thu, Jul 27, 2023 at 05:12:09PM -0700, Ian Rogers escreveu:
> Event parsing was recently refactored:
> https://lore.kernel.org/all/20230502223851.2234828-1-irogers@google.com/
>
> During these changes I wanted to get coverage of all parts of the
> parse-events.y and found that I couldn't test the array code.
>
> The first patch fixes a BPF testing issue.
> The 2nd and 3rd patch remove the BPF array event parsing code so that
> it isn't adding complexity to event parsing.
Thanks, applied and tested that 'perf trace' using the old BPF perf
integration continues to work modulo the sockaddr collectors, but that
is a problem with a newer kernel BPF verifier or clang, will
investigate.
With those commented out:
diff --git a/tools/perf/examples/bpf/augmented_raw_syscalls.c b/tools/perf/examples/bpf/augmented_raw_syscalls.c
index 9a03189d33d3816a..97871e2bd3a47bdf 100644
--- a/tools/perf/examples/bpf/augmented_raw_syscalls.c
+++ b/tools/perf/examples/bpf/augmented_raw_syscalls.c
@@ -176,6 +176,7 @@ int syscall_unaugmented(struct syscall_enter_args *args)
return 1;
}
+#if 0
/*
* These will be tail_called from SEC("raw_syscalls:sys_enter"), so will find in
* augmented_args_tmp what was read by that raw_syscalls:sys_enter and go
@@ -219,6 +220,7 @@ int sys_enter_sendto(struct syscall_enter_args *args)
return augmented__output(args, augmented_args, len + socklen);
}
+#endif
SEC("!syscalls:sys_enter_open")
int sys_enter_open(struct syscall_enter_args *args)
# perf trace -e /home/acme/git/perf-tools-next/tools/perf/examples/bpf/augmented_raw_syscalls.c,open*
0.000 systemd-oomd/952 openat(dfd: CWD, filename: "/proc/meminfo", flags: RDONLY|CLOEXEC) = 12
95.302 thermald/1231 openat(dfd: CWD, filename: "/sys/class/powercap/intel-rapl/intel-rapl:0/intel-rapl:0:2/energy_uj") = 13
95.586 thermald/1231 openat(dfd: CWD, filename: "/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj") = 13
95.857 thermald/1231 openat(dfd: CWD, filename: "/sys/class/thermal/thermal_zone2/temp") = 13
250.064 systemd-oomd/952 openat(dfd: CWD, filename: "/proc/meminfo", flags: RDONLY|CLOEXEC) = 12
500.054 systemd-oomd/952 openat(dfd: CWD, filename: "/proc/meminfo", flags: RDONLY|CLOEXEC) = 12
500.472 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
500.669 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
500.777 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
500.873 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
500.968 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
501.062 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
501.435 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
501.585 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
501.676 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
501.761 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
501.845 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
501.929 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
502.118 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
502.272 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
502.367 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
502.454 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
502.542 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
502.628 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
502.796 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
502.928 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
503.037 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
503.162 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
503.255 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
503.341 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
503.495 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
503.620 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
503.703 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
503.782 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
503.861 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
503.939 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
504.079 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1001.slice/user@1001.service/app.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
504.240 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1001.slice/user@1001.service/app.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
^C
- Arnaldo
> Ian Rogers (3):
> perf parse-event: Avoid BPF test segv
> perf tools: Revert enable indices setting syntax for BPF map
> perf parse-events: Remove array remnants
>
> tools/perf/util/bpf-loader.c | 101 ---------------------------
> tools/perf/util/parse-events.c | 18 +----
> tools/perf/util/parse-events.h | 10 ---
> tools/perf/util/parse-events.l | 11 ---
> tools/perf/util/parse-events.y | 122 ---------------------------------
> 5 files changed, 2 insertions(+), 260 deletions(-)
>
> --
> 2.41.0.487.g6d72f3e995-goog
>
--
- Arnaldo
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 0/3] Remove BPF arrays from perf event parsing
2023-07-28 14:29 ` [PATCH v1 0/3] Remove BPF arrays from perf event parsing Arnaldo Carvalho de Melo
@ 2023-07-28 15:45 ` Ian Rogers
0 siblings, 0 replies; 9+ messages in thread
From: Ian Rogers @ 2023-07-28 15:45 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Eduard Zingerman,
Kan Liang, Rob Herring, linux-perf-users, linux-kernel, bpf,
Wang Nan, Wang ShaoBo, YueHaibing, He Kuang
On Fri, Jul 28, 2023 at 7:29 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Jul 27, 2023 at 05:12:09PM -0700, Ian Rogers escreveu:
> > Event parsing was recently refactored:
> > https://lore.kernel.org/all/20230502223851.2234828-1-irogers@google.com/
> >
> > During these changes I wanted to get coverage of all parts of the
> > parse-events.y and found that I couldn't test the array code.
> >
> > The first patch fixes a BPF testing issue.
> > The 2nd and 3rd patch remove the BPF array event parsing code so that
> > it isn't adding complexity to event parsing.
>
> Thanks, applied and tested that 'perf trace' using the old BPF perf
> integration continues to work modulo the sockaddr collectors, but that
> is a problem with a newer kernel BPF verifier or clang, will
> investigate.
>
> With those commented out:
>
> diff --git a/tools/perf/examples/bpf/augmented_raw_syscalls.c b/tools/perf/examples/bpf/augmented_raw_syscalls.c
> index 9a03189d33d3816a..97871e2bd3a47bdf 100644
> --- a/tools/perf/examples/bpf/augmented_raw_syscalls.c
> +++ b/tools/perf/examples/bpf/augmented_raw_syscalls.c
> @@ -176,6 +176,7 @@ int syscall_unaugmented(struct syscall_enter_args *args)
> return 1;
> }
>
> +#if 0
> /*
> * These will be tail_called from SEC("raw_syscalls:sys_enter"), so will find in
> * augmented_args_tmp what was read by that raw_syscalls:sys_enter and go
> @@ -219,6 +220,7 @@ int sys_enter_sendto(struct syscall_enter_args *args)
>
> return augmented__output(args, augmented_args, len + socklen);
> }
> +#endif
>
> SEC("!syscalls:sys_enter_open")
> int sys_enter_open(struct syscall_enter_args *args)
>
>
>
>
> # perf trace -e /home/acme/git/perf-tools-next/tools/perf/examples/bpf/augmented_raw_syscalls.c,open*
> 0.000 systemd-oomd/952 openat(dfd: CWD, filename: "/proc/meminfo", flags: RDONLY|CLOEXEC) = 12
> 95.302 thermald/1231 openat(dfd: CWD, filename: "/sys/class/powercap/intel-rapl/intel-rapl:0/intel-rapl:0:2/energy_uj") = 13
> 95.586 thermald/1231 openat(dfd: CWD, filename: "/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj") = 13
> 95.857 thermald/1231 openat(dfd: CWD, filename: "/sys/class/thermal/thermal_zone2/temp") = 13
> 250.064 systemd-oomd/952 openat(dfd: CWD, filename: "/proc/meminfo", flags: RDONLY|CLOEXEC) = 12
> 500.054 systemd-oomd/952 openat(dfd: CWD, filename: "/proc/meminfo", flags: RDONLY|CLOEXEC) = 12
> 500.472 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
> 500.669 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
> 500.777 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
> 500.873 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
> 500.968 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
> 501.062 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/session.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
> 501.435 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
> 501.585 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
> 501.676 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
> 501.761 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
> 501.845 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
> 501.929 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
> 502.118 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
> 502.272 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
> 502.367 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
> 502.454 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
> 502.542 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
> 502.628 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-gnome\x2dsession\x2dmanager.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
> 502.796 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
> 502.928 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
> 503.037 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
> 503.162 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
> 503.255 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
> 503.341 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
> 503.495 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
> 503.620 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
> 503.703 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.min", flags: RDONLY|CLOEXEC) = 12
> 503.782 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.low", flags: RDONLY|CLOEXEC) = 12
> 503.861 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.swap.current", flags: RDONLY|CLOEXEC) = 12
> 503.939 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/system.slice/memory.stat", flags: RDONLY|CLOEXEC) = 12
> 504.079 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1001.slice/user@1001.service/app.slice/memory.pressure", flags: RDONLY|CLOEXEC) = 12
> 504.240 systemd-oomd/952 openat(dfd: CWD, filename: "/sys/fs/cgroup/user.slice/user-1001.slice/user@1001.service/app.slice/memory.current", flags: RDONLY|CLOEXEC) = 12
> ^C
Thanks Arnaldo, it could also be that we need the code and it is my
user error which hasn't found a way of testing it. If the original
Huawei authors could let us know?
I suspect with this code gone, there is more bpf-loader map related
code that can also go as there is no way to use it. I wonder if a
better longer-term strategy is to remove BPF events entirely. My
thoughts on this are:
1) the code had bitrotten a few releases ago, I updated it but the
fact it rotted means I suspect nobody uses it.
2) the map code removed here I think is just redundant and it adds
complexity, so removing it is value add - unless I'm mistaken :-)
Removing more is more value add.
3) the clang/llvm dependencies are problematic in the build, removing
these would I think be a good value add
4) in the event parsing the BPF events have paths with '/' as the path
separator. This matches the modifier separator used by events and so I
think this leads to complexity.
5) the behavior achieved by BPF events can now be achieved with BPF
filters, which are more user friendly:
https://lore.kernel.org/all/20230314234237.3008956-1-namhyung@kernel.org/
There is a dependency with perf trace, but I think perf trace should
use BPF skeletons for augmented syscalls rather than a BPF event.
Using a BPF event in the perf trace feels kind of hacky and I suspect
perf trace users don't know they need to add the event to make
augmentation work.
Thanks,
Ian
> - Arnaldo
>
> > Ian Rogers (3):
> > perf parse-event: Avoid BPF test segv
> > perf tools: Revert enable indices setting syntax for BPF map
> > perf parse-events: Remove array remnants
> >
> > tools/perf/util/bpf-loader.c | 101 ---------------------------
> > tools/perf/util/parse-events.c | 18 +----
> > tools/perf/util/parse-events.h | 10 ---
> > tools/perf/util/parse-events.l | 11 ---
> > tools/perf/util/parse-events.y | 122 ---------------------------------
> > 5 files changed, 2 insertions(+), 260 deletions(-)
> >
> > --
> > 2.41.0.487.g6d72f3e995-goog
> >
>
> --
>
> - Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread