Event parsing was recently refactored:
https://lore.kernel.org/all/[email protected]/
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.
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Em Thu, Jul 27, 2023 at 05:12:09PM -0700, Ian Rogers escreveu:
> Event parsing was recently refactored:
> https://lore.kernel.org/all/[email protected]/
>
> 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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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
On Fri, Jul 28, 2023 at 7:29 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> Em Thu, Jul 27, 2023 at 05:12:09PM -0700, Ian Rogers escreveu:
> > Event parsing was recently refactored:
> > https://lore.kernel.org/all/[email protected]/
> >
> > 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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/
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
On 04/09/2023 15:56, Ian Rogers wrote:
> On Mon, Sep 4, 2023 at 4:02 AM James Clark <[email protected]> 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 <[email protected]>
>>> ---
>>> 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: ':' |
>>>
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 <[email protected]>
> ---
> 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: ':' |
>