LinuxLists.cc - [PATCH 0/3] perf tools: Allow to enable/disable events via control pipe

[permalink] [raw]

Subject: [PATCH 1/3] perf tools: Add evlist__disable_evsel/evlist__enable_evsel

Adding interface to enable/disable single event in the
evlist based on its name. It will be used later in new
control enable/disable interface.

Keeping the evlist::enabled true when one or more events
are enabled so the toggle can work properly and toggle
evlist to disabled state.

Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/util/evlist.c | 69 ++++++++++++++++++++++++++++++++++++++--
tools/perf/util/evlist.h | 2 ++
2 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 493819173a8e..70aff26612a9 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -370,7 +370,30 @@ bool evsel__cpu_iter_skip(struct evsel *ev, int cpu)
return true;
}

-void evlist__disable(struct evlist *evlist)
+static int evsel__strcmp(struct evsel *pos, char *evsel_name)
+{
+ if (!evsel_name)
+ return 0;
+ if (evsel__is_dummy_event(pos))
+ return 1;
+ return strcmp(pos->name, evsel_name);
+}
+
+static int evlist__is_enabled(struct evlist *evlist)
+{
+ struct evsel *pos;
+
+ evlist__for_each_entry(evlist, pos) {
+ if (!evsel__is_group_leader(pos) || !pos->core.fd)
+ continue;
+ /* If at least one event is enabled, evlist is enabled. */
+ if (!pos->disabled)
+ return true;
+ }
+ return false;
+}
+
+static void __evlist__disable(struct evlist *evlist, char *evsel_name)
{
struct evsel *pos;
struct affinity affinity;
@@ -386,6 +409,8 @@ void evlist__disable(struct evlist *evlist)
affinity__set(&affinity, cpu);

evlist__for_each_entry(evlist, pos) {
+ if (evsel__strcmp(pos, evsel_name))
+ continue;
if (evsel__cpu_iter_skip(pos, cpu))
continue;
if (pos->disabled || !evsel__is_group_leader(pos) || !pos->core.fd)
@@ -403,15 +428,34 @@ void evlist__disable(struct evlist *evlist)

affinity__cleanup(&affinity);
evlist__for_each_entry(evlist, pos) {
+ if (evsel__strcmp(pos, evsel_name))
+ continue;
if (!evsel__is_group_leader(pos) || !pos->core.fd)
continue;
pos->disabled = true;
}

- evlist->enabled = false;
+ /*
+ * If we disabled only single event, we need to check
+ * the enabled state of the evlist manually.
+ */
+ if (evsel_name)
+ evlist->enabled = evlist__is_enabled(evlist);
+ else
+ evlist->enabled = false;
+}
+
+void evlist__disable(struct evlist *evlist)
+{
+ __evlist__disable(evlist, NULL);
+}
+
+void evlist__disable_evsel(struct evlist *evlist, char *evsel_name)
+{
+ __evlist__disable(evlist, evsel_name);
}

-void evlist__enable(struct evlist *evlist)
+static void __evlist__enable(struct evlist *evlist, char *evsel_name)
{
struct evsel *pos;
struct affinity affinity;
@@ -424,6 +468,8 @@ void evlist__enable(struct evlist *evlist)
affinity__set(&affinity, cpu);

evlist__for_each_entry(evlist, pos) {
+ if (evsel__strcmp(pos, evsel_name))
+ continue;
if (evsel__cpu_iter_skip(pos, cpu))
continue;
if (!evsel__is_group_leader(pos) || !pos->core.fd)
@@ -433,14 +479,31 @@ void evlist__enable(struct evlist *evlist)
}
affinity__cleanup(&affinity);
evlist__for_each_entry(evlist, pos) {
+ if (evsel__strcmp(pos, evsel_name))
+ continue;
if (!evsel__is_group_leader(pos) || !pos->core.fd)
continue;
pos->disabled = false;
}

+ /*
+ * Even single event sets the 'enabled' for evlist,
+ * so the toggle can work properly and toggle to
+ * 'disabled' state.
+ */
evlist->enabled = true;
}

+void evlist__enable(struct evlist *evlist)
+{
+ __evlist__enable(evlist, NULL);
+}
+
+void evlist__enable_evsel(struct evlist *evlist, char *evsel_name)
+{
+ __evlist__enable(evlist, evsel_name);
+}
+
void evlist__toggle_enable(struct evlist *evlist)
{
(evlist->enabled ? evlist__disable : evlist__enable)(evlist);
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 9b0c795736bb..1aae75895dea 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -186,6 +186,8 @@ size_t evlist__mmap_size(unsigned long pages);
void evlist__disable(struct evlist *evlist);
void evlist__enable(struct evlist *evlist);
void evlist__toggle_enable(struct evlist *evlist);
+void evlist__disable_evsel(struct evlist *evlist, char *evsel_name);
+void evlist__enable_evsel(struct evlist *evlist, char *evsel_name);

int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel, int idx);

--
2.26.2

2020-12-06 17:09:14

[permalink] [raw]

Subject: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

Adding new control events to enable/disable specific event.
The interface string for control file are:

'enable-<EVENT NAME>'
'disable-<EVENT NAME>'

when received the command, perf will scan the current evlist
for <EVENT NAME> and if found it's enabled/disabled.

Example session:

terminal 1:
# mkfifo control ack perf.pipe
# perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
Events disabled

terminal 2:
# cat perf.pipe | ./perf --no-pager script -i -

terminal 3:
# echo enable-sched:sched_process_fork > control

terminal 1:
# mkfifo control ack perf.pipe
# perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
...
event sched:sched_process_fork enabled

terminal 2:
# cat perf.pipe | ./perf --no-pager script -i -
bash 33349 [034] 149587.674295: sched:sched_process_fork: comm=bash pid=33349 child_comm=bash child_pid=34056
bash 33349 [034] 149588.239521: sched:sched_process_fork: comm=bash pid=33349 child_comm=bash child_pid=34057

terminal 3:
# echo enable-sched:sched_wakeup_new > control

terminal 1:
# mkfifo control ack perf.pipe
# perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
...
event sched:sched_wakeup_new enabled

terminal 2:
# cat perf.pipe | ./perf --no-pager script -i -
...
bash 33349 [034] 149632.228023: sched:sched_process_fork: comm=bash pid=33349 child_comm=bash child_pid=34059
bash 33349 [034] 149632.228050: sched:sched_wakeup_new: bash:34059 [120] success=1 CPU:036
bash 33349 [034] 149633.950005: sched:sched_process_fork: comm=bash pid=33349 child_comm=bash child_pid=34060
bash 33349 [034] 149633.950030: sched:sched_wakeup_new: bash:34060 [120] success=1 CPU:036

Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/builtin-record.c | 2 ++
tools/perf/builtin-stat.c | 2 ++
tools/perf/util/evlist.c | 30 +++++++++++++++++++++++++++++-
tools/perf/util/evlist.h | 4 ++++
4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d832c108a1ca..582b8fba012c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1949,6 +1949,8 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
break;
case EVLIST_CTL_CMD_ACK:
case EVLIST_CTL_CMD_UNSUPPORTED:
+ case EVLIST_CTL_CMD_ENABLE_EVSEL:
+ case EVLIST_CTL_CMD_DISABLE_EVSEL:
default:
break;
}
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 89c32692f40c..6a21fb665008 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -590,6 +590,8 @@ static void process_evlist(struct evlist *evlist, unsigned int interval)
case EVLIST_CTL_CMD_SNAPSHOT:
case EVLIST_CTL_CMD_ACK:
case EVLIST_CTL_CMD_UNSUPPORTED:
+ case EVLIST_CTL_CMD_ENABLE_EVSEL:
+ case EVLIST_CTL_CMD_DISABLE_EVSEL:
default:
break;
}
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 70aff26612a9..05723227bebf 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");

if (bytes_read > 0) {
- if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
+ if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
+ (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
+ *cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
+ } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
+ (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
+ *cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
+ } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
(sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
*cmd = EVLIST_CTL_CMD_ENABLE;
} else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
@@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
int ctlfd_pos = evlist->ctl_fd.pos;
struct pollfd *entries = evlist->core.pollfd.entries;
+ struct evsel *evsel;
+ char *evsel_name;

if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
return 0;
@@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
case EVLIST_CTL_CMD_DISABLE:
evlist__disable(evlist);
break;
+ case EVLIST_CTL_CMD_ENABLE_EVSEL:
+ evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;
+ evsel = evlist__find_evsel_by_str(evlist, evsel_name);
+ if (evsel) {
+ evlist__enable_evsel(evlist, evsel_name);
+ pr_info("event %s enabled\n", evsel->name);
+ } else {
+ pr_info("failed: can't find %s event\n", evsel_name);
+ }
+ break;
+ case EVLIST_CTL_CMD_DISABLE_EVSEL:
+ evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG) - 1;
+ evsel = evlist__find_evsel_by_str(evlist, evsel_name);
+ if (evsel) {
+ evlist__disable_evsel(evlist, evsel_name);
+ pr_info("event %s disabled\n", evsel->name);
+ } else {
+ pr_info("failed: can't find %s event\n", evsel_name);
+ }
+ break;
case EVLIST_CTL_CMD_SNAPSHOT:
break;
case EVLIST_CTL_CMD_ACK:
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 1aae75895dea..e4e8ff8831a3 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -330,6 +330,8 @@ struct evsel *evlist__reset_weak_group(struct evlist *evlist, struct evsel *evse
#define EVLIST_CTL_CMD_DISABLE_TAG "disable"
#define EVLIST_CTL_CMD_ACK_TAG "ack\n"
#define EVLIST_CTL_CMD_SNAPSHOT_TAG "snapshot"
+#define EVLIST_CTL_CMD_ENABLE_EVSEL_TAG "enable-"
+#define EVLIST_CTL_CMD_DISABLE_EVSEL_TAG "disable-"

#define EVLIST_CTL_CMD_MAX_LEN 64

@@ -337,6 +339,8 @@ enum evlist_ctl_cmd {
EVLIST_CTL_CMD_UNSUPPORTED = 0,
EVLIST_CTL_CMD_ENABLE,
EVLIST_CTL_CMD_DISABLE,
+ EVLIST_CTL_CMD_ENABLE_EVSEL,
+ EVLIST_CTL_CMD_DISABLE_EVSEL,
EVLIST_CTL_CMD_ACK,
EVLIST_CTL_CMD_SNAPSHOT,
};
--
2.26.2

2020-12-06 17:10:26

[permalink] [raw]

Subject: [PATCH 3/3] perf tools: Allow to list events via control file

Adding new control event to display all evlist events.

The interface string for control file is 'list'. When
received, perf will scan and print current evlist into
perf record terminal.

Example session:

terminal 1:
# mkfifo control ack perf.pipe
# perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
Events disabled

terminal 2:
# echo list > control

terminal 1:
# perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
...
sched:sched_kthread_stop
sched:sched_kthread_stop_ret
sched:sched_waking
sched:sched_wakeup
sched:sched_wakeup_new
sched:sched_switch
sched:sched_migrate_task
sched:sched_process_free
sched:sched_process_exit
sched:sched_wait_task
sched:sched_process_wait
sched:sched_process_fork
sched:sched_process_exec
sched:sched_stat_wait
sched:sched_stat_sleep
sched:sched_stat_iowait
sched:sched_stat_blocked
sched:sched_stat_runtime
sched:sched_pi_setprio
sched:sched_move_numa
sched:sched_stick_numa
sched:sched_swap_numa
sched:sched_wake_idle_without_ipi
dummy:HG

This new command is handy to get real event names when
wildcards are used.

Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/builtin-record.c | 1 +
tools/perf/builtin-stat.c | 1 +
tools/perf/util/evlist.c | 6 ++++++
tools/perf/util/evlist.h | 2 ++
4 files changed, 10 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 582b8fba012c..f620ed056c89 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1951,6 +1951,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
case EVLIST_CTL_CMD_UNSUPPORTED:
case EVLIST_CTL_CMD_ENABLE_EVSEL:
case EVLIST_CTL_CMD_DISABLE_EVSEL:
+ case EVLIST_CTL_CMD_LIST:
default:
break;
}
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 6a21fb665008..56f2206b5991 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -592,6 +592,7 @@ static void process_evlist(struct evlist *evlist, unsigned int interval)
case EVLIST_CTL_CMD_UNSUPPORTED:
case EVLIST_CTL_CMD_ENABLE_EVSEL:
case EVLIST_CTL_CMD_DISABLE_EVSEL:
+ case EVLIST_CTL_CMD_LIST:
default:
break;
}
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 05723227bebf..c05476ca2ff4 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1931,6 +1931,9 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
(sizeof(EVLIST_CTL_CMD_SNAPSHOT_TAG)-1))) {
*cmd = EVLIST_CTL_CMD_SNAPSHOT;
pr_debug("is snapshot\n");
+ } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_LIST_TAG,
+ (sizeof(EVLIST_CTL_CMD_LIST_TAG)-1))) {
+ *cmd = EVLIST_CTL_CMD_LIST;
}
}

@@ -1995,6 +1998,9 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
pr_info("failed: can't find %s event\n", evsel_name);
}
break;
+ case EVLIST_CTL_CMD_LIST:
+ evlist__for_each_entry(evlist, evsel)
+ pr_info("%s\n", evsel__name(evsel));
case EVLIST_CTL_CMD_SNAPSHOT:
break;
case EVLIST_CTL_CMD_ACK:
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index e4e8ff8831a3..6b8a9918fdb2 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -332,6 +332,7 @@ struct evsel *evlist__reset_weak_group(struct evlist *evlist, struct evsel *evse
#define EVLIST_CTL_CMD_SNAPSHOT_TAG "snapshot"
#define EVLIST_CTL_CMD_ENABLE_EVSEL_TAG "enable-"
#define EVLIST_CTL_CMD_DISABLE_EVSEL_TAG "disable-"
+#define EVLIST_CTL_CMD_LIST_TAG "list"

#define EVLIST_CTL_CMD_MAX_LEN 64

@@ -343,6 +344,7 @@ enum evlist_ctl_cmd {
EVLIST_CTL_CMD_DISABLE_EVSEL,
EVLIST_CTL_CMD_ACK,
EVLIST_CTL_CMD_SNAPSHOT,
+ EVLIST_CTL_CMD_LIST,
};

int evlist__parse_control(const char *str, int *ctl_fd, int *ctl_fd_ack, bool *ctl_fd_close);
--
2.26.2

2020-12-07 13:29:52

by Namhyung Kim

[permalink] [raw]

Subject: Re: [PATCH 0/3] perf tools: Allow to enable/disable events via control pipe

Hi Jiri,

On Mon, Dec 7, 2020 at 2:05 AM Jiri Olsa <[email protected]> wrote:
>
> hi,
> adding support to enable/disable specific events via control
> file via following commands:
>
> # echo enable-sched:sched_process_fork > control
> # echo disabled-sched:sched_process_fork > control
>
> The code is available in here:
> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> perf/control
>
> thanks,
> jirka

Acked-by: Namhyung Kim <[email protected]>

Thanks,
Namhyung

> ---
> Jiri Olsa (3):
> perf tools: Add evlist__disable_evsel/evlist__enable_evsel
> perf tools: Allow to enable/disable events via control file
> perf tools: Allow to list events via control file
>
> tools/perf/builtin-record.c | 3 +++
> tools/perf/builtin-stat.c | 3 +++
> tools/perf/util/evlist.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> tools/perf/util/evlist.h | 8 ++++++++
> 4 files changed, 115 insertions(+), 4 deletions(-)
>

2020-12-07 16:30:55

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf tools: Allow to list events via control file

Em Sun, Dec 06, 2020 at 06:05:19PM +0100, Jiri Olsa escreveu:
> Adding new control event to display all evlist events.
>
> The interface string for control file is 'list'. When
> received, perf will scan and print current evlist into
> perf record terminal.
>
> Example session:
>
> terminal 1:
> # mkfifo control ack perf.pipe
> # perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
> Events disabled
>
> terminal 2:
> # echo list > control
>
> terminal 1:
> # perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
> ...
> sched:sched_kthread_stop
> sched:sched_kthread_stop_ret
> sched:sched_waking
> sched:sched_wakeup
> sched:sched_wakeup_new
> sched:sched_switch
> sched:sched_migrate_task
> sched:sched_process_free
> sched:sched_process_exit
> sched:sched_wait_task
> sched:sched_process_wait
> sched:sched_process_fork
> sched:sched_process_exec
> sched:sched_stat_wait
> sched:sched_stat_sleep
> sched:sched_stat_iowait
> sched:sched_stat_blocked
> sched:sched_stat_runtime
> sched:sched_pi_setprio
> sched:sched_move_numa
> sched:sched_stick_numa
> sched:sched_swap_numa
> sched:sched_wake_idle_without_ipi
> dummy:HG
>
> This new command is handy to get real event names when
> wildcards are used.

Ok, would be nice to have a verbose mode like:

[acme@five ~]$ sudo ~acme/bin/perf record -e 'sched:*' sleep 0.001
[ perf record: Woken up 14 times to write data ]
[ perf record: Captured and wrote 0.023 MB perf.data (16 samples) ]
[acme@five ~]$ sudo ~acme/bin/perf evlist
sched:sched_kthread_stop
sched:sched_kthread_stop_ret
sched:sched_waking
sched:sched_wakeup
sched:sched_wakeup_new
sched:sched_switch
sched:sched_migrate_task
sched:sched_process_free
sched:sched_process_exit
sched:sched_wait_task
sched:sched_process_wait
sched:sched_process_fork
sched:sched_process_exec
sched:sched_stat_wait
sched:sched_stat_sleep
sched:sched_stat_iowait
sched:sched_stat_blocked
sched:sched_stat_runtime
sched:sched_pi_setprio
sched:sched_move_numa
sched:sched_stick_numa
sched:sched_swap_numa
sched:sched_wake_idle_without_ipi
# Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
[acme@five ~]$ sudo ~acme/bin/perf evlist -v
sched:sched_kthread_stop: type: 2, size: 120, config: 0x13f, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1
sched:sched_kthread_stop_ret: type: 2, size: 120, config: 0x13e, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_waking: type: 2, size: 120, config: 0x13d, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_wakeup: type: 2, size: 120, config: 0x13c, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_wakeup_new: type: 2, size: 120, config: 0x13b, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_switch: type: 2, size: 120, config: 0x13a, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_migrate_task: type: 2, size: 120, config: 0x139, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_process_free: type: 2, size: 120, config: 0x138, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_process_exit: type: 2, size: 120, config: 0x137, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_wait_task: type: 2, size: 120, config: 0x136, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_process_wait: type: 2, size: 120, config: 0x135, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_process_fork: type: 2, size: 120, config: 0x134, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_process_exec: type: 2, size: 120, config: 0x133, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_stat_wait: type: 2, size: 120, config: 0x132, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_stat_sleep: type: 2, size: 120, config: 0x131, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_stat_iowait: type: 2, size: 120, config: 0x130, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_stat_blocked: type: 2, size: 120, config: 0x12f, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_stat_runtime: type: 2, size: 120, config: 0x12e, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_pi_setprio: type: 2, size: 120, config: 0x12d, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_move_numa: type: 2, size: 120, config: 0x12c, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_stick_numa: type: 2, size: 120, config: 0x12b, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_swap_numa: type: 2, size: 120, config: 0x12a, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
sched:sched_wake_idle_without_ipi: type: 2, size: 120, config: 0x129, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
# Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
[acme@five ~]$

Also I think we should use 'evlist' instead of 'list', to be consistent.

Applied 1/3 and 2/3.

- Arnaldo

> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/builtin-record.c | 1 +
> tools/perf/builtin-stat.c | 1 +
> tools/perf/util/evlist.c | 6 ++++++
> tools/perf/util/evlist.h | 2 ++
> 4 files changed, 10 insertions(+)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 582b8fba012c..f620ed056c89 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1951,6 +1951,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> case EVLIST_CTL_CMD_UNSUPPORTED:
> case EVLIST_CTL_CMD_ENABLE_EVSEL:
> case EVLIST_CTL_CMD_DISABLE_EVSEL:
> + case EVLIST_CTL_CMD_LIST:
> default:
> break;
> }
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 6a21fb665008..56f2206b5991 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -592,6 +592,7 @@ static void process_evlist(struct evlist *evlist, unsigned int interval)
> case EVLIST_CTL_CMD_UNSUPPORTED:
> case EVLIST_CTL_CMD_ENABLE_EVSEL:
> case EVLIST_CTL_CMD_DISABLE_EVSEL:
> + case EVLIST_CTL_CMD_LIST:
> default:
> break;
> }
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 05723227bebf..c05476ca2ff4 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1931,6 +1931,9 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
> (sizeof(EVLIST_CTL_CMD_SNAPSHOT_TAG)-1))) {
> *cmd = EVLIST_CTL_CMD_SNAPSHOT;
> pr_debug("is snapshot\n");
> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_LIST_TAG,
> + (sizeof(EVLIST_CTL_CMD_LIST_TAG)-1))) {
> + *cmd = EVLIST_CTL_CMD_LIST;
> }
> }
>
> @@ -1995,6 +1998,9 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> pr_info("failed: can't find %s event\n", evsel_name);
> }
> break;
> + case EVLIST_CTL_CMD_LIST:
> + evlist__for_each_entry(evlist, evsel)
> + pr_info("%s\n", evsel__name(evsel));
> case EVLIST_CTL_CMD_SNAPSHOT:
> break;
> case EVLIST_CTL_CMD_ACK:
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index e4e8ff8831a3..6b8a9918fdb2 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -332,6 +332,7 @@ struct evsel *evlist__reset_weak_group(struct evlist *evlist, struct evsel *evse
> #define EVLIST_CTL_CMD_SNAPSHOT_TAG "snapshot"
> #define EVLIST_CTL_CMD_ENABLE_EVSEL_TAG "enable-"
> #define EVLIST_CTL_CMD_DISABLE_EVSEL_TAG "disable-"
> +#define EVLIST_CTL_CMD_LIST_TAG "list"
>
> #define EVLIST_CTL_CMD_MAX_LEN 64
>
> @@ -343,6 +344,7 @@ enum evlist_ctl_cmd {
> EVLIST_CTL_CMD_DISABLE_EVSEL,
> EVLIST_CTL_CMD_ACK,
> EVLIST_CTL_CMD_SNAPSHOT,
> + EVLIST_CTL_CMD_LIST,
> };
>
> int evlist__parse_control(const char *str, int *ctl_fd, int *ctl_fd_ack, bool *ctl_fd_close);
> --
> 2.26.2
>

--

- Arnaldo

2020-12-07 17:07:17

[permalink] [raw]

Subject: Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

Hi,

On 06.12.2020 20:05, Jiri Olsa wrote:
> Adding new control events to enable/disable specific event.
> The interface string for control file are:
>
> 'enable-<EVENT NAME>'
> 'disable-<EVENT NAME>'

<SNIP>

>
> when received the command, perf will scan the current evlist
> for <EVENT NAME> and if found it's enabled/disabled.

<SNIP>

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 70aff26612a9..05723227bebf 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
> bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
>
> if (bytes_read > 0) {
> - if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
> + (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
> + *cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
> + (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
> + *cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
> *cmd = EVLIST_CTL_CMD_ENABLE;
> } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
> @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
> int ctlfd_pos = evlist->ctl_fd.pos;
> struct pollfd *entries = evlist->core.pollfd.entries;
> + struct evsel *evsel;
> + char *evsel_name;
>
> if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
> return 0;
> @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> case EVLIST_CTL_CMD_DISABLE:
> evlist__disable(evlist);
> break;
> + case EVLIST_CTL_CMD_ENABLE_EVSEL:
> + evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;

It makes sense to check that evsel_name still points
into cmd_data buffer after assigning to event name.

Regards,
Alexei

2020-12-07 17:14:46

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf tools: Allow to list events via control file

Hi,

On 07.12.2020 19:28, Arnaldo Carvalho de Melo wrote:
> Em Sun, Dec 06, 2020 at 06:05:19PM +0100, Jiri Olsa escreveu:
>> Adding new control event to display all evlist events.
>>
>> The interface string for control file is 'list'. When
>> received, perf will scan and print current evlist into
>> perf record terminal.
>>
>> Example session:
>>
>> terminal 1:
>> # mkfifo control ack perf.pipe
>> # perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
>> Events disabled
>>
>> terminal 2:
>> # echo list > control
>>
>> terminal 1:
>> # perf record --control=fifo:control,ack -D -1 --no-buffering -e 'sched:*' -o - > perf.pipe
>> ...
>> sched:sched_kthread_stop
>> sched:sched_kthread_stop_ret
>> sched:sched_waking
>> sched:sched_wakeup
>> sched:sched_wakeup_new
>> sched:sched_switch
>> sched:sched_migrate_task
>> sched:sched_process_free
>> sched:sched_process_exit
>> sched:sched_wait_task
>> sched:sched_process_wait
>> sched:sched_process_fork
>> sched:sched_process_exec
>> sched:sched_stat_wait
>> sched:sched_stat_sleep
>> sched:sched_stat_iowait
>> sched:sched_stat_blocked
>> sched:sched_stat_runtime
>> sched:sched_pi_setprio
>> sched:sched_move_numa
>> sched:sched_stick_numa
>> sched:sched_swap_numa
>> sched:sched_wake_idle_without_ipi
>> dummy:HG
>>
>> This new command is handy to get real event names when
>> wildcards are used.
>
> Ok, would be nice to have a verbose mode like:
>
> [acme@five ~]$ sudo ~acme/bin/perf record -e 'sched:*' sleep 0.001
> [ perf record: Woken up 14 times to write data ]
> [ perf record: Captured and wrote 0.023 MB perf.data (16 samples) ]
> [acme@five ~]$ sudo ~acme/bin/perf evlist
> sched:sched_kthread_stop
> sched:sched_kthread_stop_ret
> sched:sched_waking
> sched:sched_wakeup
> sched:sched_wakeup_new
> sched:sched_switch
> sched:sched_migrate_task
> sched:sched_process_free
> sched:sched_process_exit
> sched:sched_wait_task
> sched:sched_process_wait
> sched:sched_process_fork
> sched:sched_process_exec
> sched:sched_stat_wait
> sched:sched_stat_sleep
> sched:sched_stat_iowait
> sched:sched_stat_blocked
> sched:sched_stat_runtime
> sched:sched_pi_setprio
> sched:sched_move_numa
> sched:sched_stick_numa
> sched:sched_swap_numa
> sched:sched_wake_idle_without_ipi
> # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
> [acme@five ~]$ sudo ~acme/bin/perf evlist -v
> sched:sched_kthread_stop: type: 2, size: 120, config: 0x13f, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1
> sched:sched_kthread_stop_ret: type: 2, size: 120, config: 0x13e, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_waking: type: 2, size: 120, config: 0x13d, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_wakeup: type: 2, size: 120, config: 0x13c, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_wakeup_new: type: 2, size: 120, config: 0x13b, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_switch: type: 2, size: 120, config: 0x13a, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_migrate_task: type: 2, size: 120, config: 0x139, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_process_free: type: 2, size: 120, config: 0x138, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_process_exit: type: 2, size: 120, config: 0x137, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_wait_task: type: 2, size: 120, config: 0x136, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_process_wait: type: 2, size: 120, config: 0x135, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_process_fork: type: 2, size: 120, config: 0x134, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_process_exec: type: 2, size: 120, config: 0x133, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_stat_wait: type: 2, size: 120, config: 0x132, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_stat_sleep: type: 2, size: 120, config: 0x131, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_stat_iowait: type: 2, size: 120, config: 0x130, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_stat_blocked: type: 2, size: 120, config: 0x12f, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_stat_runtime: type: 2, size: 120, config: 0x12e, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_pi_setprio: type: 2, size: 120, config: 0x12d, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_move_numa: type: 2, size: 120, config: 0x12c, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_stick_numa: type: 2, size: 120, config: 0x12b, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_swap_numa: type: 2, size: 120, config: 0x12a, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> sched:sched_wake_idle_without_ipi: type: 2, size: 120, config: 0x129, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
> [acme@five ~]$
>
> Also I think we should use 'evlist' instead of 'list', to be consistent.

FWIW,

Or may be even name the command starting with a verb like 'list_events'

Thanks,
Alexei

>
> Applied 1/3 and 2/3.
>
> - Arnaldo
>
>> Signed-off-by: Jiri Olsa <[email protected]>
>> ---
>> tools/perf/builtin-record.c | 1 +
>> tools/perf/builtin-stat.c | 1 +
>> tools/perf/util/evlist.c | 6 ++++++
>> tools/perf/util/evlist.h | 2 ++
>> 4 files changed, 10 insertions(+)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index 582b8fba012c..f620ed056c89 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -1951,6 +1951,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>> case EVLIST_CTL_CMD_UNSUPPORTED:
>> case EVLIST_CTL_CMD_ENABLE_EVSEL:
>> case EVLIST_CTL_CMD_DISABLE_EVSEL:
>> + case EVLIST_CTL_CMD_LIST:
>> default:
>> break;
>> }
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 6a21fb665008..56f2206b5991 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -592,6 +592,7 @@ static void process_evlist(struct evlist *evlist, unsigned int interval)
>> case EVLIST_CTL_CMD_UNSUPPORTED:
>> case EVLIST_CTL_CMD_ENABLE_EVSEL:
>> case EVLIST_CTL_CMD_DISABLE_EVSEL:
>> + case EVLIST_CTL_CMD_LIST:
>> default:
>> break;
>> }
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 05723227bebf..c05476ca2ff4 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -1931,6 +1931,9 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
>> (sizeof(EVLIST_CTL_CMD_SNAPSHOT_TAG)-1))) {
>> *cmd = EVLIST_CTL_CMD_SNAPSHOT;
>> pr_debug("is snapshot\n");
>> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_LIST_TAG,
>> + (sizeof(EVLIST_CTL_CMD_LIST_TAG)-1))) {
>> + *cmd = EVLIST_CTL_CMD_LIST;
>> }
>> }
>>
>> @@ -1995,6 +1998,9 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>> pr_info("failed: can't find %s event\n", evsel_name);
>> }
>> break;
>> + case EVLIST_CTL_CMD_LIST:
>> + evlist__for_each_entry(evlist, evsel)
>> + pr_info("%s\n", evsel__name(evsel));
>> case EVLIST_CTL_CMD_SNAPSHOT:
>> break;
>> case EVLIST_CTL_CMD_ACK:
>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>> index e4e8ff8831a3..6b8a9918fdb2 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -332,6 +332,7 @@ struct evsel *evlist__reset_weak_group(struct evlist *evlist, struct evsel *evse
>> #define EVLIST_CTL_CMD_SNAPSHOT_TAG "snapshot"
>> #define EVLIST_CTL_CMD_ENABLE_EVSEL_TAG "enable-"
>> #define EVLIST_CTL_CMD_DISABLE_EVSEL_TAG "disable-"
>> +#define EVLIST_CTL_CMD_LIST_TAG "list"
>>
>> #define EVLIST_CTL_CMD_MAX_LEN 64
>>
>> @@ -343,6 +344,7 @@ enum evlist_ctl_cmd {
>> EVLIST_CTL_CMD_DISABLE_EVSEL,
>> EVLIST_CTL_CMD_ACK,
>> EVLIST_CTL_CMD_SNAPSHOT,
>> + EVLIST_CTL_CMD_LIST,
>> };
>>
>> int evlist__parse_control(const char *str, int *ctl_fd, int *ctl_fd_ack, bool *ctl_fd_close);
>> --
>> 2.26.2
>>
>

2020-12-07 17:16:16

[permalink] [raw]

Subject: Re: [PATCH 1/3] perf tools: Add evlist__disable_evsel/evlist__enable_evsel

Hi,

On 06.12.2020 20:05, Jiri Olsa wrote:
> Adding interface to enable/disable single event in the
> evlist based on its name. It will be used later in new
> control enable/disable interface.
>
> Keeping the evlist::enabled true when one or more events
> are enabled so the toggle can work properly and toggle
> evlist to disabled state.
>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/util/evlist.c | 69 ++++++++++++++++++++++++++++++++++++++--
> tools/perf/util/evlist.h | 2 ++
> 2 files changed, 68 insertions(+), 3 deletions(-)

Acked-by: Alexei Budankov <[email protected]>

Regards,
Alexei

2020-12-07 19:56:09

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf tools: Allow to list events via control file

On Mon, Dec 07, 2020 at 01:28:06PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > sched:sched_stick_numa
> > sched:sched_swap_numa
> > sched:sched_wake_idle_without_ipi
> > dummy:HG
> >
> > This new command is handy to get real event names when
> > wildcards are used.
>
> Ok, would be nice to have a verbose mode like:
>
> [acme@five ~]$ sudo ~acme/bin/perf record -e 'sched:*' sleep 0.001
> [ perf record: Woken up 14 times to write data ]
> [ perf record: Captured and wrote 0.023 MB perf.data (16 samples) ]
> [acme@five ~]$ sudo ~acme/bin/perf evlist
> sched:sched_kthread_stop
> sched:sched_kthread_stop_ret
> sched:sched_waking
> sched:sched_wakeup
> sched:sched_wakeup_new
> sched:sched_switch
> sched:sched_migrate_task
> sched:sched_process_free
> sched:sched_process_exit
> sched:sched_wait_task
> sched:sched_process_wait
> sched:sched_process_fork
> sched:sched_process_exec
> sched:sched_stat_wait
> sched:sched_stat_sleep
> sched:sched_stat_iowait
> sched:sched_stat_blocked
> sched:sched_stat_runtime
> sched:sched_pi_setprio
> sched:sched_move_numa
> sched:sched_stick_numa
> sched:sched_swap_numa
> sched:sched_wake_idle_without_ipi
> # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
> [acme@five ~]$ sudo ~acme/bin/perf evlist -v
> sched:sched_kthread_stop: type: 2, size: 120, config: 0x13f, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1

yea, hopefuly we won't be surprised what's enabled live in evsel ;-)

SNIP

> sched:sched_wake_idle_without_ipi: type: 2, size: 120, config: 0x129, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
> [acme@five ~]$
>
> Also I think we should use 'evlist' instead of 'list', to be consistent.

ok, makes sense

thanks,
jirka

2020-12-07 20:00:09

[permalink] [raw]

Subject: Re: [PATCH 3/3] perf tools: Allow to list events via control file

On Mon, Dec 07, 2020 at 08:09:06PM +0300, Alexei Budankov wrote:

SNIP

> > sched:sched_move_numa: type: 2, size: 120, config: 0x12c, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> > sched:sched_stick_numa: type: 2, size: 120, config: 0x12b, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> > sched:sched_swap_numa: type: 2, size: 120, config: 0x12a, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> > sched:sched_wake_idle_without_ipi: type: 2, size: 120, config: 0x129, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|ID|CPU|PERIOD|RAW, read_format: ID, disabled: 1, inherit: 1, enable_on_exec: 1, sample_id_all: 1, exclude_guest: 1
> > # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
> > [acme@five ~]$
> >
> > Also I think we should use 'evlist' instead of 'list', to be consistent.
>
> FWIW,
>
> Or may be even name the command starting with a verb like 'list_events'

I think evlist is better because there's 'perf evlist' command

thanks,
jirka

2020-12-10 16:29:16

[permalink] [raw]

Subject: Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

On Mon, Dec 07, 2020 at 08:02:20PM +0300, Alexei Budankov wrote:
> Hi,
>
> On 06.12.2020 20:05, Jiri Olsa wrote:
> > Adding new control events to enable/disable specific event.
> > The interface string for control file are:
> >
> > 'enable-<EVENT NAME>'
> > 'disable-<EVENT NAME>'
>
> <SNIP>
>
> >
> > when received the command, perf will scan the current evlist
> > for <EVENT NAME> and if found it's enabled/disabled.
>
> <SNIP>
>
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 70aff26612a9..05723227bebf 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
> > bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
> >
> > if (bytes_read > 0) {
> > - if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> > + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
> > + (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
> > + *cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
> > + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
> > + (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
> > + *cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
> > + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> > (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
> > *cmd = EVLIST_CTL_CMD_ENABLE;
> > } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
> > @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> > char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
> > int ctlfd_pos = evlist->ctl_fd.pos;
> > struct pollfd *entries = evlist->core.pollfd.entries;
> > + struct evsel *evsel;
> > + char *evsel_name;
> >
> > if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
> > return 0;
> > @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> > case EVLIST_CTL_CMD_DISABLE:
> > evlist__disable(evlist);
> > break;
> > + case EVLIST_CTL_CMD_ENABLE_EVSEL:
> > + evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;
>
> It makes sense to check that evsel_name still points
> into cmd_data buffer after assigning to event name.

right, will add that

thanks,
jirka

2020-12-10 17:39:59

[permalink] [raw]

Subject: Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

Em Thu, Dec 10, 2020 at 05:24:30PM +0100, Jiri Olsa escreveu:
> On Mon, Dec 07, 2020 at 08:02:20PM +0300, Alexei Budankov wrote:
> > Hi,
> >
> > On 06.12.2020 20:05, Jiri Olsa wrote:
> > > Adding new control events to enable/disable specific event.
> > > The interface string for control file are:
> > >
> > > 'enable-<EVENT NAME>'
> > > 'disable-<EVENT NAME>'
> >
> > <SNIP>
> >
> > >
> > > when received the command, perf will scan the current evlist
> > > for <EVENT NAME> and if found it's enabled/disabled.
> >
> > <SNIP>
> >
> > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > index 70aff26612a9..05723227bebf 100644
> > > --- a/tools/perf/util/evlist.c
> > > +++ b/tools/perf/util/evlist.c
> > > @@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
> > > bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
> > >
> > > if (bytes_read > 0) {
> > > - if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> > > + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
> > > + (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
> > > + *cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
> > > + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
> > > + (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
> > > + *cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
> > > + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> > > (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
> > > *cmd = EVLIST_CTL_CMD_ENABLE;
> > > } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
> > > @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> > > char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
> > > int ctlfd_pos = evlist->ctl_fd.pos;
> > > struct pollfd *entries = evlist->core.pollfd.entries;
> > > + struct evsel *evsel;
> > > + char *evsel_name;
> > >
> > > if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
> > > return 0;
> > > @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> > > case EVLIST_CTL_CMD_DISABLE:
> > > evlist__disable(evlist);
> > > break;
> > > + case EVLIST_CTL_CMD_ENABLE_EVSEL:
> > > + evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;
> >
> > It makes sense to check that evsel_name still points
> > into cmd_data buffer after assigning to event name.
>
> right, will add that

So, I'm finishing test builds, so probably I'll push the first two
patches publicly and then you can send a patch on top of this, ok?

Unless the tests break somewhere and then I'll have to restart, so I'll
fold in whatever gets at that time...

- Arnaldo

2020-12-10 18:11:10

[permalink] [raw]

Subject: Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

On Thu, Dec 10, 2020 at 05:24:30PM +0100, Jiri Olsa wrote:
> On Mon, Dec 07, 2020 at 08:02:20PM +0300, Alexei Budankov wrote:
> > Hi,
> >
> > On 06.12.2020 20:05, Jiri Olsa wrote:
> > > Adding new control events to enable/disable specific event.
> > > The interface string for control file are:
> > >
> > > 'enable-<EVENT NAME>'
> > > 'disable-<EVENT NAME>'
> >
> > <SNIP>
> >
> > >
> > > when received the command, perf will scan the current evlist
> > > for <EVENT NAME> and if found it's enabled/disabled.
> >
> > <SNIP>
> >
> > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > index 70aff26612a9..05723227bebf 100644
> > > --- a/tools/perf/util/evlist.c
> > > +++ b/tools/perf/util/evlist.c
> > > @@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
> > > bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
> > >
> > > if (bytes_read > 0) {
> > > - if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> > > + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
> > > + (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
> > > + *cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
> > > + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
> > > + (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
> > > + *cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
> > > + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> > > (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
> > > *cmd = EVLIST_CTL_CMD_ENABLE;
> > > } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
> > > @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> > > char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
> > > int ctlfd_pos = evlist->ctl_fd.pos;
> > > struct pollfd *entries = evlist->core.pollfd.entries;
> > > + struct evsel *evsel;
> > > + char *evsel_name;
> > >
> > > if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
> > > return 0;
> > > @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> > > case EVLIST_CTL_CMD_DISABLE:
> > > evlist__disable(evlist);
> > > break;
> > > + case EVLIST_CTL_CMD_ENABLE_EVSEL:
> > > + evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;
> >
> > It makes sense to check that evsel_name still points
> > into cmd_data buffer after assigning to event name.
>
> right, will add that

actualy it's already checked in evlist__ctlfd_recv, evsel_name at
worst will be empty string so evlist__find_evsel_by_str will fail

I'll add '' around %s in the error output string:

failed: can't find '%s' event

so it's obvious when it's empty

jirka

2020-12-10 18:25:39

[permalink] [raw]

Subject: Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

On 10.12.2020 21:06, Jiri Olsa wrote:
> On Thu, Dec 10, 2020 at 05:24:30PM +0100, Jiri Olsa wrote:
>> On Mon, Dec 07, 2020 at 08:02:20PM +0300, Alexei Budankov wrote:
>>> Hi,
>>>
>>> On 06.12.2020 20:05, Jiri Olsa wrote:
>>>> Adding new control events to enable/disable specific event.
>>>> The interface string for control file are:
>>>>
>>>> 'enable-<EVENT NAME>'
>>>> 'disable-<EVENT NAME>'
>>>
>>> <SNIP>
>>>
>>>>
>>>> when received the command, perf will scan the current evlist
>>>> for <EVENT NAME> and if found it's enabled/disabled.
>>>
>>> <SNIP>
>>>
>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>> index 70aff26612a9..05723227bebf 100644
>>>> --- a/tools/perf/util/evlist.c
>>>> +++ b/tools/perf/util/evlist.c
>>>> @@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
>>>> bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
>>>>
>>>> if (bytes_read > 0) {
>>>> - if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
>>>> + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
>>>> + (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
>>>> + *cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
>>>> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
>>>> + (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
>>>> + *cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
>>>> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
>>>> (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
>>>> *cmd = EVLIST_CTL_CMD_ENABLE;
>>>> } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
>>>> @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>>>> char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
>>>> int ctlfd_pos = evlist->ctl_fd.pos;
>>>> struct pollfd *entries = evlist->core.pollfd.entries;
>>>> + struct evsel *evsel;
>>>> + char *evsel_name;
>>>>
>>>> if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
>>>> return 0;
>>>> @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>>>> case EVLIST_CTL_CMD_DISABLE:
>>>> evlist__disable(evlist);
>>>> break;
>>>> + case EVLIST_CTL_CMD_ENABLE_EVSEL:
>>>> + evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;
>>>
>>> It makes sense to check that evsel_name still points
>>> into cmd_data buffer after assigning to event name.
>>
>> right, will add that
>
> actualy it's already checked in evlist__ctlfd_recv, evsel_name at
> worst will be empty string so evlist__find_evsel_by_str will fail
>
> I'll add '' around %s in the error output string:
>
> failed: can't find '%s' event
>
> so it's obvious when it's empty

Looks good to me. Thanks!

Alexei

>
> jirka
>
> .
>

2020-12-10 18:32:02

[permalink] [raw]

Subject: Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

Em Thu, Dec 10, 2020 at 09:20:42PM +0300, Alexei Budankov escreveu:
>
> On 10.12.2020 21:06, Jiri Olsa wrote:
> > On Thu, Dec 10, 2020 at 05:24:30PM +0100, Jiri Olsa wrote:
> >> On Mon, Dec 07, 2020 at 08:02:20PM +0300, Alexei Budankov wrote:
> >>> Hi,
> >>>
> >>> On 06.12.2020 20:05, Jiri Olsa wrote:
> >>>> Adding new control events to enable/disable specific event.
> >>>> The interface string for control file are:
> >>>>
> >>>> 'enable-<EVENT NAME>'
> >>>> 'disable-<EVENT NAME>'
> >>>
> >>> <SNIP>
> >>>
> >>>>
> >>>> when received the command, perf will scan the current evlist
> >>>> for <EVENT NAME> and if found it's enabled/disabled.
> >>>
> >>> <SNIP>
> >>>
> >>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>> index 70aff26612a9..05723227bebf 100644
> >>>> --- a/tools/perf/util/evlist.c
> >>>> +++ b/tools/perf/util/evlist.c
> >>>> @@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
> >>>> bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
> >>>>
> >>>> if (bytes_read > 0) {
> >>>> - if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> >>>> + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
> >>>> + (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
> >>>> + *cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
> >>>> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
> >>>> + (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
> >>>> + *cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
> >>>> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
> >>>> (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
> >>>> *cmd = EVLIST_CTL_CMD_ENABLE;
> >>>> } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
> >>>> @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> >>>> char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
> >>>> int ctlfd_pos = evlist->ctl_fd.pos;
> >>>> struct pollfd *entries = evlist->core.pollfd.entries;
> >>>> + struct evsel *evsel;
> >>>> + char *evsel_name;
> >>>>
> >>>> if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
> >>>> return 0;
> >>>> @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> >>>> case EVLIST_CTL_CMD_DISABLE:
> >>>> evlist__disable(evlist);
> >>>> break;
> >>>> + case EVLIST_CTL_CMD_ENABLE_EVSEL:
> >>>> + evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;
> >>>
> >>> It makes sense to check that evsel_name still points
> >>> into cmd_data buffer after assigning to event name.
> >>
> >> right, will add that
> >
> > actualy it's already checked in evlist__ctlfd_recv, evsel_name at
> > worst will be empty string so evlist__find_evsel_by_str will fail
> >
> > I'll add '' around %s in the error output string:
> >
> > failed: can't find '%s' event
> >
> > so it's obvious when it's empty
>
> Looks good to me. Thanks!

I'm taking this as an Acked-by: you as per
Documentation/process/submitting-patches.rst:

<quoted>
Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
has at least reviewed the patch and has indicated acceptance. Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by: (but note that it is usually better to ask for an
explicit ack).
</>

Thanks,

- Arnaldo

2020-12-10 18:36:54

[permalink] [raw]

Subject: Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

On 10.12.2020 21:20, Alexei Budankov wrote:
>
> On 10.12.2020 21:06, Jiri Olsa wrote:
>> On Thu, Dec 10, 2020 at 05:24:30PM +0100, Jiri Olsa wrote:
>>> On Mon, Dec 07, 2020 at 08:02:20PM +0300, Alexei Budankov wrote:
>>>> Hi,
>>>>
>>>> On 06.12.2020 20:05, Jiri Olsa wrote:
>>>>> Adding new control events to enable/disable specific event.
>>>>> The interface string for control file are:
>>>>>
>>>>> 'enable-<EVENT NAME>'
>>>>> 'disable-<EVENT NAME>'
>>>>
>>>> <SNIP>
>>>>
>>>>>
>>>>> when received the command, perf will scan the current evlist
>>>>> for <EVENT NAME> and if found it's enabled/disabled.
>>>>
>>>> <SNIP>
>>>>
>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>> index 70aff26612a9..05723227bebf 100644
>>>>> --- a/tools/perf/util/evlist.c
>>>>> +++ b/tools/perf/util/evlist.c
>>>>> @@ -1915,7 +1915,13 @@ static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
>>>>> bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
>>>>>
>>>>> if (bytes_read > 0) {
>>>>> - if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
>>>>> + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_EVSEL_TAG,
>>>>> + (sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG)-1))) {
>>>>> + *cmd = EVLIST_CTL_CMD_ENABLE_EVSEL;
>>>>> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_EVSEL_TAG,
>>>>> + (sizeof(EVLIST_CTL_CMD_DISABLE_EVSEL_TAG)-1))) {
>>>>> + *cmd = EVLIST_CTL_CMD_DISABLE_EVSEL;
>>>>> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
>>>>> (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) {
>>>>> *cmd = EVLIST_CTL_CMD_ENABLE;
>>>>> } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
>>>>> @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>>>>> char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
>>>>> int ctlfd_pos = evlist->ctl_fd.pos;
>>>>> struct pollfd *entries = evlist->core.pollfd.entries;
>>>>> + struct evsel *evsel;
>>>>> + char *evsel_name;
>>>>>
>>>>> if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
>>>>> return 0;
>>>>> @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>>>>> case EVLIST_CTL_CMD_DISABLE:
>>>>> evlist__disable(evlist);
>>>>> break;
>>>>> + case EVLIST_CTL_CMD_ENABLE_EVSEL:
>>>>> + evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;
>>>>
>>>> It makes sense to check that evsel_name still points
>>>> into cmd_data buffer after assigning to event name.
>>>
>>> right, will add that
>>
>> actualy it's already checked in evlist__ctlfd_recv, evsel_name at
>> worst will be empty string so evlist__find_evsel_by_str will fail
>>
>> I'll add '' around %s in the error output string:
>>
>> failed: can't find '%s' event
>>
>> so it's obvious when it's empty

Acked-by: Alexei Budankov <[email protected]>

Thanks,
Alexei

2020-12-11 20:52:37

[permalink] [raw]

Subject: Re: [PATCH 2/3] perf tools: Allow to enable/disable events via control file

Em Thu, Dec 10, 2020 at 02:15:03PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Dec 10, 2020 at 05:24:30PM +0100, Jiri Olsa escreveu:
> > On Mon, Dec 07, 2020 at 08:02:20PM +0300, Alexei Budankov wrote:
> > > On 06.12.2020 20:05, Jiri Olsa wrote:
> > > > @@ -1952,6 +1958,8 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> > > > char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
> > > > int ctlfd_pos = evlist->ctl_fd.pos;
> > > > struct pollfd *entries = evlist->core.pollfd.entries;
> > > > + struct evsel *evsel;
> > > > + char *evsel_name;
> > > >
> > > > if (!evlist__ctlfd_initialized(evlist) || !entries[ctlfd_pos].revents)
> > > > return 0;
> > > > @@ -1967,6 +1975,26 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> > > > case EVLIST_CTL_CMD_DISABLE:
> > > > evlist__disable(evlist);
> > > > break;
> > > > + case EVLIST_CTL_CMD_ENABLE_EVSEL:
> > > > + evsel_name = cmd_data + sizeof(EVLIST_CTL_CMD_ENABLE_EVSEL_TAG) - 1;

> > > It makes sense to check that evsel_name still points
> > > into cmd_data buffer after assigning to event name.

> > right, will add that

> So, I'm finishing test builds, so probably I'll push the first two
> patches publicly and then you can send a patch on top of this, ok?

> Unless the tests break somewhere and then I'll have to restart, so I'll
> fold in whatever gets at that time...

That was quick:

[perfbuilder@five ~]$ time dm
Thu Dec 10 01:44:52 PM -03 2020
# export PERF_TARBALL=http://192.168.86.5/perf/perf-5.10.0-rc6.tar.xz
# dm
1 78.47 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final)
2 104.38 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final)
3 105.64 alpine:3.6 : Ok gcc (Alpine 6.3.0) 6.3.0, clang version 4.0.0 (tags/RELEASE_400/final)
4 96.52 alpine:3.7 : Ok gcc (Alpine 6.4.0) 6.4.0, Alpine clang version 5.0.0 (tags/RELEASE_500/final) (based on LLVM 5.0.0)
5 81.08 alpine:3.8 : Ok gcc (Alpine 6.4.0) 6.4.0, Alpine clang version 5.0.1 (tags/RELEASE_501/final) (based on LLVM 5.0.1)
6 85.20 alpine:3.9 : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 5.0.1 (tags/RELEASE_502/final) (based on LLVM 5.0.1)
7 106.10 alpine:3.10 : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 8.0.0 (tags/RELEASE_800/final) (based on LLVM 8.0.0)
8 120.20 alpine:3.11 : Ok gcc (Alpine 9.3.0) 9.3.0, Alpine clang version 9.0.0 (https://git.alpinelinux.org/aports f7f0d2c2b8bcd6a5843401a9a702029556492689) (based on LLVM 9.0.0)
9 109.99 alpine:3.12 : Ok gcc (Alpine 9.3.0) 9.3.0, Alpine clang version 10.0.0 (https://gitlab.alpinelinux.org/alpine/aports.git 7445adce501f8473efdb93b17b5eaf2f1445ed4c)
10 117.76 alpine:edge : Ok gcc (Alpine 10.2.0) 10.2.0, Alpine clang version 10.0.1
11 69.15 alt:p8 : Ok x86_64-alt-linux-gcc (GCC) 5.3.1 20151207 (ALT p8 5.3.1-alt3.M80P.1), clang version 3.8.0 (tags/RELEASE_380/final)
12 84.55 alt:p9 : Ok x86_64-alt-linux-gcc (GCC) 8.4.1 20200305 (ALT p9 8.4.1-alt0.p9.1), clang version 10.0.0
13 82.69 alt:sisyphus : Ok x86_64-alt-linux-gcc (GCC) 9.3.1 20200518 (ALT Sisyphus 9.3.1-alt1), clang version 10.0.1
14 66.22 amazonlinux:1 : Ok gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2), clang version 3.6.2 (tags/RELEASE_362/final)
15 100.16 amazonlinux:2 : Ok gcc (GCC) 7.3.1 20180712 (Red Hat 7.3.1-9), clang version 7.0.1 (Amazon Linux 2 7.0.1-1.amzn2.0.2)
16 11.20 android-ndk:r12b-arm : FAIL arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease)
17 11.90 android-ndk:r15c-arm : FAIL arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease)
18 26.33 centos:6 : Ok gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23)
19 31.80 centos:7 : Ok gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)
20 116.02 centos:8 : Ok gcc (GCC) 8.3.1 20191121 (Red Hat 8.3.1-5), clang version 9.0.1 (Red Hat 9.0.1-2.module_el8.2.0+309+0c7b6b03)
21 62.27 clearlinux:latest : Ok gcc (Clear Linux OS for Intel Architecture) 10.2.1 20201106 releases/gcc-10.2.0-475-g099857318c, clang version 10.0.1
22 77.82 debian:8 : Ok gcc (Debian 4.9.2-10+deb8u2) 4.9.2, Debian clang version 3.5.0-10 (tags/RELEASE_350/final) (based on LLVM 3.5.0)
23: debian:9

There I go...

[perfbuilder@five ~]$ tail -20 dm.log/android-ndk\:r12b-arm
FLEX /tmp/build/perf/util/expr-flex.c
CC /tmp/build/perf/util/pmu-bison.o
CC /tmp/build/perf/util/expr-bison.o
CC /tmp/build/perf/util/expr-flex.o
CC /tmp/build/perf/util/expr.o
CC /tmp/build/perf/util/parse-events.o
CC /tmp/build/perf/util/parse-events-flex.o
CC /tmp/build/perf/util/pmu.o
CC /tmp/build/perf/util/pmu-flex.o
LD /tmp/build/perf/util/intel-pt-decoder/perf-in.o
LD /tmp/build/perf/util/perf-in.o
LD /tmp/build/perf/perf-in.o
LINK /tmp/build/perf/perf
/tmp/build/perf/perf-in.o:hist.c:function pmu_for_each_sys_event: error: undefined reference to 'pmu_sys_event_tables'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile.perf:659: /tmp/build/perf/perf] Error 1
make[1]: *** [Makefile.perf:233: sub-make] Error 2
make: *** [Makefile:70: all] Error 2
make: Leaving directory '/git/linux/tools/perf'
+ exit 1
[perfbuilder@five ~]

Now to figure out who generates this pmu_sys_event_tables, John?

- Arnaldo

2020-12-11 21:02:16