[PATCH v9 00/15] perf: support enable and disable commands in stat and record modes

[PATCH v9 00/15] perf: support enable and disable commands in stat and record modes

[Alexey Budankov]

Changes in v9:
- avoided screwing of fds by fdarray__filter() to fix staleness of pos returned by fdarray__add()
- implemented notion of properties for fds kept by fdarray object and introduced nonfilterable
  property for control fd
- avoided counting of nonfilterable fds by fdarray__filter()
- replaced strlen() with sizeof(str)-1 for CMD_TAGS
- avoided mixing timeout and interval processing in one function
- renamed child to child_exited variable
- processed time_to_sleep < time_diff condition
- placed evlist__ctlfd_process() call after evlist__poll() and evlist__filter_pollfd()
  calls in record mode

v8: https://lore.kernel.org/lkml/0781a077-aa82-5b4a-273e-c17372a72b93@linux.intel.com/

Changes in v8:
- avoided moving of fds at fdarray__filter() call
- skipped counting of fds with zeroed revents at fdarray__filter() call
- converted explicit --ctl-fd[-ack] into --control fd:ctl-fd[,ack-fd option 
- updated docs to accommodate --control fd:ctl-fd[,ack-fd] option

v7: https://lore.kernel.org/lkml/5de4b954-24f0-1e8d-5a0d-7b12783b8218@linux.intel.com/

Changes in v7:
- added missing perf-record.txt changes 
- adjusted docs wording for --ctl-fd,ctl-fd-ack options 
  to additionally mention --delay=-1 effect

v6: https://lore.kernel.org/lkml/f8e3a714-d9b1-4647-e1d2-9981cbaa83ec@linux.intel.com/

Changes in v6:
- split re-factoring of events handling loops for stat mode
  into smaller incremental parts
- added parts missing at v5
- corrected v5 runtime issues

v5: https://lore.kernel.org/lkml/e5cac8dd-7aa4-ec7c-671c-07756907acba@linux.intel.com/

Changes in v5:
- split re-factoring of events handling loops for stat mode
  into smaller incremental parts

v4: https://lore.kernel.org/lkml/653fe5f3-c986-a841-1ed8-0a7d2fa24c00@linux.intel.com/

Changes in v4:
- made checking of ctlfd state unconditional in record trace streaming loop
- introduced static poll fds to keep evlist__filter_pollfd() unaffected
- handled ret code of evlist__initialize_ctlfd() where need
- renamed and structured handle_events() function
- applied anonymous structs where needed

v3: https://lore.kernel.org/lkml/eb38e9e5-754f-d410-1d9b-e26b702d51b7@linux.intel.com/

Changes in v3:
- renamed functions and types from perf_evlist_ to evlist_ to avoid
  clash with libperf code;
- extended commands to be strings of variable length consisting of
  command name and also possibly including command specific data;
- merged docs update with the code changes;
- updated docs for -D,--delay=-1 option for stat and record modes;

v2: https://lore.kernel.org/lkml/d582cc3d-2302-c7e2-70d3-bc7ab6f628c3@linux.intel.com/

Changes in v2:
- renamed resume and pause commands to enable and disable ones, renamed
  CTL_CMD_RESUME and CTL_CMD_PAUSE to CTL_CMD_ENABLE and CTL_CMD_DISABLE
  to fit to the appropriate ioctls and avoid mixing up with PAUSE_OUTPUT
  ioctl;
- factored out event handling loop into a handle_events() for stat mode;
- separated -D,--delay=-1 into separate patches for stat and record modes;

v1: https://lore.kernel.org/lkml/825a5132-b58d-c0b6-b050-5a6040386ec7@linux.intel.com/

repo: tip of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/core

The patch set implements handling of 'start disabled', 'enable' and 'disable'
external control commands which can be provided for stat and record modes
of the tool from an external controlling process. 'start disabled' command
can be used to postpone enabling of events in the beginning of a monitoring
session. 'enable' and 'disable' commands can be used to enable and disable
events correspondingly any time after the start of the session.

The 'start disabled', 'enable' and 'disable' external control commands can be
used to focus measurement on specially selected time intervals of workload
execution. Focused measurement reduces tool intrusion and influence on
workload behavior, reduces distortion and amount of collected and stored
data, mitigates data accuracy loss because measurement and data capturing
happen only during intervals of interest.

A controlling process can be a bash shell script [1], native executable or
any other language program that can directly work with file descriptors,
e.g. pipes [2], and spawn a process, specially the tool one.

-D,--delay <val> option is extended with -1 value to skip events enabling
in the beginning of a monitoring session ('start disabled' command).
--control fd:ctl-fd[,ack-fd] command line option is introduced to provide the
tool with a pair of file descriptors to listen to control commands and reply
to the controlling process on the completion of received commands.

The tool reads control command message from ctl-fd descriptor, handles the
command and optionally replies acknowledgement message to ack-fd descriptor,
if it is specified on the command line. 'enable' command is recognized as
'enable' string message and 'disable' command is recognized as 'disable'
string message both received from ctl-fd descriptor. Completion message is
'ack\n' and sent to ack-fd descriptor.

Example bash script demonstrating simple use case follows:

#!/bin/bash

ctl_dir=/tmp/

ctl_fifo=${ctl_dir}perf_ctl.fifo
test -p ${ctl_fifo} && unlink ${ctl_fifo}
mkfifo ${ctl_fifo} && exec {ctl_fd}<>${ctl_fifo}

ctl_ack_fifo=${ctl_dir}perf_ctl_ack.fifo
test -p ${ctl_ack_fifo} && unlink ${ctl_ack_fifo}
mkfifo ${ctl_ack_fifo} && exec {ctl_fd_ack}<>${ctl_ack_fifo}

perf stat -D -1 -e cpu-cycles -a -I 1000       \
          --control fd:${ctl_fd},${ctl_fd_ack} \
          -- sleep 40 &
perf_pid=$!

sleep 5  && echo 'enable' >&${ctl_fd} && read -u ${ctl_fd_ack} e1 && echo "enabled(${e1})"
sleep 10 && echo 'disable' >&${ctl_fd} && read -u ${ctl_fd_ack} d1 && echo "disabled(${d1})"
sleep 5  && echo 'enable' >&${ctl_fd} && read -u ${ctl_fd_ack} e2 && echo "enabled(${e2})"
sleep 10 && echo 'disable' >&${ctl_fd} && read -u ${ctl_fd_ack} d2 && echo "disabled(${d2})"

exec {ctl_fd_ack}>&- && unlink ${ctl_ack_fifo}
exec {ctl_fd}>&- && unlink ${ctl_fifo}

wait -n ${perf_pid}
exit $?


Script output:

[root@host dir] example
Events disabled
#           time             counts unit events
     1.001101062      <not counted>      cpu-cycles                                                  
     2.002994944      <not counted>      cpu-cycles                                                  
     3.004864340      <not counted>      cpu-cycles                                                  
     4.006727177      <not counted>      cpu-cycles                                                  
Events enabled
enabled(ack)
     4.993808464          3,124,246      cpu-cycles                                                  
     5.008597004          3,325,624      cpu-cycles                                                  
     6.010387483         83,472,992      cpu-cycles                                                  
     7.012266598         55,877,621      cpu-cycles                                                  
     8.014175695         97,892,729      cpu-cycles                                                  
     9.016056093         68,461,242      cpu-cycles                                                  
    10.017937507         55,449,643      cpu-cycles                                                  
    11.019830154         68,938,167      cpu-cycles                                                  
    12.021719952         55,164,101      cpu-cycles                                                  
    13.023627550         70,535,720      cpu-cycles                                                  
    14.025580995         53,240,125      cpu-cycles                                                  
disabled(ack)
    14.997518260         53,558,068      cpu-cycles                                                  
Events disabled
    15.027216416      <not counted>      cpu-cycles                                                  
    16.029052729      <not counted>      cpu-cycles                                                  
    17.030904762      <not counted>      cpu-cycles                                                  
    18.032073424      <not counted>      cpu-cycles                                                  
    19.033805074      <not counted>      cpu-cycles                                                  
Events enabled
enabled(ack)
    20.001279097          3,021,022      cpu-cycles                                                  
    20.035044381          6,434,367      cpu-cycles                                                  
    21.036923813         89,358,251      cpu-cycles                                                  
    22.038825169         72,516,351      cpu-cycles                                                  
#           time             counts unit events
    23.040715596         55,046,157      cpu-cycles                                                  
    24.042643757         78,128,649      cpu-cycles                                                  
    25.044558535         61,052,428      cpu-cycles                                                  
    26.046452785         62,142,806      cpu-cycles                                                  
    27.048353021         74,477,971      cpu-cycles                                                  
    28.050241286         61,001,623      cpu-cycles                                                  
    29.052149961         61,653,502      cpu-cycles                                                  
disabled(ack)
    30.004980264         82,729,640      cpu-cycles                                                  
Events disabled
    30.053516176      <not counted>      cpu-cycles                                                  
    31.055348366      <not counted>      cpu-cycles                                                  
    32.057202097      <not counted>      cpu-cycles                                                  
    33.059040702      <not counted>      cpu-cycles                                                  
    34.060843288      <not counted>      cpu-cycles                                                  
    35.000888624      <not counted>      cpu-cycles                                                  
[root@host dir]# 

[1] http://man7.org/linux/man-pages/man1/bash.1.html
[2] http://man7.org/linux/man-pages/man2/pipe.2.html

---
Alexey Budankov (15):
  tools/libperf: avoid fds moving by fdarray__filter()
  tools/libperf: add properties to struct pollfd *entries objects
  tools/libperf: don't count nonfilterable fds by fdarray__filter()
  perf evlist: introduce control file descriptors
  perf evlist: implement control command handling functions
  perf stat: factor out body of event handling loop for system wide
  perf stat: move target check to loop control statement
  perf stat: factor out body of event handling loop for fork case
  perf stat: factor out event handling loop into dispatch_events()
  perf stat: extend -D,--delay option with -1 value
  perf stat: implement control commands handling
  perf stat: introduce --control fd:ctl-fd[,ack-fd] options
  perf record: extend -D,--delay option with -1 value
  perf record: implement control commands handling
  perf record: introduce --control fd:ctl-fd[,ack-fd] options

 tools/lib/api/fd/array.c                 |  31 ++--
 tools/lib/api/fd/array.h                 |  18 ++-
 tools/lib/perf/evlist.c                  |  10 +-
 tools/lib/perf/include/internal/evlist.h |   2 +-
 tools/perf/Documentation/perf-record.txt |  44 +++++-
 tools/perf/Documentation/perf-stat.txt   |  44 +++++-
 tools/perf/builtin-record.c              |  65 ++++++++-
 tools/perf/builtin-stat.c                | 174 ++++++++++++++++++-----
 tools/perf/builtin-trace.c               |   2 +-
 tools/perf/tests/fdarray.c               |  22 +--
 tools/perf/util/evlist.c                 | 139 +++++++++++++++++-
 tools/perf/util/evlist.h                 |  25 ++++
 tools/perf/util/record.h                 |   4 +-
 tools/perf/util/stat.h                   |   4 +-
 14 files changed, 489 insertions(+), 95 deletions(-)

-- 
2.24.1

[PATCH v9 01/15] tools/libperf: avoid fds moving by fdarray__filter()

[Alexey Budankov]

Avoid fds moving by fdarray__filter() so fds indices returned
by fdarray__add() can be used for access and processing of
objects at struct pollfd *entries.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/lib/api/fd/array.c   | 11 +++++------
 tools/perf/tests/fdarray.c | 20 ++------------------
 2 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
index 58d44d5eee31..89f9a2193c2d 100644
--- a/tools/lib/api/fd/array.c
+++ b/tools/lib/api/fd/array.c
@@ -93,22 +93,21 @@ int fdarray__filter(struct fdarray *fda, short revents,
 		return 0;
 
 	for (fd = 0; fd < fda->nr; ++fd) {
+		if (!fda->entries[fd].events)
+			continue;
+
 		if (fda->entries[fd].revents & revents) {
 			if (entry_destructor)
 				entry_destructor(fda, fd, arg);
 
+			fda->entries[fd].revents = fda->entries[fd].events = 0;
 			continue;
 		}
 
-		if (fd != nr) {
-			fda->entries[nr] = fda->entries[fd];
-			fda->priv[nr]	 = fda->priv[fd];
-		}
-
 		++nr;
 	}
 
-	return fda->nr = nr;
+	return nr;
 }
 
 int fdarray__poll(struct fdarray *fda, int timeout)
diff --git a/tools/perf/tests/fdarray.c b/tools/perf/tests/fdarray.c
index c7c81c4a5b2b..d0c8a05aab2f 100644
--- a/tools/perf/tests/fdarray.c
+++ b/tools/perf/tests/fdarray.c
@@ -12,6 +12,7 @@ static void fdarray__init_revents(struct fdarray *fda, short revents)
 
 	for (fd = 0; fd < fda->nr; ++fd) {
 		fda->entries[fd].fd	 = fda->nr - fd;
+		fda->entries[fd].events  = revents;
 		fda->entries[fd].revents = revents;
 	}
 }
@@ -29,7 +30,7 @@ static int fdarray__fprintf_prefix(struct fdarray *fda, const char *prefix, FILE
 
 int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_unused)
 {
-	int nr_fds, expected_fd[2], fd, err = TEST_FAIL;
+	int nr_fds, err = TEST_FAIL;
 	struct fdarray *fda = fdarray__new(5, 5);
 
 	if (fda == NULL) {
@@ -55,7 +56,6 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
 
 	fdarray__init_revents(fda, POLLHUP);
 	fda->entries[2].revents = POLLIN;
-	expected_fd[0] = fda->entries[2].fd;
 
 	pr_debug("\nfiltering all but fda->entries[2]:");
 	fdarray__fprintf_prefix(fda, "before", stderr);
@@ -66,17 +66,9 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
 		goto out_delete;
 	}
 
-	if (fda->entries[0].fd != expected_fd[0]) {
-		pr_debug("\nfda->entries[0].fd=%d != %d\n",
-			 fda->entries[0].fd, expected_fd[0]);
-		goto out_delete;
-	}
-
 	fdarray__init_revents(fda, POLLHUP);
 	fda->entries[0].revents = POLLIN;
-	expected_fd[0] = fda->entries[0].fd;
 	fda->entries[3].revents = POLLIN;
-	expected_fd[1] = fda->entries[3].fd;
 
 	pr_debug("\nfiltering all but (fda->entries[0], fda->entries[3]):");
 	fdarray__fprintf_prefix(fda, "before", stderr);
@@ -88,14 +80,6 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
 		goto out_delete;
 	}
 
-	for (fd = 0; fd < 2; ++fd) {
-		if (fda->entries[fd].fd != expected_fd[fd]) {
-			pr_debug("\nfda->entries[%d].fd=%d != %d\n", fd,
-				 fda->entries[fd].fd, expected_fd[fd]);
-			goto out_delete;
-		}
-	}
-
 	pr_debug("\n");
 
 	err = 0;
-- 
2.24.1

[PATCH v9 02/15] tools/libperf: add properties to struct pollfd *entries objects

[Alexey Budankov]

Store boolean properties per struct pollfd *entries object in a
bitmap of int size. Implement fdarray_prop__nonfilterable property
to skip object from counting by fdarray_filter().

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/lib/api/fd/array.c                 | 17 +++++++++--------
 tools/lib/api/fd/array.h                 | 18 +++++++++++++-----
 tools/lib/perf/evlist.c                  | 10 +++++-----
 tools/lib/perf/include/internal/evlist.h |  2 +-
 tools/perf/tests/fdarray.c               |  2 +-
 tools/perf/util/evlist.c                 |  2 +-
 6 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
index 89f9a2193c2d..a4223f8cb1ce 100644
--- a/tools/lib/api/fd/array.c
+++ b/tools/lib/api/fd/array.c
@@ -12,31 +12,31 @@
 void fdarray__init(struct fdarray *fda, int nr_autogrow)
 {
 	fda->entries	 = NULL;
-	fda->priv	 = NULL;
+	fda->prop	 = NULL;
 	fda->nr		 = fda->nr_alloc = 0;
 	fda->nr_autogrow = nr_autogrow;
 }
 
 int fdarray__grow(struct fdarray *fda, int nr)
 {
-	void *priv;
+	void *prop;
 	int nr_alloc = fda->nr_alloc + nr;
-	size_t psize = sizeof(fda->priv[0]) * nr_alloc;
+	size_t psize = sizeof(fda->prop[0]) * nr_alloc;
 	size_t size  = sizeof(struct pollfd) * nr_alloc;
 	struct pollfd *entries = realloc(fda->entries, size);
 
 	if (entries == NULL)
 		return -ENOMEM;
 
-	priv = realloc(fda->priv, psize);
-	if (priv == NULL) {
+	prop = realloc(fda->prop, psize);
+	if (prop == NULL) {
 		free(entries);
 		return -ENOMEM;
 	}
 
 	fda->nr_alloc = nr_alloc;
 	fda->entries  = entries;
-	fda->priv     = priv;
+	fda->prop     = prop;
 	return 0;
 }
 
@@ -59,7 +59,7 @@ struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow)
 void fdarray__exit(struct fdarray *fda)
 {
 	free(fda->entries);
-	free(fda->priv);
+	free(fda->prop);
 	fdarray__init(fda, 0);
 }
 
@@ -69,7 +69,7 @@ void fdarray__delete(struct fdarray *fda)
 	free(fda);
 }
 
-int fdarray__add(struct fdarray *fda, int fd, short revents)
+int fdarray__add(struct fdarray *fda, int fd, short revents, enum fdarray_props props)
 {
 	int pos = fda->nr;
 
@@ -79,6 +79,7 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
 
 	fda->entries[fda->nr].fd     = fd;
 	fda->entries[fda->nr].events = revents;
+	fda->prop[fda->nr].bits = props;
 	fda->nr++;
 	return pos;
 }
diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
index b39557d1a88f..19b6a34aeea5 100644
--- a/tools/lib/api/fd/array.h
+++ b/tools/lib/api/fd/array.h
@@ -21,10 +21,18 @@ struct fdarray {
 	int	       nr_alloc;
 	int	       nr_autogrow;
 	struct pollfd *entries;
-	union {
-		int    idx;
-		void   *ptr;
-	} *priv;
+	struct {
+		union {
+			int    idx;
+			void   *ptr;
+		} priv;
+		int bits;
+	} *prop;
+};
+
+enum fdarray_props {
+	fdarray_prop__default	    = 0x00000000,
+	fdarray_prop__nonfilterable = 0x00000001
 };
 
 void fdarray__init(struct fdarray *fda, int nr_autogrow);
@@ -33,7 +41,7 @@ void fdarray__exit(struct fdarray *fda);
 struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow);
 void fdarray__delete(struct fdarray *fda);
 
-int fdarray__add(struct fdarray *fda, int fd, short revents);
+int fdarray__add(struct fdarray *fda, int fd, short revents, enum fdarray_props props);
 int fdarray__poll(struct fdarray *fda, int timeout);
 int fdarray__filter(struct fdarray *fda, short revents,
 		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index 6a875a0f01bb..25e76e458afb 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -305,12 +305,12 @@ int perf_evlist__alloc_pollfd(struct perf_evlist *evlist)
 }
 
 int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd,
-			    void *ptr, short revent)
+			    void *ptr, short revent, enum fdarray_props props)
 {
-	int pos = fdarray__add(&evlist->pollfd, fd, revent | POLLERR | POLLHUP);
+	int pos = fdarray__add(&evlist->pollfd, fd, revent | POLLERR | POLLHUP, props);
 
 	if (pos >= 0) {
-		evlist->pollfd.priv[pos].ptr = ptr;
+		evlist->pollfd.prop[pos].priv.ptr = ptr;
 		fcntl(fd, F_SETFL, O_NONBLOCK);
 	}
 
@@ -320,7 +320,7 @@ int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd,
 static void perf_evlist__munmap_filtered(struct fdarray *fda, int fd,
 					 void *arg __maybe_unused)
 {
-	struct perf_mmap *map = fda->priv[fd].ptr;
+	struct perf_mmap *map = fda->prop[fd].priv.ptr;
 
 	if (map)
 		perf_mmap__put(map);
@@ -488,7 +488,7 @@ mmap_per_evsel(struct perf_evlist *evlist, struct perf_evlist_mmap_ops *ops,
 		revent = !overwrite ? POLLIN : 0;
 
 		if (!evsel->system_wide &&
-		    perf_evlist__add_pollfd(evlist, fd, map, revent) < 0) {
+		    perf_evlist__add_pollfd(evlist, fd, map, revent, fdarray_prop__default) < 0) {
 			perf_mmap__put(map);
 			return -1;
 		}
diff --git a/tools/lib/perf/include/internal/evlist.h b/tools/lib/perf/include/internal/evlist.h
index 74dc8c3f0b66..87bc0fa7293c 100644
--- a/tools/lib/perf/include/internal/evlist.h
+++ b/tools/lib/perf/include/internal/evlist.h
@@ -45,7 +45,7 @@ struct perf_evlist_mmap_ops {
 
 int perf_evlist__alloc_pollfd(struct perf_evlist *evlist);
 int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd,
-			    void *ptr, short revent);
+			    void *ptr, short revent, enum fdarray_props props);
 
 int perf_evlist__mmap_ops(struct perf_evlist *evlist,
 			  struct perf_evlist_mmap_ops *ops,
diff --git a/tools/perf/tests/fdarray.c b/tools/perf/tests/fdarray.c
index d0c8a05aab2f..b573f386ab2f 100644
--- a/tools/perf/tests/fdarray.c
+++ b/tools/perf/tests/fdarray.c
@@ -112,7 +112,7 @@ int test__fdarray__add(struct test *test __maybe_unused, int subtest __maybe_unu
 	}
 
 #define FDA_ADD(_idx, _fd, _revents, _nr)				   \
-	if (fdarray__add(fda, _fd, _revents) < 0) {			   \
+	if (fdarray__add(fda, _fd, _revents, fdarray_prop__default) < 0) { \
 		pr_debug("\n%d: fdarray__add(fda, %d, %d) failed!",	   \
 			 __LINE__,_fd, _revents);			   \
 		goto out_delete;					   \
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 1b884695b4d3..7b1beffec6a1 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -497,7 +497,7 @@ int perf_evlist__enable_event_idx(struct evlist *evlist,
 
 int evlist__add_pollfd(struct evlist *evlist, int fd)
 {
-	return perf_evlist__add_pollfd(&evlist->core, fd, NULL, POLLIN);
+	return perf_evlist__add_pollfd(&evlist->core, fd, NULL, POLLIN, fdarray_prop__default);
 }
 
 int evlist__filter_pollfd(struct evlist *evlist, short revents_and_mask)
-- 
2.24.1

[PATCH v9 03/15] tools/libperf: don't count nonfilterable fds by fdarray__filter()

[Alexey Budankov]

Avoid counting of struct pollfd *entries objects with
fdarray_prop__nonfilterable propery by fdarray__filter().
Non-filterable objects are still processed if monitored
events have been signaled for them.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/lib/api/fd/array.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
index a4223f8cb1ce..e902c0260414 100644
--- a/tools/lib/api/fd/array.c
+++ b/tools/lib/api/fd/array.c
@@ -105,7 +105,8 @@ int fdarray__filter(struct fdarray *fda, short revents,
 			continue;
 		}
 
-		++nr;
+		if (!(fda->prop[fd].bits & fdarray_prop__nonfilterable))
+			++nr;
 	}
 
 	return nr;
-- 
2.24.1

[PATCH v9 04/15] perf evlist: introduce control file descriptors

[Alexey Budankov]

Define and initialize control file descriptors.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/util/evlist.c | 3 +++
 tools/perf/util/evlist.h | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7b1beffec6a1..ee545a122b52 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -63,6 +63,9 @@ void evlist__init(struct evlist *evlist, struct perf_cpu_map *cpus,
 	perf_evlist__set_maps(&evlist->core, cpus, threads);
 	evlist->workload.pid = -1;
 	evlist->bkw_mmap_state = BKW_MMAP_NOTREADY;
+	evlist->ctl_fd.fd = -1;
+	evlist->ctl_fd.ack = -1;
+	evlist->ctl_fd.pos = -1;
 }
 
 struct evlist *evlist__new(void)
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 38901c0d1599..2caf19fb87a8 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -74,6 +74,11 @@ struct evlist {
 		pthread_t		th;
 		volatile int		done;
 	} thread;
+	struct {
+		int	fd;
+		int	ack;
+		int	pos;
+	} ctl_fd;
 };
 
 struct evsel_str_handler {
-- 
2.24.1

[PATCH v9 05/15] perf evlist: implement control command handling functions

[Alexey Budankov]

Implement functions of initialization, finalization and processing
of control command messages coming from control file descriptors.
Allocate control file descriptor as descriptor at struct pollfd
object of evsel_list for atomic poll() operation.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/util/evlist.c | 134 +++++++++++++++++++++++++++++++++++++++
 tools/perf/util/evlist.h |  17 +++++
 2 files changed, 151 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index ee545a122b52..bd0b2efa2e4a 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1714,3 +1714,137 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
 	}
 	return leader;
 }
+
+int evlist__initialize_ctlfd(struct evlist *evlist, int fd, int ack)
+{
+	if (fd == -1) {
+		pr_debug("Control descriptor is not initialized\n");
+		return 0;
+	}
+
+	evlist->ctl_fd.pos = perf_evlist__add_pollfd(&evlist->core, fd, NULL, POLLIN,
+						     fdarray_prop__nonfilterable);
+	if (evlist->ctl_fd.pos < 0) {
+		evlist->ctl_fd.pos = -1;
+		pr_err("Failed to add ctl fd entry: %m\n");
+		return -1;
+	}
+
+	evlist->ctl_fd.fd = fd;
+	evlist->ctl_fd.ack = ack;
+
+	return 0;
+}
+
+int evlist__finalize_ctlfd(struct evlist *evlist)
+{
+	struct pollfd *entries = evlist->core.pollfd.entries;
+
+	if (evlist->ctl_fd.pos == -1)
+		return 0;
+
+	entries[evlist->ctl_fd.pos].fd = -1;
+	entries[evlist->ctl_fd.pos].events = 0;
+	entries[evlist->ctl_fd.pos].revents = 0;
+
+	evlist->ctl_fd.pos = -1;
+	evlist->ctl_fd.ack = -1;
+	evlist->ctl_fd.fd = -1;
+
+	return 0;
+}
+
+static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd,
+			      char *cmd_data, size_t data_size)
+{
+	int err;
+	char c;
+	size_t bytes_read = 0;
+
+	memset(cmd_data, 0, data_size--);
+
+	do {
+		err = read(evlist->ctl_fd.fd, &c, 1);
+		if (err > 0) {
+			if (c == '\n' || c == '\0')
+				break;
+			cmd_data[bytes_read++] = c;
+			if (bytes_read == data_size)
+				break;
+		} else {
+			if (err == -1)
+				pr_err("Failed to read from ctlfd %d: %m\n", evlist->ctl_fd.fd);
+			break;
+		}
+	} while (1);
+
+	pr_debug("Message from ctl_fd: \"%s%s\"\n", cmd_data,
+		 bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0");
+
+	if (err > 0) {
+		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,
+				    (sizeof(EVLIST_CTL_CMD_DISABLE_TAG)-1))) {
+			*cmd = EVLIST_CTL_CMD_DISABLE;
+		}
+	}
+
+	return err;
+}
+
+static int evlist__ctlfd_ack(struct evlist *evlist)
+{
+	int err;
+
+	if (evlist->ctl_fd.ack == -1)
+		return 0;
+
+	err = write(evlist->ctl_fd.ack, EVLIST_CTL_CMD_ACK_TAG,
+		    sizeof(EVLIST_CTL_CMD_ACK_TAG));
+	if (err == -1)
+		pr_err("failed to write to ctl_ack_fd %d: %m\n", evlist->ctl_fd.ack);
+
+	return err;
+}
+
+int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
+{
+	int err = 0;
+	char cmd_data[EVLIST_CTL_CMD_MAX_LEN];
+	int ctlfd_pos = evlist->ctl_fd.pos;
+	struct pollfd *entries = evlist->core.pollfd.entries;
+
+	if (ctlfd_pos == -1 || !entries[ctlfd_pos].revents)
+		return 0;
+
+	if (entries[ctlfd_pos].revents & POLLIN) {
+		err = evlist__ctlfd_recv(evlist, cmd, cmd_data,
+					 EVLIST_CTL_CMD_MAX_LEN);
+		if (err > 0) {
+			switch (*cmd) {
+			case EVLIST_CTL_CMD_ENABLE:
+				evlist__enable(evlist);
+				break;
+			case EVLIST_CTL_CMD_DISABLE:
+				evlist__disable(evlist);
+				break;
+			case EVLIST_CTL_CMD_ACK:
+			case EVLIST_CTL_CMD_UNSUPPORTED:
+			default:
+				pr_debug("ctlfd: unsupported %d\n", *cmd);
+				break;
+			}
+			if (!(*cmd == EVLIST_CTL_CMD_ACK || *cmd == EVLIST_CTL_CMD_UNSUPPORTED))
+				evlist__ctlfd_ack(evlist);
+		}
+	}
+
+	if (entries[ctlfd_pos].revents & (POLLHUP | POLLERR))
+		evlist__finalize_ctlfd(evlist);
+	else
+		entries[ctlfd_pos].revents = 0;
+
+	return err;
+}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 2caf19fb87a8..7e7abc621366 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -359,4 +359,21 @@ void perf_evlist__force_leader(struct evlist *evlist);
 struct evsel *perf_evlist__reset_weak_group(struct evlist *evlist,
 						 struct evsel *evsel,
 						bool close);
+#define EVLIST_CTL_CMD_ENABLE_TAG  "enable"
+#define EVLIST_CTL_CMD_DISABLE_TAG "disable"
+#define EVLIST_CTL_CMD_ACK_TAG     "ack\n"
+
+#define EVLIST_CTL_CMD_MAX_LEN 64
+
+enum evlist_ctl_cmd {
+	EVLIST_CTL_CMD_UNSUPPORTED = 0,
+	EVLIST_CTL_CMD_ENABLE,
+	EVLIST_CTL_CMD_DISABLE,
+	EVLIST_CTL_CMD_ACK
+};
+
+int evlist__initialize_ctlfd(struct evlist *evlist, int ctl_fd, int ctl_fd_ack);
+int evlist__finalize_ctlfd(struct evlist *evlist);
+int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd);
+
 #endif /* __PERF_EVLIST_H */
-- 
2.24.1

[PATCH v9 06/15] perf stat: factor out body of event handling loop for system wide

[Alexey Budankov]

Introduce handle_interval() function that factors out body of event
handling loop for attach and system wide monitoring use cases.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/builtin-stat.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 922d9961ba98..c74e8f94375c 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -475,6 +475,16 @@ static void process_interval(void)
 	print_counters(&rs, 0, NULL);
 }
 
+static bool handle_interval(unsigned int interval, int *times)
+{
+	if (interval) {
+		process_interval();
+		if (interval_count && !(--(*times)))
+			return true;
+	}
+	return false;
+}
+
 static void enable_counters(void)
 {
 	if (stat_config.initial_delay)
@@ -611,6 +621,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	struct affinity affinity;
 	int i, cpu;
 	bool second_pass = false;
+	bool stop = false;
 
 	if (interval) {
 		ts.tv_sec  = interval / USEC_PER_MSEC;
@@ -805,17 +816,13 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
 		enable_counters();
-		while (!done) {
+		while (!done && !stop) {
 			nanosleep(&ts, NULL);
 			if (!is_target_alive(&target, evsel_list->core.threads))
 				break;
 			if (timeout)
 				break;
-			if (interval) {
-				process_interval();
-				if (interval_count && !(--times))
-					break;
-			}
+			stop = handle_interval(interval, &times);
 		}
 	}
 
-- 
2.24.1

[PATCH v9 07/15] perf stat: move target check to loop control statement

[Alexey Budankov]

Check for target existence in loop control statement jointly with 'stop'
indicator based on command line values and external asynchronous 'done'
signal.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/builtin-stat.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index c74e8f94375c..3baf00482666 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -816,10 +816,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
 		enable_counters();
-		while (!done && !stop) {
+		while (!done && !stop && is_target_alive(&target, evsel_list->core.threads)) {
 			nanosleep(&ts, NULL);
-			if (!is_target_alive(&target, evsel_list->core.threads))
-				break;
 			if (timeout)
 				break;
 			stop = handle_interval(interval, &times);
-- 
2.24.1

[PATCH v9 08/15] perf stat: factor out body of event handling loop for fork case

[Alexey Budankov]

Factor out body of event handling loop for fork case reusing
handle_interval() function.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/builtin-stat.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 3baf00482666..3e11f854ffc8 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -791,13 +791,11 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		enable_counters();
 
 		if (interval || timeout) {
-			while (!waitpid(child_pid, &status, WNOHANG)) {
+			while (!stop && !waitpid(child_pid, &status, WNOHANG)) {
 				nanosleep(&ts, NULL);
 				if (timeout)
 					break;
-				process_interval();
-				if (interval_count && !(--times))
-					break;
+				stop = handle_interval(interval, &times);
 			}
 		}
 		if (child_pid != -1) {
-- 
2.24.1

[PATCH v9 09/15] perf stat: factor out event handling loop into dispatch_events()

[Alexey Budankov]

Consolidate event dispatching loops for fork, attach and system
wide monitoring use cases into common dispatch_events() function.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/builtin-stat.c | 42 +++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 3e11f854ffc8..723f1fe27d63 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -550,6 +550,30 @@ static bool is_target_alive(struct target *_target,
 	return false;
 }
 
+static int dispatch_events(bool forks, int timeout, int interval, int *times, struct timespec *ts)
+{
+	bool stop = false;
+	int child_exited = 0, status = 0;
+
+	while (1) {
+		if (forks)
+			child_exited = waitpid(child_pid, &status, WNOHANG);
+		else
+			child_exited = !is_target_alive(&target, evsel_list->core.threads) ? 1 : 0;
+
+		if (done || stop || child_exited)
+			break;
+
+		nanosleep(ts, NULL);
+		if (timeout)
+			stop = true;
+		else
+			stop = handle_interval(interval, times);
+	}
+
+	return status;
+}
+
 enum counter_recovery {
 	COUNTER_SKIP,
 	COUNTER_RETRY,
@@ -621,7 +645,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	struct affinity affinity;
 	int i, cpu;
 	bool second_pass = false;
-	bool stop = false;
 
 	if (interval) {
 		ts.tv_sec  = interval / USEC_PER_MSEC;
@@ -790,14 +813,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		perf_evlist__start_workload(evsel_list);
 		enable_counters();
 
-		if (interval || timeout) {
-			while (!stop && !waitpid(child_pid, &status, WNOHANG)) {
-				nanosleep(&ts, NULL);
-				if (timeout)
-					break;
-				stop = handle_interval(interval, &times);
-			}
-		}
+		if (interval || timeout)
+			status = dispatch_events(forks, timeout, interval, &times, &ts);
 		if (child_pid != -1) {
 			if (timeout)
 				kill(child_pid, SIGTERM);
@@ -814,12 +831,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
 		enable_counters();
-		while (!done && !stop && is_target_alive(&target, evsel_list->core.threads)) {
-			nanosleep(&ts, NULL);
-			if (timeout)
-				break;
-			stop = handle_interval(interval, &times);
-		}
+		status = dispatch_events(forks, timeout, interval, &times, &ts);
 	}
 
 	disable_counters();
-- 
2.24.1

[PATCH v9 10/15] perf stat: extend -D,--delay option with -1 value

[Alexey Budankov]

Extend -D,--delay option with -1 value to start monitoring with
events disabled to be enabled later by enable command provided
via control file descriptor.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/Documentation/perf-stat.txt |  5 +++--
 tools/perf/builtin-stat.c              | 18 ++++++++++++++----
 tools/perf/util/evlist.h               |  3 +++
 tools/perf/util/stat.h                 |  2 +-
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
index b029ee728a0b..9f32f6cd558d 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -238,8 +238,9 @@ mode, use --per-node in addition to -a. (system-wide).
 
 -D msecs::
 --delay msecs::
-After starting the program, wait msecs before measuring. This is useful to
-filter out the startup phase of the program, which is often very different.
+After starting the program, wait msecs before measuring (-1: start with events
+disabled). This is useful to filter out the startup phase of the program,
+which is often very different.
 
 -T::
 --transaction::
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 723f1fe27d63..9e4288ecf2b8 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -487,16 +487,26 @@ static bool handle_interval(unsigned int interval, int *times)
 
 static void enable_counters(void)
 {
-	if (stat_config.initial_delay)
+	if (stat_config.initial_delay < 0) {
+		pr_info(EVLIST_DISABLED_MSG);
+		return;
+	}
+
+	if (stat_config.initial_delay > 0) {
+		pr_info(EVLIST_DISABLED_MSG);
 		usleep(stat_config.initial_delay * USEC_PER_MSEC);
+	}
 
 	/*
 	 * We need to enable counters only if:
 	 * - we don't have tracee (attaching to task or cpu)
 	 * - we have initial delay configured
 	 */
-	if (!target__none(&target) || stat_config.initial_delay)
+	if (!target__none(&target) || stat_config.initial_delay) {
 		evlist__enable(evsel_list);
+		if (stat_config.initial_delay > 0)
+			pr_info(EVLIST_ENABLED_MSG);
+	}
 }
 
 static void disable_counters(void)
@@ -1056,8 +1066,8 @@ static struct option stat_options[] = {
 		     "aggregate counts per thread", AGGR_THREAD),
 	OPT_SET_UINT(0, "per-node", &stat_config.aggr_mode,
 		     "aggregate counts per numa node", AGGR_NODE),
-	OPT_UINTEGER('D', "delay", &stat_config.initial_delay,
-		     "ms to wait before starting measurement after program start"),
+	OPT_INTEGER('D', "delay", &stat_config.initial_delay,
+		    "ms to wait before starting measurement after program start (-1: start with events disabled)"),
 	OPT_CALLBACK_NOOPT(0, "metric-only", &stat_config.metric_only, NULL,
 			"Only print computed metrics. No raw values", enable_metric_only),
 	OPT_BOOLEAN(0, "metric-no-group", &stat_config.metric_no_group,
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 7e7abc621366..0728a4c35f7d 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -376,4 +376,7 @@ int evlist__initialize_ctlfd(struct evlist *evlist, int ctl_fd, int ctl_fd_ack);
 int evlist__finalize_ctlfd(struct evlist *evlist);
 int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd);
 
+#define EVLIST_ENABLED_MSG "Events enabled\n"
+#define EVLIST_DISABLED_MSG "Events disabled\n"
+
 #endif /* __PERF_EVLIST_H */
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index 6911c7249199..41d59f192931 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -116,7 +116,7 @@ struct perf_stat_config {
 	FILE			*output;
 	unsigned int		 interval;
 	unsigned int		 timeout;
-	unsigned int		 initial_delay;
+	int			 initial_delay;
 	unsigned int		 unit_width;
 	unsigned int		 metric_only_len;
 	int			 times;
-- 
2.24.1

[PATCH v9 11/15] perf stat: implement control commands handling

[Alexey Budankov]

Implement handling of 'enable' and 'disable' control commands
coming from control file descriptor. process_evlist() function
checks for events on control fds and makes required operations.
If poll event splits initiated timeout interval then the reminder
is calculated and still waited in the following poll() syscall.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 20 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 9e4288ecf2b8..5021f7286422 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
 	return false;
 }
 
+static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
+{
+	bool stop = false;
+	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
+
+	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
+		switch (cmd) {
+		case EVLIST_CTL_CMD_ENABLE:
+			pr_info(EVLIST_ENABLED_MSG);
+			stop = handle_interval(interval, times);
+			break;
+		case EVLIST_CTL_CMD_DISABLE:
+			stop = handle_interval(interval, times);
+			pr_info(EVLIST_DISABLED_MSG);
+			break;
+		case EVLIST_CTL_CMD_ACK:
+		case EVLIST_CTL_CMD_UNSUPPORTED:
+		default:
+			break;
+		}
+	}
+
+	return stop;
+}
+
 static void enable_counters(void)
 {
 	if (stat_config.initial_delay < 0) {
@@ -560,10 +585,21 @@ static bool is_target_alive(struct target *_target,
 	return false;
 }
 
-static int dispatch_events(bool forks, int timeout, int interval, int *times, struct timespec *ts)
+static int dispatch_events(bool forks, int timeout, int interval, int *times)
 {
 	bool stop = false;
 	int child_exited = 0, status = 0;
+	int time_to_sleep, sleep_time;
+	struct timespec time_start, time_stop, time_diff;
+
+	if (interval)
+		sleep_time = interval;
+	else if (timeout)
+		sleep_time = timeout;
+	else
+		sleep_time = 1000;
+
+	time_to_sleep = sleep_time;
 
 	while (1) {
 		if (forks)
@@ -574,11 +610,22 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times, st
 		if (done || stop || child_exited)
 			break;
 
-		nanosleep(ts, NULL);
-		if (timeout)
-			stop = true;
-		else
-			stop = handle_interval(interval, times);
+		clock_gettime(CLOCK_MONOTONIC, &time_start);
+		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
+			if (timeout)
+				stop = true;
+			else
+				stop = handle_interval(interval, times);
+			time_to_sleep = sleep_time;
+		} else { /* fd revent */
+			stop = process_evlist(evsel_list, interval, times);
+			clock_gettime(CLOCK_MONOTONIC, &time_stop);
+			diff_timespec(&time_diff, &time_stop, &time_start);
+			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
+					 time_diff.tv_nsec / NSEC_PER_MSEC;
+			if (time_to_sleep < 0)
+				time_to_sleep = 0;
+		}
 	}
 
 	return status;
@@ -647,7 +694,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	char msg[BUFSIZ];
 	unsigned long long t0, t1;
 	struct evsel *counter;
-	struct timespec ts;
 	size_t l;
 	int status = 0;
 	const bool forks = (argc > 0);
@@ -656,17 +702,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	int i, cpu;
 	bool second_pass = false;
 
-	if (interval) {
-		ts.tv_sec  = interval / USEC_PER_MSEC;
-		ts.tv_nsec = (interval % USEC_PER_MSEC) * NSEC_PER_MSEC;
-	} else if (timeout) {
-		ts.tv_sec  = timeout / USEC_PER_MSEC;
-		ts.tv_nsec = (timeout % USEC_PER_MSEC) * NSEC_PER_MSEC;
-	} else {
-		ts.tv_sec  = 1;
-		ts.tv_nsec = 0;
-	}
-
 	if (forks) {
 		if (perf_evlist__prepare_workload(evsel_list, &target, argv, is_pipe,
 						  workload_exec_failed_signal) < 0) {
@@ -824,7 +859,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		enable_counters();
 
 		if (interval || timeout)
-			status = dispatch_events(forks, timeout, interval, &times, &ts);
+			status = dispatch_events(forks, timeout, interval, &times);
 		if (child_pid != -1) {
 			if (timeout)
 				kill(child_pid, SIGTERM);
@@ -841,7 +876,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
 		enable_counters();
-		status = dispatch_events(forks, timeout, interval, &times, &ts);
+		status = dispatch_events(forks, timeout, interval, &times);
 	}
 
 	disable_counters();
-- 
2.24.1

[PATCH v9 12/15] perf stat: introduce --control fd:ctl-fd[,ack-fd] options

[Alexey Budankov]

Introduce --control fd:ctl-fd[,ack-fd] options to pass open file
descriptors numbers from command line. Extend perf-stat.txt file
with --control fd:ctl-fd[,ack-fd] options description. Document
possible usage model introduced by --control fd:ctl-fd[,ack-fd]
options by providing example bash shell script.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/Documentation/perf-stat.txt | 39 ++++++++++++++++++++++++++
 tools/perf/builtin-stat.c              | 38 +++++++++++++++++++++++++
 tools/perf/util/stat.h                 |  2 ++
 3 files changed, 79 insertions(+)

diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
index 9f32f6cd558d..c9bfefc051fb 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -176,6 +176,45 @@ with it.  --append may be used here.  Examples:
      3>results  perf stat --log-fd 3          -- $cmd
      3>>results perf stat --log-fd 3 --append -- $cmd
 
+--control fd:ctl-fd[,ack-fd]
+Listen on ctl-fd descriptor for command to control measurement ('enable': enable events,
+'disable': disable events). Measurements can be started with events disabled using
+--delay=-1 option. Optionally send control command completion ('ack\n') to ack-fd descriptor
+to synchronize with the controlling process. Example of bash shell script to enable and
+disable events during measurements:
+
+#!/bin/bash
+
+ctl_dir=/tmp/
+
+ctl_fifo=${ctl_dir}perf_ctl.fifo
+test -p ${ctl_fifo} && unlink ${ctl_fifo}
+mkfifo ${ctl_fifo}
+exec {ctl_fd}<>${ctl_fifo}
+
+ctl_ack_fifo=${ctl_dir}perf_ctl_ack.fifo
+test -p ${ctl_ack_fifo} && unlink ${ctl_ack_fifo}
+mkfifo ${ctl_ack_fifo}
+exec {ctl_fd_ack}<>${ctl_ack_fifo}
+
+perf stat -D -1 -e cpu-cycles -a -I 1000       \
+          --control fd:${ctl_fd},${ctl_fd_ack} \
+          -- sleep 30 &
+perf_pid=$!
+
+sleep 5  && echo 'enable' >&${ctl_fd} && read -u ${ctl_fd_ack} e1 && echo "enabled(${e1})"
+sleep 10 && echo 'disable' >&${ctl_fd} && read -u ${ctl_fd_ack} d1 && echo "disabled(${d1})"
+
+exec {ctl_fd_ack}>&-
+unlink ${ctl_ack_fifo}
+
+exec {ctl_fd}>&-
+unlink ${ctl_fifo}
+
+wait -n ${perf_pid}
+exit $?
+
+
 --pre::
 --post::
 	Pre and post measurement hooks, e.g.:
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 5021f7286422..72fe310b7241 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -188,6 +188,8 @@ static struct perf_stat_config stat_config = {
 	.metric_only_len	= METRIC_ONLY_LEN,
 	.walltime_nsecs_stats	= &walltime_nsecs_stats,
 	.big_num		= true,
+	.ctl_fd			= -1,
+	.ctl_fd_ack		= -1
 };
 
 static bool cpus_map_matched(struct evsel *a, struct evsel *b)
@@ -1030,6 +1032,33 @@ static int parse_metric_groups(const struct option *opt,
 					 &stat_config.metric_events);
 }
 
+static int parse_control_option(const struct option *opt,
+				const char *str,
+				int unset __maybe_unused)
+{
+	char *comma = NULL, *endptr = NULL;
+	struct perf_stat_config *config = (struct perf_stat_config *)opt->value;
+
+	if (strncmp(str, "fd:", 3))
+		return -EINVAL;
+
+	config->ctl_fd = strtoul(&str[3], &endptr, 0);
+	if (endptr == &str[3])
+		return -EINVAL;
+
+	comma = strchr(str, ',');
+	if (comma) {
+		if (endptr != comma)
+			return -EINVAL;
+
+		config->ctl_fd_ack = strtoul(comma + 1, &endptr, 0);
+		if (endptr == comma + 1 || *endptr != '\0')
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static struct option stat_options[] = {
 	OPT_BOOLEAN('T', "transaction", &transaction_run,
 		    "hardware transaction statistics"),
@@ -1131,6 +1160,10 @@ static struct option stat_options[] = {
 		"libpfm4 event selector. use 'perf list' to list available events",
 		parse_libpfm_events_option),
 #endif
+	OPT_CALLBACK(0, "control", &stat_config, "fd:ctl-fd[,ack-fd]",
+		     "Listen on ctl-fd descriptor for command to control measurement ('enable': enable events, 'disable': disable events).\n"
+		     "\t\t\t  Optionally send control command completion ('ack\\n') to ack-fd descriptor.",
+		      parse_control_option),
 	OPT_END()
 };
 
@@ -2300,6 +2333,9 @@ int cmd_stat(int argc, const char **argv)
 	signal(SIGALRM, skip_signal);
 	signal(SIGABRT, skip_signal);
 
+	if (evlist__initialize_ctlfd(evsel_list, stat_config.ctl_fd, stat_config.ctl_fd_ack))
+		goto out;
+
 	status = 0;
 	for (run_idx = 0; forever || run_idx < stat_config.run_count; run_idx++) {
 		if (stat_config.run_count != 1 && verbose > 0)
@@ -2319,6 +2355,8 @@ int cmd_stat(int argc, const char **argv)
 	if (!forever && status != -1 && (!interval || stat_config.summary))
 		print_counters(NULL, argc, argv);
 
+	evlist__finalize_ctlfd(evsel_list);
+
 	if (STAT_RECORD) {
 		/*
 		 * We synthesize the kernel mmap record just so that older tools
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index 41d59f192931..f8778cffd941 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -133,6 +133,8 @@ struct perf_stat_config {
 	struct perf_cpu_map		*cpus_aggr_map;
 	u64			*walltime_run;
 	struct rblist		 metric_events;
+	int			 ctl_fd;
+	int			 ctl_fd_ack;
 };
 
 void perf_stat__set_big_num(int set);
-- 
2.24.1

[PATCH v9 13/15] perf record: extend -D,--delay option with -1 value

[Alexey Budankov]

Extend -D,--delay option with -1 to start collection with events
disabled to be enabled later by 'enable' command provided via
control file descriptor.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/Documentation/perf-record.txt |  5 +++--
 tools/perf/builtin-record.c              | 12 ++++++++----
 tools/perf/builtin-trace.c               |  2 +-
 tools/perf/util/record.h                 |  2 +-
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index fa8a5fcd27ab..a84376605805 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -407,8 +407,9 @@ if combined with -a or -C options.
 
 -D::
 --delay=::
-After starting the program, wait msecs before measuring. This is useful to
-filter out the startup phase of the program, which is often very different.
+After starting the program, wait msecs before measuring (-1: start with events
+disabled). This is useful to filter out the startup phase of the program, which
+is often very different.
 
 -I::
 --intr-regs::
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 19b1d5effb7a..cd1892c4844b 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1749,8 +1749,12 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	}
 
 	if (opts->initial_delay) {
-		usleep(opts->initial_delay * USEC_PER_MSEC);
-		evlist__enable(rec->evlist);
+		pr_info(EVLIST_DISABLED_MSG);
+		if (opts->initial_delay > 0) {
+			usleep(opts->initial_delay * USEC_PER_MSEC);
+			evlist__enable(rec->evlist);
+			pr_info(EVLIST_ENABLED_MSG);
+		}
 	}
 
 	trigger_ready(&auxtrace_snapshot_trigger);
@@ -2462,8 +2466,8 @@ static struct option __record_options[] = {
 	OPT_CALLBACK('G', "cgroup", &record.evlist, "name",
 		     "monitor event in cgroup name only",
 		     parse_cgroups),
-	OPT_UINTEGER('D', "delay", &record.opts.initial_delay,
-		  "ms to wait before starting measurement after program start"),
+	OPT_INTEGER('D', "delay", &record.opts.initial_delay,
+		  "ms to wait before starting measurement after program start (-1: start with events disabled)"),
 	OPT_BOOLEAN(0, "kcore", &record.opts.kcore, "copy /proc/kcore"),
 	OPT_STRING('u', "uid", &record.opts.target.uid_str, "user",
 		   "user to profile"),
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index a333a9a64f27..bea461b6f937 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4812,7 +4812,7 @@ int cmd_trace(int argc, const char **argv)
 			"per thread proc mmap processing timeout in ms"),
 	OPT_CALLBACK('G', "cgroup", &trace, "name", "monitor event in cgroup name only",
 		     trace__parse_cgroups),
-	OPT_UINTEGER('D', "delay", &trace.opts.initial_delay,
+	OPT_INTEGER('D', "delay", &trace.opts.initial_delay,
 		     "ms to wait before starting measurement after program "
 		     "start"),
 	OPTS_EVSWITCH(&trace.evswitch),
diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
index 39d1de4b2a36..da138dcb4d34 100644
--- a/tools/perf/util/record.h
+++ b/tools/perf/util/record.h
@@ -61,7 +61,7 @@ struct record_opts {
 	const char    *auxtrace_snapshot_opts;
 	const char    *auxtrace_sample_opts;
 	bool	      sample_transaction;
-	unsigned      initial_delay;
+	int	      initial_delay;
 	bool	      use_clockid;
 	clockid_t     clockid;
 	u64	      clockid_res_ns;
-- 
2.24.1

[PATCH v9 14/15] perf record: implement control commands handling

[Alexey Budankov]

Implement handling of 'enable' and 'disable' control commands
coming from control file descriptor.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/builtin-record.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index cd1892c4844b..632e61fe70bd 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1527,6 +1527,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	bool disabled = false, draining = false;
 	int fd;
 	float ratio = 0;
+	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
 
 	atexit(record__sig_exit);
 	signal(SIGCHLD, sig_handler);
@@ -1846,6 +1847,21 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 				draining = true;
 		}
 
+		if (evlist__ctlfd_process(rec->evlist, &cmd) > 0) {
+			switch (cmd) {
+			case EVLIST_CTL_CMD_ENABLE:
+				pr_info(EVLIST_ENABLED_MSG);
+				break;
+			case EVLIST_CTL_CMD_DISABLE:
+				pr_info(EVLIST_DISABLED_MSG);
+				break;
+			case EVLIST_CTL_CMD_ACK:
+			case EVLIST_CTL_CMD_UNSUPPORTED:
+			default:
+				break;
+			}
+		}
+
 		/*
 		 * When perf is starting the traced process, at the end events
 		 * die with the process and we wait for that. Thus no need to
-- 
2.24.1

[PATCH v9 15/15] perf record: introduce --control fd:ctl-fd[,ack-fd] options

[Alexey Budankov]

Introduce --control fd:ctl-fd[,ack-fd] options to pass open file
descriptors numbers from command line. Extend perf-record.txt file
with --control fd:ctl-fd[,ack-fd] options description. Document
possible usage model introduced by --control fd:ctl-fd[,ack-fd]
options by providing example bash shell script.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/Documentation/perf-record.txt | 39 ++++++++++++++++++++++++
 tools/perf/builtin-record.c              | 37 ++++++++++++++++++++++
 tools/perf/util/record.h                 |  2 ++
 3 files changed, 78 insertions(+)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index a84376605805..3f72d8e261f3 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -627,6 +627,45 @@ option. The -e option and this one can be mixed and matched.  Events
 can be grouped using the {} notation.
 endif::HAVE_LIBPFM[]
 
+--control fd:ctl-fd[,ack-fd]
+Listen on ctl-fd descriptor for command to control measurement ('enable': enable events,
+'disable': disable events). Measurements can be started with events disabled using
+--delay=-1 option. Optionally send control command completion ('ack\n') to ack-fd descriptor
+to synchronize with the controlling process. Example of bash shell script to enable and
+disable events during measurements:
+
+#!/bin/bash
+
+ctl_dir=/tmp/
+
+ctl_fifo=${ctl_dir}perf_ctl.fifo
+test -p ${ctl_fifo} && unlink ${ctl_fifo}
+mkfifo ${ctl_fifo}
+exec {ctl_fd}<>${ctl_fifo}
+
+ctl_ack_fifo=${ctl_dir}perf_ctl_ack.fifo
+test -p ${ctl_ack_fifo} && unlink ${ctl_ack_fifo}
+mkfifo ${ctl_ack_fifo}
+exec {ctl_fd_ack}<>${ctl_ack_fifo}
+
+perf record -D -1 -e cpu-cycles -a               \
+            --control fd:${ctl_fd},${ctl_fd_ack} \
+            -- sleep 30 &
+perf_pid=$!
+
+sleep 5  && echo 'enable' >&${ctl_fd} && read -u ${ctl_fd_ack} e1 && echo "enabled(${e1})"
+sleep 10 && echo 'disable' >&${ctl_fd} && read -u ${ctl_fd_ack} d1 && echo "disabled(${d1})"
+
+exec {ctl_fd_ack}>&-
+unlink ${ctl_ack_fifo}
+
+exec {ctl_fd}>&-
+unlink ${ctl_fifo}
+
+wait -n ${perf_pid}
+exit $?
+
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1], linkperf:perf-intel-pt[1]
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 632e61fe70bd..0718aa71b4ba 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1749,6 +1749,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		perf_evlist__start_workload(rec->evlist);
 	}
 
+	if (evlist__initialize_ctlfd(rec->evlist, opts->ctl_fd, opts->ctl_fd_ack))
+		goto out_child;
+
 	if (opts->initial_delay) {
 		pr_info(EVLIST_DISABLED_MSG);
 		if (opts->initial_delay > 0) {
@@ -1895,6 +1898,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		record__synthesize_workload(rec, true);
 
 out_child:
+	evlist__finalize_ctlfd(rec->evlist);
 	record__mmap_read_all(rec, true);
 	record__aio_mmap_read_sync(rec);
 
@@ -2244,6 +2248,33 @@ static int record__parse_mmap_pages(const struct option *opt,
 	return ret;
 }
 
+static int parse_control_option(const struct option *opt,
+				const char *str,
+				int unset __maybe_unused)
+{
+	char *comma = NULL, *endptr = NULL;
+	struct record_opts *config = (struct record_opts *)opt->value;
+
+	if (strncmp(str, "fd:", 3))
+		return -EINVAL;
+
+	config->ctl_fd = strtoul(&str[3], &endptr, 0);
+	if (endptr == &str[3])
+		return -EINVAL;
+
+	comma = strchr(str, ',');
+	if (comma) {
+		if (endptr != comma)
+			return -EINVAL;
+
+		config->ctl_fd_ack = strtoul(comma + 1, &endptr, 0);
+		if (endptr == comma + 1 || *endptr != '\0')
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void switch_output_size_warn(struct record *rec)
 {
 	u64 wakeup_size = evlist__mmap_size(rec->opts.mmap_pages);
@@ -2380,6 +2411,8 @@ static struct record record = {
 		},
 		.mmap_flush          = MMAP_FLUSH_DEFAULT,
 		.nr_threads_synthesize = 1,
+		.ctl_fd              = -1,
+		.ctl_fd_ack          = -1,
 	},
 	.tool = {
 		.sample		= process_sample_event,
@@ -2581,6 +2614,10 @@ static struct option __record_options[] = {
 		"libpfm4 event selector. use 'perf list' to list available events",
 		parse_libpfm_events_option),
 #endif
+	OPT_CALLBACK(0, "control", &record.opts, "fd:ctl-fd[,ack-fd]",
+		     "Listen on ctl-fd descriptor for command to control measurement ('enable': enable events, 'disable': disable events).\n"
+		     "\t\t\t  Optionally send control command completion ('ack\\n') to ack-fd descriptor.",
+		      parse_control_option),
 	OPT_END()
 };
 
diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
index da138dcb4d34..4cb72a478af1 100644
--- a/tools/perf/util/record.h
+++ b/tools/perf/util/record.h
@@ -70,6 +70,8 @@ struct record_opts {
 	int	      mmap_flush;
 	unsigned int  comp_level;
 	unsigned int  nr_threads_synthesize;
+	int	      ctl_fd;
+	int	      ctl_fd_ack;
 };
 
 extern const char * const *record_usage;
-- 
2.24.1

Re: [PATCH v9 02/15] tools/libperf: add properties to struct pollfd *entries objects

[Jiri Olsa]

On Fri, Jul 03, 2020 at 10:41:45AM +0300, Alexey Budankov wrote:
> 
> Store boolean properties per struct pollfd *entries object in a
> bitmap of int size. Implement fdarray_prop__nonfilterable property
> to skip object from counting by fdarray_filter().

ok, I think can do it like this, few comments below

thanks,
jirka

> 
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  tools/lib/api/fd/array.c                 | 17 +++++++++--------
>  tools/lib/api/fd/array.h                 | 18 +++++++++++++-----
>  tools/lib/perf/evlist.c                  | 10 +++++-----
>  tools/lib/perf/include/internal/evlist.h |  2 +-
>  tools/perf/tests/fdarray.c               |  2 +-
>  tools/perf/util/evlist.c                 |  2 +-
>  6 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 89f9a2193c2d..a4223f8cb1ce 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -12,31 +12,31 @@
>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>  {
>  	fda->entries	 = NULL;
> -	fda->priv	 = NULL;
> +	fda->prop	 = NULL;
>  	fda->nr		 = fda->nr_alloc = 0;
>  	fda->nr_autogrow = nr_autogrow;
>  }
>  
>  int fdarray__grow(struct fdarray *fda, int nr)
>  {
> -	void *priv;
> +	void *prop;
>  	int nr_alloc = fda->nr_alloc + nr;
> -	size_t psize = sizeof(fda->priv[0]) * nr_alloc;
> +	size_t psize = sizeof(fda->prop[0]) * nr_alloc;
>  	size_t size  = sizeof(struct pollfd) * nr_alloc;
>  	struct pollfd *entries = realloc(fda->entries, size);
>  
>  	if (entries == NULL)
>  		return -ENOMEM;
>  
> -	priv = realloc(fda->priv, psize);
> -	if (priv == NULL) {
> +	prop = realloc(fda->prop, psize);
> +	if (prop == NULL) {
>  		free(entries);
>  		return -ENOMEM;
>  	}
>  
>  	fda->nr_alloc = nr_alloc;
>  	fda->entries  = entries;
> -	fda->priv     = priv;
> +	fda->prop     = prop;
>  	return 0;
>  }
>  
> @@ -59,7 +59,7 @@ struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow)
>  void fdarray__exit(struct fdarray *fda)
>  {
>  	free(fda->entries);
> -	free(fda->priv);
> +	free(fda->prop);
>  	fdarray__init(fda, 0);
>  }
>  
> @@ -69,7 +69,7 @@ void fdarray__delete(struct fdarray *fda)
>  	free(fda);
>  }
>  
> -int fdarray__add(struct fdarray *fda, int fd, short revents)
> +int fdarray__add(struct fdarray *fda, int fd, short revents, enum fdarray_props props)
>  {
>  	int pos = fda->nr;
>  
> @@ -79,6 +79,7 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>  
>  	fda->entries[fda->nr].fd     = fd;
>  	fda->entries[fda->nr].events = revents;
> +	fda->prop[fda->nr].bits = props;
>  	fda->nr++;
>  	return pos;
>  }
> diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
> index b39557d1a88f..19b6a34aeea5 100644
> --- a/tools/lib/api/fd/array.h
> +++ b/tools/lib/api/fd/array.h
> @@ -21,10 +21,18 @@ struct fdarray {
>  	int	       nr_alloc;
>  	int	       nr_autogrow;
>  	struct pollfd *entries;
> -	union {
> -		int    idx;
> -		void   *ptr;
> -	} *priv;
> +	struct {
> +		union {
> +			int    idx;
> +			void   *ptr;
> +		} priv;
> +		int bits;
> +	} *prop;
> +};

why not keeping the 'priv' as a struct, like:

	struct {
		union {
			int    idx;
			void   *ptr;
		};
		unsigned int flags;
	} *priv;

I think we would have much less changes, also please rename bits
to flags and use some unsigned type for that

> +
> +enum fdarray_props {
> +	fdarray_prop__default	    = 0x00000000,
> +	fdarray_prop__nonfilterable = 0x00000001
>  };

s/fdarray_props/fdarray_flag/

>  
>  void fdarray__init(struct fdarray *fda, int nr_autogrow);
> @@ -33,7 +41,7 @@ void fdarray__exit(struct fdarray *fda);
>  struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow);
>  void fdarray__delete(struct fdarray *fda);
>  
> -int fdarray__add(struct fdarray *fda, int fd, short revents);
> +int fdarray__add(struct fdarray *fda, int fd, short revents, enum fdarray_props props);
>  int fdarray__poll(struct fdarray *fda, int timeout);
>  int fdarray__filter(struct fdarray *fda, short revents,
>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index 6a875a0f01bb..25e76e458afb 100644

SNIP

Re: [PATCH v9 09/15] perf stat: factor out event handling loop into dispatch_events()

[Jiri Olsa]

On Fri, Jul 03, 2020 at 10:46:15AM +0300, Alexey Budankov wrote:
> 
> Consolidate event dispatching loops for fork, attach and system
> wide monitoring use cases into common dispatch_events() function.
> 
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c | 42 +++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 3e11f854ffc8..723f1fe27d63 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -550,6 +550,30 @@ static bool is_target_alive(struct target *_target,
>  	return false;
>  }
>  
> +static int dispatch_events(bool forks, int timeout, int interval, int *times, struct timespec *ts)
> +{
> +	bool stop = false;
> +	int child_exited = 0, status = 0;
> +
> +	while (1) {
> +		if (forks)
> +			child_exited = waitpid(child_pid, &status, WNOHANG);
> +		else
> +			child_exited = !is_target_alive(&target, evsel_list->core.threads) ? 1 : 0;
> +
> +		if (done || stop || child_exited)
> +			break;

can (done || stop) be in the while condition and
we'd check just child_exited in here?

> +
> +		nanosleep(ts, NULL);
> +		if (timeout)
> +			stop = true;

can we just break out in here? like the original code?
I don't think we need the extra iteration

> +		else
> +			stop = handle_interval(interval, times);

same here..?


thanks,
jirka


> +	}
> +
> +	return status;
> +}
> +

SNIP

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Jiri Olsa]

On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
> 
> Implement handling of 'enable' and 'disable' control commands
> coming from control file descriptor. process_evlist() function
> checks for events on control fds and makes required operations.
> If poll event splits initiated timeout interval then the reminder
> is calculated and still waited in the following poll() syscall.
> 
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
>  1 file changed, 55 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 9e4288ecf2b8..5021f7286422 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
>  	return false;
>  }
>  
> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> +{
> +	bool stop = false;
> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> +
> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
> +		switch (cmd) {
> +		case EVLIST_CTL_CMD_ENABLE:
> +			pr_info(EVLIST_ENABLED_MSG);
> +			stop = handle_interval(interval, times);
> +			break;
> +		case EVLIST_CTL_CMD_DISABLE:
> +			stop = handle_interval(interval, times);

I still don't understand why you call handle_interval in here

I don't see it being necessary.. you enable events and handle_interval,
wil be called in the next iteration of dispatch_events, why complicate
this function with that?

thanks,
jirka

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Jiri Olsa]

On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:

SNIP

>  
>  	while (1) {
>  		if (forks)
> @@ -574,11 +610,22 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times, st
>  		if (done || stop || child_exited)
>  			break;
>  
> -		nanosleep(ts, NULL);
> -		if (timeout)
> -			stop = true;
> -		else
> -			stop = handle_interval(interval, times);
> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
> +			if (timeout)
> +				stop = true;
> +			else
> +				stop = handle_interval(interval, times);
> +			time_to_sleep = sleep_time;
> +		} else { /* fd revent */
> +			stop = process_evlist(evsel_list, interval, times);
> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
> +			diff_timespec(&time_diff, &time_stop, &time_start);
> +			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
> +					 time_diff.tv_nsec / NSEC_PER_MSEC;
> +			if (time_to_sleep < 0)
> +				time_to_sleep = 0;

could this computation go to a separate function? something like:

time_to_sleep = compute_tts(time_start, time_stop);

thanks,
jirka

Re: [PATCH v9 02/15] tools/libperf: add properties to struct pollfd *entries objects

[Alexey Budankov]

On 06.07.2020 15:24, Jiri Olsa wrote:
> On Fri, Jul 03, 2020 at 10:41:45AM +0300, Alexey Budankov wrote:
>>
>> Store boolean properties per struct pollfd *entries object in a
>> bitmap of int size. Implement fdarray_prop__nonfilterable property
>> to skip object from counting by fdarray_filter().
> 
> ok, I think can do it like this, few comments below
> 
> thanks,
> jirka
> 
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  tools/lib/api/fd/array.c                 | 17 +++++++++--------
>>  tools/lib/api/fd/array.h                 | 18 +++++++++++++-----
>>  tools/lib/perf/evlist.c                  | 10 +++++-----
>>  tools/lib/perf/include/internal/evlist.h |  2 +-
>>  tools/perf/tests/fdarray.c               |  2 +-
>>  tools/perf/util/evlist.c                 |  2 +-
>>  6 files changed, 30 insertions(+), 21 deletions(-)

<SNIP>

>>
>> diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
>> index b39557d1a88f..19b6a34aeea5 100644
>> --- a/tools/lib/api/fd/array.h
>> +++ b/tools/lib/api/fd/array.h
>> @@ -21,10 +21,18 @@ struct fdarray {
>>  	int	       nr_alloc;
>>  	int	       nr_autogrow;
>>  	struct pollfd *entries;
>> -	union {
>> -		int    idx;
>> -		void   *ptr;
>> -	} *priv;
>> +	struct {
>> +		union {
>> +			int    idx;
>> +			void   *ptr;
>> +		} priv;
>> +		int bits;
>> +	} *prop;
>> +};
> 
> why not keeping the 'priv' as a struct, like:
> 
> 	struct {
> 		union {
> 			int    idx;
> 			void   *ptr;
> 		};
> 		unsigned int flags;
> 	} *priv;
> 
> I think we would have much less changes, also please rename bits
> to flags and use some unsigned type for that

Well, I supposed that priv is short for private what means the layout
of struct priv object is opaque to fdarray implementation and it just
passes the object as a void pointer to external callbacks (e.g in __filter()).
So I preserved this semantics and wrapped and extended priv object
with with flags field. It can be implemented with struct priv if you like.

> 
>> +
>> +enum fdarray_props {
>> +	fdarray_prop__default	    = 0x00000000,
>> +	fdarray_prop__nonfilterable = 0x00000001
>>  };
> 
> s/fdarray_props/fdarray_flag/

Accepted.

Alexey

Re: [PATCH v9 09/15] perf stat: factor out event handling loop into dispatch_events()

[Alexey Budankov]

On 06.07.2020 15:27, Jiri Olsa wrote:
> On Fri, Jul 03, 2020 at 10:46:15AM +0300, Alexey Budankov wrote:
>>
>> Consolidate event dispatching loops for fork, attach and system
>> wide monitoring use cases into common dispatch_events() function.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  tools/perf/builtin-stat.c | 42 +++++++++++++++++++++++++--------------
>>  1 file changed, 27 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 3e11f854ffc8..723f1fe27d63 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -550,6 +550,30 @@ static bool is_target_alive(struct target *_target,
>>  	return false;
>>  }
>>  
>> +static int dispatch_events(bool forks, int timeout, int interval, int *times, struct timespec *ts)
>> +{
>> +	bool stop = false;
>> +	int child_exited = 0, status = 0;
>> +
>> +	while (1) {
>> +		if (forks)
>> +			child_exited = waitpid(child_pid, &status, WNOHANG);
>> +		else
>> +			child_exited = !is_target_alive(&target, evsel_list->core.threads) ? 1 : 0;
>> +
>> +		if (done || stop || child_exited)
>> +			break;
> 
> can (done || stop) be in the while condition and
> we'd check just child_exited in here?
> 
>> +
>> +		nanosleep(ts, NULL);
>> +		if (timeout)
>> +			stop = true;
> 
> can we just break out in here? like the original code?
> I don't think we need the extra iteration
> 
>> +		else
>> +			stop = handle_interval(interval, times);
> 
> same here..?

Accepted. In v10.

Alexey

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Alexey Budankov]

On 06.07.2020 15:37, Jiri Olsa wrote:
> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>>  
>>  	while (1) {
>>  		if (forks)
>> @@ -574,11 +610,22 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times, st
>>  		if (done || stop || child_exited)
>>  			break;
>>  
>> -		nanosleep(ts, NULL);
>> -		if (timeout)
>> -			stop = true;
>> -		else
>> -			stop = handle_interval(interval, times);
>> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
>> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
>> +			if (timeout)
>> +				stop = true;
>> +			else
>> +				stop = handle_interval(interval, times);
>> +			time_to_sleep = sleep_time;
>> +		} else { /* fd revent */
>> +			stop = process_evlist(evsel_list, interval, times);
>> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
>> +			diff_timespec(&time_diff, &time_stop, &time_start);
>> +			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
>> +					 time_diff.tv_nsec / NSEC_PER_MSEC;
>> +			if (time_to_sleep < 0)
>> +				time_to_sleep = 0;
> 
> could this computation go to a separate function? something like:
> 
> time_to_sleep = compute_tts(time_start, time_stop);

Accepted. In v10.

Alexey

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Alexey Budankov]

On 06.07.2020 15:34, Jiri Olsa wrote:
> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
>>
>> Implement handling of 'enable' and 'disable' control commands
>> coming from control file descriptor. process_evlist() function
>> checks for events on control fds and makes required operations.
>> If poll event splits initiated timeout interval then the reminder
>> is calculated and still waited in the following poll() syscall.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 55 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 9e4288ecf2b8..5021f7286422 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
>>  	return false;
>>  }
>>  
>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>> +{
>> +	bool stop = false;
>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>> +
>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>> +		switch (cmd) {
>> +		case EVLIST_CTL_CMD_ENABLE:
>> +			pr_info(EVLIST_ENABLED_MSG);
>> +			stop = handle_interval(interval, times);
>> +			break;
>> +		case EVLIST_CTL_CMD_DISABLE:
>> +			stop = handle_interval(interval, times);
> 
> I still don't understand why you call handle_interval in here
> 
> I don't see it being necessary.. you enable events and handle_interval,
> wil be called in the next iteration of dispatch_events, why complicate
> this function with that?

Printing event counts at the moment of command processing lets scripts
built on top of stat output to provide more plain and accurate metrics.
Otherwise it may get spikes in the beginning of the next time interval
because not all counts lay inside [Events enabled, Events disable]
If -I interval is large tail event count can be also large. Compare the
output below with the output in the cover letter. Either way is possible
but the latter one likely complicates the scripts I mentioned above.

perf=tools/perf/perf
${perf} stat -D -1 -e cpu-cycles -a -I 1000       \
             --control fd:${ctl_fd},${ctl_fd_ack} \
             -- sleep 40 &

Events disabled
#           time             counts unit events
     1.001100723      <not counted>      cpu-cycles                                                  
     2.003146566      <not counted>      cpu-cycles                                                  
     3.005073317      <not counted>      cpu-cycles                                                  
     4.006337062      <not counted>      cpu-cycles                                                  
Events enabled
enable acked(ack)
     5.011182000         54,128,692      cpu-cycles <===                                                 
     6.012300167      3,648,804,827      cpu-cycles                                                  
     7.013631689        590,438,536      cpu-cycles                                                  
     8.015558583        406,935,663      cpu-cycles                                                  
     9.017455505        407,806,862      cpu-cycles                                                  
    10.019300780        399,351,824      cpu-cycles                                                  
    11.021180025        404,584,417      cpu-cycles                                                  
    12.023033661        537,787,981      cpu-cycles                                                  
    13.024422354        699,395,364      cpu-cycles                                                  
    14.026325749        397,871,324      cpu-cycles                                                  
disable acked()
Events disabled
    15.027857981        396,956,159      cpu-cycles <===
    16.029279264      <not counted>      cpu-cycles                                                  
    17.031131311      <not counted>      cpu-cycles                                                  
    18.033010580      <not counted>      cpu-cycles                                                  
    19.034918883      <not counted>      cpu-cycles                                                  
enable acked(ack)
Events enabled
    20.036758793        183,544,975      cpu-cycles <===                                             
    21.038163289        419,054,544      cpu-cycles                                                  
    22.040108245        413,993,309      cpu-cycles                                                  
    23.042042365        403,584,493      cpu-cycles                                                  
    24.043985381        416,512,094      cpu-cycles                                                  
    25.045925682        401,513,429      cpu-cycles                                                  
#           time             counts unit events
    26.047822238        461,205,096      cpu-cycles                                                  
    27.049784263        414,319,162      cpu-cycles                                                  
    28.051745360        403,706,915      cpu-cycles                                                  
    29.053674600        416,502,883      cpu-cycles                                                  
disable acked()
Events disabled
    30.054750685        414,184,409      cpu-cycles <===

Alexey

> 
> thanks,
> jirka
> 

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Jiri Olsa]

On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
> 
> On 06.07.2020 15:34, Jiri Olsa wrote:
> > On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
> >>
> >> Implement handling of 'enable' and 'disable' control commands
> >> coming from control file descriptor. process_evlist() function
> >> checks for events on control fds and makes required operations.
> >> If poll event splits initiated timeout interval then the reminder
> >> is calculated and still waited in the following poll() syscall.
> >>
> >> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> >> ---
> >>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
> >>  1 file changed, 55 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >> index 9e4288ecf2b8..5021f7286422 100644
> >> --- a/tools/perf/builtin-stat.c
> >> +++ b/tools/perf/builtin-stat.c
> >> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
> >>  	return false;
> >>  }
> >>  
> >> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> >> +{
> >> +	bool stop = false;
> >> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> >> +
> >> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
> >> +		switch (cmd) {
> >> +		case EVLIST_CTL_CMD_ENABLE:
> >> +			pr_info(EVLIST_ENABLED_MSG);
> >> +			stop = handle_interval(interval, times);
> >> +			break;
> >> +		case EVLIST_CTL_CMD_DISABLE:
> >> +			stop = handle_interval(interval, times);
> > 
> > I still don't understand why you call handle_interval in here
> > 
> > I don't see it being necessary.. you enable events and handle_interval,
> > wil be called in the next iteration of dispatch_events, why complicate
> > this function with that?
> 
> Printing event counts at the moment of command processing lets scripts
> built on top of stat output to provide more plain and accurate metrics.
> Otherwise it may get spikes in the beginning of the next time interval
> because not all counts lay inside [Events enabled, Events disable]
> If -I interval is large tail event count can be also large. Compare the
> output below with the output in the cover letter. Either way is possible
> but the latter one likely complicates the scripts I mentioned above.
> 
> perf=tools/perf/perf
> ${perf} stat -D -1 -e cpu-cycles -a -I 1000       \
>              --control fd:${ctl_fd},${ctl_fd_ack} \
>              -- sleep 40 &
> 
> Events disabled
> #           time             counts unit events
>      1.001100723      <not counted>      cpu-cycles                                                  
>      2.003146566      <not counted>      cpu-cycles                                                  
>      3.005073317      <not counted>      cpu-cycles                                                  
>      4.006337062      <not counted>      cpu-cycles                                                  
> Events enabled
> enable acked(ack)
>      5.011182000         54,128,692      cpu-cycles <===                                                 
>      6.012300167      3,648,804,827      cpu-cycles                                                  
>      7.013631689        590,438,536      cpu-cycles                                                  
>      8.015558583        406,935,663      cpu-cycles                                                  
>      9.017455505        407,806,862      cpu-cycles                                                  
>     10.019300780        399,351,824      cpu-cycles                                                  
>     11.021180025        404,584,417      cpu-cycles                                                  
>     12.023033661        537,787,981      cpu-cycles                                                  
>     13.024422354        699,395,364      cpu-cycles                                                  
>     14.026325749        397,871,324      cpu-cycles                                                  
> disable acked()
> Events disabled
>     15.027857981        396,956,159      cpu-cycles <===
>     16.029279264      <not counted>      cpu-cycles                                                  
>     17.031131311      <not counted>      cpu-cycles                                                  
>     18.033010580      <not counted>      cpu-cycles                                                  
>     19.034918883      <not counted>      cpu-cycles                                                  
> enable acked(ack)
> Events enabled
>     20.036758793        183,544,975      cpu-cycles <===                                             
>     21.038163289        419,054,544      cpu-cycles                                                  
>     22.040108245        413,993,309      cpu-cycles                                                  
>     23.042042365        403,584,493      cpu-cycles                                                  
>     24.043985381        416,512,094      cpu-cycles                                                  
>     25.045925682        401,513,429      cpu-cycles                                                  
> #           time             counts unit events
>     26.047822238        461,205,096      cpu-cycles                                                  
>     27.049784263        414,319,162      cpu-cycles                                                  
>     28.051745360        403,706,915      cpu-cycles                                                  
>     29.053674600        416,502,883      cpu-cycles                                                  
> disable acked()
> Events disabled
>     30.054750685        414,184,409      cpu-cycles <===

ok, but we could still take handle_interval out of process_evlist
and the interval process will be more clear for me (with some
additional comments in the code) ... perhaps something like below?

thanks,
jirka


---
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 5021f7286422..af83bf6b2db0 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -485,19 +485,18 @@ static bool handle_interval(unsigned int interval, int *times)
 	return false;
 }
 
-static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
+static bool process_evlist(struct evlist *evlist)
 {
-	bool stop = false;
 	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
+	bool enabled = false;
 
 	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
 		switch (cmd) {
 		case EVLIST_CTL_CMD_ENABLE:
 			pr_info(EVLIST_ENABLED_MSG);
-			stop = handle_interval(interval, times);
+			enabled = true;
 			break;
 		case EVLIST_CTL_CMD_DISABLE:
-			stop = handle_interval(interval, times);
 			pr_info(EVLIST_DISABLED_MSG);
 			break;
 		case EVLIST_CTL_CMD_ACK:
@@ -507,7 +506,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
 		}
 	}
 
-	return stop;
+	return enabled;
 }
 
 static void enable_counters(void)
@@ -618,7 +617,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
 				stop = handle_interval(interval, times);
 			time_to_sleep = sleep_time;
 		} else { /* fd revent */
-			stop = process_evlist(evsel_list, interval, times);
+			if (process_evlist(evsel_list))
+				stop = handle_interval(interval, times);
 			clock_gettime(CLOCK_MONOTONIC, &time_stop);
 			diff_timespec(&time_diff, &time_stop, &time_start);
 			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Alexey Budankov]

On 06.07.2020 22:34, Jiri Olsa wrote:
> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>>
>> On 06.07.2020 15:34, Jiri Olsa wrote:
>>> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
>>>>
>>>> Implement handling of 'enable' and 'disable' control commands
>>>> coming from control file descriptor. process_evlist() function
>>>> checks for events on control fds and makes required operations.
>>>> If poll event splits initiated timeout interval then the reminder
>>>> is calculated and still waited in the following poll() syscall.
>>>>
>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>> ---
>>>>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
>>>>  1 file changed, 55 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>> index 9e4288ecf2b8..5021f7286422 100644
>>>> --- a/tools/perf/builtin-stat.c
>>>> +++ b/tools/perf/builtin-stat.c
>>>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
>>>>  	return false;
>>>>  }
>>>>  
>>>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>>>> +{
>>>> +	bool stop = false;
>>>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>>>> +
>>>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>>>> +		switch (cmd) {
>>>> +		case EVLIST_CTL_CMD_ENABLE:
>>>> +			pr_info(EVLIST_ENABLED_MSG);
>>>> +			stop = handle_interval(interval, times);
>>>> +			break;
>>>> +		case EVLIST_CTL_CMD_DISABLE:
>>>> +			stop = handle_interval(interval, times);
>>>
>>> I still don't understand why you call handle_interval in here
>>>
>>> I don't see it being necessary.. you enable events and handle_interval,
>>> wil be called in the next iteration of dispatch_events, why complicate
>>> this function with that?
>>
>> Printing event counts at the moment of command processing lets scripts
>> built on top of stat output to provide more plain and accurate metrics.
>> Otherwise it may get spikes in the beginning of the next time interval
>> because not all counts lay inside [Events enabled, Events disable]
>> If -I interval is large tail event count can be also large. Compare the
>> output below with the output in the cover letter. Either way is possible
>> but the latter one likely complicates the scripts I mentioned above.
>>
>> perf=tools/perf/perf
>> ${perf} stat -D -1 -e cpu-cycles -a -I 1000       \
>>              --control fd:${ctl_fd},${ctl_fd_ack} \
>>              -- sleep 40 &
>>
>> Events disabled
>> #           time             counts unit events
>>      1.001100723      <not counted>      cpu-cycles                                                  
>>      2.003146566      <not counted>      cpu-cycles                                                  
>>      3.005073317      <not counted>      cpu-cycles                                                  
>>      4.006337062      <not counted>      cpu-cycles                                                  
>> Events enabled
>> enable acked(ack)
>>      5.011182000         54,128,692      cpu-cycles <===                                                 
>>      6.012300167      3,648,804,827      cpu-cycles                                                  
>>      7.013631689        590,438,536      cpu-cycles                                                  
>>      8.015558583        406,935,663      cpu-cycles                                                  
>>      9.017455505        407,806,862      cpu-cycles                                                  
>>     10.019300780        399,351,824      cpu-cycles                                                  
>>     11.021180025        404,584,417      cpu-cycles                                                  
>>     12.023033661        537,787,981      cpu-cycles                                                  
>>     13.024422354        699,395,364      cpu-cycles                                                  
>>     14.026325749        397,871,324      cpu-cycles                                                  
>> disable acked()
>> Events disabled
>>     15.027857981        396,956,159      cpu-cycles <===
>>     16.029279264      <not counted>      cpu-cycles                                                  
>>     17.031131311      <not counted>      cpu-cycles                                                  
>>     18.033010580      <not counted>      cpu-cycles                                                  
>>     19.034918883      <not counted>      cpu-cycles                                                  
>> enable acked(ack)
>> Events enabled
>>     20.036758793        183,544,975      cpu-cycles <===                                             
>>     21.038163289        419,054,544      cpu-cycles                                                  
>>     22.040108245        413,993,309      cpu-cycles                                                  
>>     23.042042365        403,584,493      cpu-cycles                                                  
>>     24.043985381        416,512,094      cpu-cycles                                                  
>>     25.045925682        401,513,429      cpu-cycles                                                  
>> #           time             counts unit events
>>     26.047822238        461,205,096      cpu-cycles                                                  
>>     27.049784263        414,319,162      cpu-cycles                                                  
>>     28.051745360        403,706,915      cpu-cycles                                                  
>>     29.053674600        416,502,883      cpu-cycles                                                  
>> disable acked()
>> Events disabled
>>     30.054750685        414,184,409      cpu-cycles <===
> 
> ok, but we could still take handle_interval out of process_evlist
> and the interval process will be more clear for me (with some
> additional comments in the code) ... perhaps something like below?
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 5021f7286422..af83bf6b2db0 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -485,19 +485,18 @@ static bool handle_interval(unsigned int interval, int *times)
>  	return false;
>  }
>  
> -static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> +static bool process_evlist(struct evlist *evlist)
>  {
> -	bool stop = false;
>  	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> +	bool enabled = false;
>  
>  	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>  		switch (cmd) {
>  		case EVLIST_CTL_CMD_ENABLE:
>  			pr_info(EVLIST_ENABLED_MSG);
> -			stop = handle_interval(interval, times);
> +			enabled = true;
>  			break;
>  		case EVLIST_CTL_CMD_DISABLE:
> -			stop = handle_interval(interval, times);
>  			pr_info(EVLIST_DISABLED_MSG);
>  			break;
>  		case EVLIST_CTL_CMD_ACK:
> @@ -507,7 +506,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
>  		}
>  	}
>  
> -	return stop;
> +	return enabled;
>  }
>  
>  static void enable_counters(void)
> @@ -618,7 +617,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
>  				stop = handle_interval(interval, times);
>  			time_to_sleep = sleep_time;
>  		} else { /* fd revent */
> -			stop = process_evlist(evsel_list, interval, times);
> +			if (process_evlist(evsel_list))
> +				stop = handle_interval(interval, times);

It will call only on enable command and lead to artificial spikes in the beginning of interval.
May be just take handle_interval() out of process_evlist() and have it similar to record case?

Alexey

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Jiri Olsa]

On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
> 
> On 06.07.2020 22:34, Jiri Olsa wrote:
> > On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
> >>
> >> On 06.07.2020 15:34, Jiri Olsa wrote:
> >>> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
> >>>>
> >>>> Implement handling of 'enable' and 'disable' control commands
> >>>> coming from control file descriptor. process_evlist() function
> >>>> checks for events on control fds and makes required operations.
> >>>> If poll event splits initiated timeout interval then the reminder
> >>>> is calculated and still waited in the following poll() syscall.
> >>>>
> >>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> >>>> ---
> >>>>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
> >>>>  1 file changed, 55 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >>>> index 9e4288ecf2b8..5021f7286422 100644
> >>>> --- a/tools/perf/builtin-stat.c
> >>>> +++ b/tools/perf/builtin-stat.c
> >>>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
> >>>>  	return false;
> >>>>  }
> >>>>  
> >>>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> >>>> +{
> >>>> +	bool stop = false;
> >>>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> >>>> +
> >>>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
> >>>> +		switch (cmd) {
> >>>> +		case EVLIST_CTL_CMD_ENABLE:
> >>>> +			pr_info(EVLIST_ENABLED_MSG);
> >>>> +			stop = handle_interval(interval, times);
> >>>> +			break;
> >>>> +		case EVLIST_CTL_CMD_DISABLE:
> >>>> +			stop = handle_interval(interval, times);
> >>>
> >>> I still don't understand why you call handle_interval in here
> >>>
> >>> I don't see it being necessary.. you enable events and handle_interval,
> >>> wil be called in the next iteration of dispatch_events, why complicate
> >>> this function with that?
> >>
> >> Printing event counts at the moment of command processing lets scripts
> >> built on top of stat output to provide more plain and accurate metrics.
> >> Otherwise it may get spikes in the beginning of the next time interval
> >> because not all counts lay inside [Events enabled, Events disable]
> >> If -I interval is large tail event count can be also large. Compare the
> >> output below with the output in the cover letter. Either way is possible
> >> but the latter one likely complicates the scripts I mentioned above.
> >>
> >> perf=tools/perf/perf
> >> ${perf} stat -D -1 -e cpu-cycles -a -I 1000       \
> >>              --control fd:${ctl_fd},${ctl_fd_ack} \
> >>              -- sleep 40 &
> >>
> >> Events disabled
> >> #           time             counts unit events
> >>      1.001100723      <not counted>      cpu-cycles                                                  
> >>      2.003146566      <not counted>      cpu-cycles                                                  
> >>      3.005073317      <not counted>      cpu-cycles                                                  
> >>      4.006337062      <not counted>      cpu-cycles                                                  
> >> Events enabled
> >> enable acked(ack)
> >>      5.011182000         54,128,692      cpu-cycles <===                                                 
> >>      6.012300167      3,648,804,827      cpu-cycles                                                  
> >>      7.013631689        590,438,536      cpu-cycles                                                  
> >>      8.015558583        406,935,663      cpu-cycles                                                  
> >>      9.017455505        407,806,862      cpu-cycles                                                  
> >>     10.019300780        399,351,824      cpu-cycles                                                  
> >>     11.021180025        404,584,417      cpu-cycles                                                  
> >>     12.023033661        537,787,981      cpu-cycles                                                  
> >>     13.024422354        699,395,364      cpu-cycles                                                  
> >>     14.026325749        397,871,324      cpu-cycles                                                  
> >> disable acked()
> >> Events disabled
> >>     15.027857981        396,956,159      cpu-cycles <===
> >>     16.029279264      <not counted>      cpu-cycles                                                  
> >>     17.031131311      <not counted>      cpu-cycles                                                  
> >>     18.033010580      <not counted>      cpu-cycles                                                  
> >>     19.034918883      <not counted>      cpu-cycles                                                  
> >> enable acked(ack)
> >> Events enabled
> >>     20.036758793        183,544,975      cpu-cycles <===                                             
> >>     21.038163289        419,054,544      cpu-cycles                                                  
> >>     22.040108245        413,993,309      cpu-cycles                                                  
> >>     23.042042365        403,584,493      cpu-cycles                                                  
> >>     24.043985381        416,512,094      cpu-cycles                                                  
> >>     25.045925682        401,513,429      cpu-cycles                                                  
> >> #           time             counts unit events
> >>     26.047822238        461,205,096      cpu-cycles                                                  
> >>     27.049784263        414,319,162      cpu-cycles                                                  
> >>     28.051745360        403,706,915      cpu-cycles                                                  
> >>     29.053674600        416,502,883      cpu-cycles                                                  
> >> disable acked()
> >> Events disabled
> >>     30.054750685        414,184,409      cpu-cycles <===
> > 
> > ok, but we could still take handle_interval out of process_evlist
> > and the interval process will be more clear for me (with some
> > additional comments in the code) ... perhaps something like below?
> > 
> > thanks,
> > jirka
> > 
> > 
> > ---
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 5021f7286422..af83bf6b2db0 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -485,19 +485,18 @@ static bool handle_interval(unsigned int interval, int *times)
> >  	return false;
> >  }
> >  
> > -static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> > +static bool process_evlist(struct evlist *evlist)
> >  {
> > -	bool stop = false;
> >  	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> > +	bool enabled = false;
> >  
> >  	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
> >  		switch (cmd) {
> >  		case EVLIST_CTL_CMD_ENABLE:
> >  			pr_info(EVLIST_ENABLED_MSG);
> > -			stop = handle_interval(interval, times);
> > +			enabled = true;
> >  			break;
> >  		case EVLIST_CTL_CMD_DISABLE:
> > -			stop = handle_interval(interval, times);
> >  			pr_info(EVLIST_DISABLED_MSG);
> >  			break;
> >  		case EVLIST_CTL_CMD_ACK:
> > @@ -507,7 +506,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
> >  		}
> >  	}
> >  
> > -	return stop;
> > +	return enabled;
> >  }
> >  
> >  static void enable_counters(void)
> > @@ -618,7 +617,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
> >  				stop = handle_interval(interval, times);
> >  			time_to_sleep = sleep_time;
> >  		} else { /* fd revent */
> > -			stop = process_evlist(evsel_list, interval, times);
> > +			if (process_evlist(evsel_list))
> > +				stop = handle_interval(interval, times);
> 
> It will call only on enable command and lead to artificial spikes in the beginning of interval.
> May be just take handle_interval() out of process_evlist() and have it similar to record case?

it can be called also for disable case then

jirka

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Alexey Budankov]

On 07.07.2020 16:14, Jiri Olsa wrote:
> On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
>>
>> On 06.07.2020 22:34, Jiri Olsa wrote:
>>> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>>>>
>>>> On 06.07.2020 15:34, Jiri Olsa wrote:
>>>>> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
>>>>>>
>>>>>> Implement handling of 'enable' and 'disable' control commands
>>>>>> coming from control file descriptor. process_evlist() function
>>>>>> checks for events on control fds and makes required operations.
>>>>>> If poll event splits initiated timeout interval then the reminder
>>>>>> is calculated and still waited in the following poll() syscall.
>>>>>>
>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>> ---
>>>>>>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
>>>>>>  1 file changed, 55 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>>>> index 9e4288ecf2b8..5021f7286422 100644
>>>>>> --- a/tools/perf/builtin-stat.c
>>>>>> +++ b/tools/perf/builtin-stat.c
>>>>>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
>>>>>>  	return false;
>>>>>>  }
>>>>>>  
>>>>>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>>>>>> +{
>>>>>> +	bool stop = false;
>>>>>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>>>>>> +
>>>>>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>>>>>> +		switch (cmd) {
>>>>>> +		case EVLIST_CTL_CMD_ENABLE:
>>>>>> +			pr_info(EVLIST_ENABLED_MSG);
>>>>>> +			stop = handle_interval(interval, times);
>>>>>> +			break;
>>>>>> +		case EVLIST_CTL_CMD_DISABLE:
>>>>>> +			stop = handle_interval(interval, times);
>>>>>
>>>>> I still don't understand why you call handle_interval in here
>>>>>
>>>>> I don't see it being necessary.. you enable events and handle_interval,
>>>>> wil be called in the next iteration of dispatch_events, why complicate
>>>>> this function with that?
>>>>
>>>> Printing event counts at the moment of command processing lets scripts
>>>> built on top of stat output to provide more plain and accurate metrics.
>>>> Otherwise it may get spikes in the beginning of the next time interval
>>>> because not all counts lay inside [Events enabled, Events disable]
>>>> If -I interval is large tail event count can be also large. Compare the
>>>> output below with the output in the cover letter. Either way is possible
>>>> but the latter one likely complicates the scripts I mentioned above.
>>>>
>>>> perf=tools/perf/perf
>>>> ${perf} stat -D -1 -e cpu-cycles -a -I 1000       \
>>>>              --control fd:${ctl_fd},${ctl_fd_ack} \
>>>>              -- sleep 40 &
>>>>
>>>> Events disabled
>>>> #           time             counts unit events
>>>>      1.001100723      <not counted>      cpu-cycles                                                  
>>>>      2.003146566      <not counted>      cpu-cycles                                                  
>>>>      3.005073317      <not counted>      cpu-cycles                                                  
>>>>      4.006337062      <not counted>      cpu-cycles                                                  
>>>> Events enabled
>>>> enable acked(ack)
>>>>      5.011182000         54,128,692      cpu-cycles <===                                                 
>>>>      6.012300167      3,648,804,827      cpu-cycles                                                  
>>>>      7.013631689        590,438,536      cpu-cycles                                                  
>>>>      8.015558583        406,935,663      cpu-cycles                                                  
>>>>      9.017455505        407,806,862      cpu-cycles                                                  
>>>>     10.019300780        399,351,824      cpu-cycles                                                  
>>>>     11.021180025        404,584,417      cpu-cycles                                                  
>>>>     12.023033661        537,787,981      cpu-cycles                                                  
>>>>     13.024422354        699,395,364      cpu-cycles                                                  
>>>>     14.026325749        397,871,324      cpu-cycles                                                  
>>>> disable acked()
>>>> Events disabled
>>>>     15.027857981        396,956,159      cpu-cycles <===
>>>>     16.029279264      <not counted>      cpu-cycles                                                  
>>>>     17.031131311      <not counted>      cpu-cycles                                                  
>>>>     18.033010580      <not counted>      cpu-cycles                                                  
>>>>     19.034918883      <not counted>      cpu-cycles                                                  
>>>> enable acked(ack)
>>>> Events enabled
>>>>     20.036758793        183,544,975      cpu-cycles <===                                             
>>>>     21.038163289        419,054,544      cpu-cycles                                                  
>>>>     22.040108245        413,993,309      cpu-cycles                                                  
>>>>     23.042042365        403,584,493      cpu-cycles                                                  
>>>>     24.043985381        416,512,094      cpu-cycles                                                  
>>>>     25.045925682        401,513,429      cpu-cycles                                                  
>>>> #           time             counts unit events
>>>>     26.047822238        461,205,096      cpu-cycles                                                  
>>>>     27.049784263        414,319,162      cpu-cycles                                                  
>>>>     28.051745360        403,706,915      cpu-cycles                                                  
>>>>     29.053674600        416,502,883      cpu-cycles                                                  
>>>> disable acked()
>>>> Events disabled
>>>>     30.054750685        414,184,409      cpu-cycles <===
>>>
>>> ok, but we could still take handle_interval out of process_evlist
>>> and the interval process will be more clear for me (with some
>>> additional comments in the code) ... perhaps something like below?
>>>
>>> thanks,
>>> jirka
>>>
>>>
>>> ---
>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>> index 5021f7286422..af83bf6b2db0 100644
>>> --- a/tools/perf/builtin-stat.c
>>> +++ b/tools/perf/builtin-stat.c
>>> @@ -485,19 +485,18 @@ static bool handle_interval(unsigned int interval, int *times)
>>>  	return false;
>>>  }
>>>  
>>> -static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>>> +static bool process_evlist(struct evlist *evlist)
>>>  {
>>> -	bool stop = false;
>>>  	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>>> +	bool enabled = false;
>>>  
>>>  	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>>>  		switch (cmd) {
>>>  		case EVLIST_CTL_CMD_ENABLE:
>>>  			pr_info(EVLIST_ENABLED_MSG);
>>> -			stop = handle_interval(interval, times);
>>> +			enabled = true;
>>>  			break;
>>>  		case EVLIST_CTL_CMD_DISABLE:
>>> -			stop = handle_interval(interval, times);
>>>  			pr_info(EVLIST_DISABLED_MSG);
>>>  			break;
>>>  		case EVLIST_CTL_CMD_ACK:
>>> @@ -507,7 +506,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
>>>  		}
>>>  	}
>>>  
>>> -	return stop;
>>> +	return enabled;
>>>  }
>>>  
>>>  static void enable_counters(void)
>>> @@ -618,7 +617,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
>>>  				stop = handle_interval(interval, times);
>>>  			time_to_sleep = sleep_time;
>>>  		} else { /* fd revent */
>>> -			stop = process_evlist(evsel_list, interval, times);
>>> +			if (process_evlist(evsel_list))
>>> +				stop = handle_interval(interval, times);
>>
>> It will call only on enable command and lead to artificial spikes in the beginning of interval.
>> May be just take handle_interval() out of process_evlist() and have it similar to record case?
> 
> it can be called also for disable case then


Made it like this so now times counter is not affected during commands processing:

static void process_evlist(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
{
	if (evlist__ctlfd_process(evlist, cmd) > 0) {
		switch (*cmd) {
		case EVLIST_CTL_CMD_ENABLE:
			pr_info(EVLIST_ENABLED_MSG);
			break;
		case EVLIST_CTL_CMD_DISABLE:
			pr_info(EVLIST_DISABLED_MSG);
			break;
		case EVLIST_CTL_CMD_ACK:
		case EVLIST_CTL_CMD_UNSUPPORTED:
		default:
			break;
		}
	}
}

...

		clock_gettime(CLOCK_MONOTONIC, &time_start);
		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
			if (timeout)
				break;
			else
				stop = handle_interval(interval, times);
			time_to_sleep = sleep_time;
		} else { /* fd revent */
			process_evlist(evsel_list, &cmd);
			if (interval) {
				switch (cmd) {
				case EVLIST_CTL_CMD_ENABLE:
				case EVLIST_CTL_CMD_DISABLE:
					process_interval();
				case EVLIST_CTL_CMD_ACK:
				case EVLIST_CTL_CMD_UNSUPPORTED:
				default:
					break;
				}
			}
			clock_gettime(CLOCK_MONOTONIC, &time_stop);
			compute_tts(&time_start, &time_stop, &time_to_sleep);
		}

Alexey

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Jiri Olsa]

On Tue, Jul 07, 2020 at 04:24:28PM +0300, Alexey Budankov wrote:
> 
> On 07.07.2020 16:14, Jiri Olsa wrote:
> > On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
> >>
> >> On 06.07.2020 22:34, Jiri Olsa wrote:
> >>> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
> >>>>
> >>>> On 06.07.2020 15:34, Jiri Olsa wrote:
> >>>>> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
> >>>>>>
> >>>>>> Implement handling of 'enable' and 'disable' control commands
> >>>>>> coming from control file descriptor. process_evlist() function
> >>>>>> checks for events on control fds and makes required operations.
> >>>>>> If poll event splits initiated timeout interval then the reminder
> >>>>>> is calculated and still waited in the following poll() syscall.
> >>>>>>
> >>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> >>>>>> ---
> >>>>>>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
> >>>>>>  1 file changed, 55 insertions(+), 20 deletions(-)
> >>>>>>
> >>>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >>>>>> index 9e4288ecf2b8..5021f7286422 100644
> >>>>>> --- a/tools/perf/builtin-stat.c
> >>>>>> +++ b/tools/perf/builtin-stat.c
> >>>>>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
> >>>>>>  	return false;
> >>>>>>  }
> >>>>>>  
> >>>>>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> >>>>>> +{
> >>>>>> +	bool stop = false;
> >>>>>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> >>>>>> +
> >>>>>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
> >>>>>> +		switch (cmd) {
> >>>>>> +		case EVLIST_CTL_CMD_ENABLE:
> >>>>>> +			pr_info(EVLIST_ENABLED_MSG);
> >>>>>> +			stop = handle_interval(interval, times);
> >>>>>> +			break;
> >>>>>> +		case EVLIST_CTL_CMD_DISABLE:
> >>>>>> +			stop = handle_interval(interval, times);
> >>>>>
> >>>>> I still don't understand why you call handle_interval in here
> >>>>>
> >>>>> I don't see it being necessary.. you enable events and handle_interval,
> >>>>> wil be called in the next iteration of dispatch_events, why complicate
> >>>>> this function with that?
> >>>>
> >>>> Printing event counts at the moment of command processing lets scripts
> >>>> built on top of stat output to provide more plain and accurate metrics.
> >>>> Otherwise it may get spikes in the beginning of the next time interval
> >>>> because not all counts lay inside [Events enabled, Events disable]
> >>>> If -I interval is large tail event count can be also large. Compare the
> >>>> output below with the output in the cover letter. Either way is possible
> >>>> but the latter one likely complicates the scripts I mentioned above.
> >>>>
> >>>> perf=tools/perf/perf
> >>>> ${perf} stat -D -1 -e cpu-cycles -a -I 1000       \
> >>>>              --control fd:${ctl_fd},${ctl_fd_ack} \
> >>>>              -- sleep 40 &
> >>>>
> >>>> Events disabled
> >>>> #           time             counts unit events
> >>>>      1.001100723      <not counted>      cpu-cycles                                                  
> >>>>      2.003146566      <not counted>      cpu-cycles                                                  
> >>>>      3.005073317      <not counted>      cpu-cycles                                                  
> >>>>      4.006337062      <not counted>      cpu-cycles                                                  
> >>>> Events enabled
> >>>> enable acked(ack)
> >>>>      5.011182000         54,128,692      cpu-cycles <===                                                 
> >>>>      6.012300167      3,648,804,827      cpu-cycles                                                  
> >>>>      7.013631689        590,438,536      cpu-cycles                                                  
> >>>>      8.015558583        406,935,663      cpu-cycles                                                  
> >>>>      9.017455505        407,806,862      cpu-cycles                                                  
> >>>>     10.019300780        399,351,824      cpu-cycles                                                  
> >>>>     11.021180025        404,584,417      cpu-cycles                                                  
> >>>>     12.023033661        537,787,981      cpu-cycles                                                  
> >>>>     13.024422354        699,395,364      cpu-cycles                                                  
> >>>>     14.026325749        397,871,324      cpu-cycles                                                  
> >>>> disable acked()
> >>>> Events disabled
> >>>>     15.027857981        396,956,159      cpu-cycles <===
> >>>>     16.029279264      <not counted>      cpu-cycles                                                  
> >>>>     17.031131311      <not counted>      cpu-cycles                                                  
> >>>>     18.033010580      <not counted>      cpu-cycles                                                  
> >>>>     19.034918883      <not counted>      cpu-cycles                                                  
> >>>> enable acked(ack)
> >>>> Events enabled
> >>>>     20.036758793        183,544,975      cpu-cycles <===                                             
> >>>>     21.038163289        419,054,544      cpu-cycles                                                  
> >>>>     22.040108245        413,993,309      cpu-cycles                                                  
> >>>>     23.042042365        403,584,493      cpu-cycles                                                  
> >>>>     24.043985381        416,512,094      cpu-cycles                                                  
> >>>>     25.045925682        401,513,429      cpu-cycles                                                  
> >>>> #           time             counts unit events
> >>>>     26.047822238        461,205,096      cpu-cycles                                                  
> >>>>     27.049784263        414,319,162      cpu-cycles                                                  
> >>>>     28.051745360        403,706,915      cpu-cycles                                                  
> >>>>     29.053674600        416,502,883      cpu-cycles                                                  
> >>>> disable acked()
> >>>> Events disabled
> >>>>     30.054750685        414,184,409      cpu-cycles <===
> >>>
> >>> ok, but we could still take handle_interval out of process_evlist
> >>> and the interval process will be more clear for me (with some
> >>> additional comments in the code) ... perhaps something like below?
> >>>
> >>> thanks,
> >>> jirka
> >>>
> >>>
> >>> ---
> >>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >>> index 5021f7286422..af83bf6b2db0 100644
> >>> --- a/tools/perf/builtin-stat.c
> >>> +++ b/tools/perf/builtin-stat.c
> >>> @@ -485,19 +485,18 @@ static bool handle_interval(unsigned int interval, int *times)
> >>>  	return false;
> >>>  }
> >>>  
> >>> -static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> >>> +static bool process_evlist(struct evlist *evlist)
> >>>  {
> >>> -	bool stop = false;
> >>>  	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> >>> +	bool enabled = false;
> >>>  
> >>>  	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
> >>>  		switch (cmd) {
> >>>  		case EVLIST_CTL_CMD_ENABLE:
> >>>  			pr_info(EVLIST_ENABLED_MSG);
> >>> -			stop = handle_interval(interval, times);
> >>> +			enabled = true;
> >>>  			break;
> >>>  		case EVLIST_CTL_CMD_DISABLE:
> >>> -			stop = handle_interval(interval, times);
> >>>  			pr_info(EVLIST_DISABLED_MSG);
> >>>  			break;
> >>>  		case EVLIST_CTL_CMD_ACK:
> >>> @@ -507,7 +506,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
> >>>  		}
> >>>  	}
> >>>  
> >>> -	return stop;
> >>> +	return enabled;
> >>>  }
> >>>  
> >>>  static void enable_counters(void)
> >>> @@ -618,7 +617,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
> >>>  				stop = handle_interval(interval, times);
> >>>  			time_to_sleep = sleep_time;
> >>>  		} else { /* fd revent */
> >>> -			stop = process_evlist(evsel_list, interval, times);
> >>> +			if (process_evlist(evsel_list))
> >>> +				stop = handle_interval(interval, times);
> >>
> >> It will call only on enable command and lead to artificial spikes in the beginning of interval.
> >> May be just take handle_interval() out of process_evlist() and have it similar to record case?
> > 
> > it can be called also for disable case then
> 
> 
> Made it like this so now times counter is not affected during commands processing:

hm, can't process list just return true
when the interval needs to be printed?

jirka

> 
> static void process_evlist(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> {
> 	if (evlist__ctlfd_process(evlist, cmd) > 0) {
> 		switch (*cmd) {
> 		case EVLIST_CTL_CMD_ENABLE:
> 			pr_info(EVLIST_ENABLED_MSG);
> 			break;
> 		case EVLIST_CTL_CMD_DISABLE:
> 			pr_info(EVLIST_DISABLED_MSG);
> 			break;
> 		case EVLIST_CTL_CMD_ACK:
> 		case EVLIST_CTL_CMD_UNSUPPORTED:
> 		default:
> 			break;
> 		}
> 	}
> }
> 
> ...
> 
> 		clock_gettime(CLOCK_MONOTONIC, &time_start);
> 		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
> 			if (timeout)
> 				break;
> 			else
> 				stop = handle_interval(interval, times);
> 			time_to_sleep = sleep_time;
> 		} else { /* fd revent */
> 			process_evlist(evsel_list, &cmd);
> 			if (interval) {
> 				switch (cmd) {
> 				case EVLIST_CTL_CMD_ENABLE:
> 				case EVLIST_CTL_CMD_DISABLE:
> 					process_interval();
> 				case EVLIST_CTL_CMD_ACK:
> 				case EVLIST_CTL_CMD_UNSUPPORTED:
> 				default:
> 					break;
> 				}
> 			}
> 			clock_gettime(CLOCK_MONOTONIC, &time_stop);
> 			compute_tts(&time_start, &time_stop, &time_to_sleep);
> 		}
> 
> Alexey
> 

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Alexey Budankov]

On 07.07.2020 17:23, Jiri Olsa wrote:
> On Tue, Jul 07, 2020 at 04:24:28PM +0300, Alexey Budankov wrote:
>>
>> On 07.07.2020 16:14, Jiri Olsa wrote:
>>> On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
>>>>
>>>> On 06.07.2020 22:34, Jiri Olsa wrote:
>>>>> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>>>>>>
>>>>>> On 06.07.2020 15:34, Jiri Olsa wrote:
>>>>>>> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
>>>>>>>>
>>>>>>>> Implement handling of 'enable' and 'disable' control commands
>>>>>>>> coming from control file descriptor. process_evlist() function
>>>>>>>> checks for events on control fds and makes required operations.
>>>>>>>> If poll event splits initiated timeout interval then the reminder
>>>>>>>> is calculated and still waited in the following poll() syscall.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>>>> ---
>>>>>>>>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
>>>>>>>>  1 file changed, 55 insertions(+), 20 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>>>>>> index 9e4288ecf2b8..5021f7286422 100644
>>>>>>>> --- a/tools/perf/builtin-stat.c
>>>>>>>> +++ b/tools/perf/builtin-stat.c
>>>>>>>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
>>>>>>>>  	return false;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>>>>>>>> +{
>>>>>>>> +	bool stop = false;
>>>>>>>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>>>>>>>> +
>>>>>>>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>>>>>>>> +		switch (cmd) {
>>>>>>>> +		case EVLIST_CTL_CMD_ENABLE:
>>>>>>>> +			pr_info(EVLIST_ENABLED_MSG);
>>>>>>>> +			stop = handle_interval(interval, times);
>>>>>>>> +			break;
>>>>>>>> +		case EVLIST_CTL_CMD_DISABLE:
>>>>>>>> +			stop = handle_interval(interval, times);
>>>>>>>
>>>>>>> I still don't understand why you call handle_interval in here
>>>>>>>
>>>>>>> I don't see it being necessary.. you enable events and handle_interval,
>>>>>>> wil be called in the next iteration of dispatch_events, why complicate
>>>>>>> this function with that?
>>>>>>
>>>>>> Printing event counts at the moment of command processing lets scripts
>>>>>> built on top of stat output to provide more plain and accurate metrics.
>>>>>> Otherwise it may get spikes in the beginning of the next time interval
>>>>>> because not all counts lay inside [Events enabled, Events disable]
>>>>>> If -I interval is large tail event count can be also large. Compare the
>>>>>> output below with the output in the cover letter. Either way is possible
>>>>>> but the latter one likely complicates the scripts I mentioned above.
>>>>>>
>>>>>> perf=tools/perf/perf
>>>>>> ${perf} stat -D -1 -e cpu-cycles -a -I 1000       \
>>>>>>              --control fd:${ctl_fd},${ctl_fd_ack} \
>>>>>>              -- sleep 40 &
>>>>>>
>>>>>> Events disabled
>>>>>> #           time             counts unit events
>>>>>>      1.001100723      <not counted>      cpu-cycles                                                  
>>>>>>      2.003146566      <not counted>      cpu-cycles                                                  
>>>>>>      3.005073317      <not counted>      cpu-cycles                                                  
>>>>>>      4.006337062      <not counted>      cpu-cycles                                                  
>>>>>> Events enabled
>>>>>> enable acked(ack)
>>>>>>      5.011182000         54,128,692      cpu-cycles <===                                                 
>>>>>>      6.012300167      3,648,804,827      cpu-cycles                                                  
>>>>>>      7.013631689        590,438,536      cpu-cycles                                                  
>>>>>>      8.015558583        406,935,663      cpu-cycles                                                  
>>>>>>      9.017455505        407,806,862      cpu-cycles                                                  
>>>>>>     10.019300780        399,351,824      cpu-cycles                                                  
>>>>>>     11.021180025        404,584,417      cpu-cycles                                                  
>>>>>>     12.023033661        537,787,981      cpu-cycles                                                  
>>>>>>     13.024422354        699,395,364      cpu-cycles                                                  
>>>>>>     14.026325749        397,871,324      cpu-cycles                                                  
>>>>>> disable acked()
>>>>>> Events disabled
>>>>>>     15.027857981        396,956,159      cpu-cycles <===
>>>>>>     16.029279264      <not counted>      cpu-cycles                                                  
>>>>>>     17.031131311      <not counted>      cpu-cycles                                                  
>>>>>>     18.033010580      <not counted>      cpu-cycles                                                  
>>>>>>     19.034918883      <not counted>      cpu-cycles                                                  
>>>>>> enable acked(ack)
>>>>>> Events enabled
>>>>>>     20.036758793        183,544,975      cpu-cycles <===                                             
>>>>>>     21.038163289        419,054,544      cpu-cycles                                                  
>>>>>>     22.040108245        413,993,309      cpu-cycles                                                  
>>>>>>     23.042042365        403,584,493      cpu-cycles                                                  
>>>>>>     24.043985381        416,512,094      cpu-cycles                                                  
>>>>>>     25.045925682        401,513,429      cpu-cycles                                                  
>>>>>> #           time             counts unit events
>>>>>>     26.047822238        461,205,096      cpu-cycles                                                  
>>>>>>     27.049784263        414,319,162      cpu-cycles                                                  
>>>>>>     28.051745360        403,706,915      cpu-cycles                                                  
>>>>>>     29.053674600        416,502,883      cpu-cycles                                                  
>>>>>> disable acked()
>>>>>> Events disabled
>>>>>>     30.054750685        414,184,409      cpu-cycles <===
>>>>>
>>>>> ok, but we could still take handle_interval out of process_evlist
>>>>> and the interval process will be more clear for me (with some
>>>>> additional comments in the code) ... perhaps something like below?
>>>>>
>>>>> thanks,
>>>>> jirka
>>>>>
>>>>>
>>>>> ---
>>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>>> index 5021f7286422..af83bf6b2db0 100644
>>>>> --- a/tools/perf/builtin-stat.c
>>>>> +++ b/tools/perf/builtin-stat.c
>>>>> @@ -485,19 +485,18 @@ static bool handle_interval(unsigned int interval, int *times)
>>>>>  	return false;
>>>>>  }
>>>>>  
>>>>> -static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>>>>> +static bool process_evlist(struct evlist *evlist)
>>>>>  {
>>>>> -	bool stop = false;
>>>>>  	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>>>>> +	bool enabled = false;
>>>>>  
>>>>>  	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>>>>>  		switch (cmd) {
>>>>>  		case EVLIST_CTL_CMD_ENABLE:
>>>>>  			pr_info(EVLIST_ENABLED_MSG);
>>>>> -			stop = handle_interval(interval, times);
>>>>> +			enabled = true;
>>>>>  			break;
>>>>>  		case EVLIST_CTL_CMD_DISABLE:
>>>>> -			stop = handle_interval(interval, times);
>>>>>  			pr_info(EVLIST_DISABLED_MSG);
>>>>>  			break;
>>>>>  		case EVLIST_CTL_CMD_ACK:
>>>>> @@ -507,7 +506,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
>>>>>  		}
>>>>>  	}
>>>>>  
>>>>> -	return stop;
>>>>> +	return enabled;
>>>>>  }
>>>>>  
>>>>>  static void enable_counters(void)
>>>>> @@ -618,7 +617,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
>>>>>  				stop = handle_interval(interval, times);
>>>>>  			time_to_sleep = sleep_time;
>>>>>  		} else { /* fd revent */
>>>>> -			stop = process_evlist(evsel_list, interval, times);
>>>>> +			if (process_evlist(evsel_list))
>>>>> +				stop = handle_interval(interval, times);
>>>>
>>>> It will call only on enable command and lead to artificial spikes in the beginning of interval.
>>>> May be just take handle_interval() out of process_evlist() and have it similar to record case?
>>>
>>> it can be called also for disable case then
>>
>>
>> Made it like this so now times counter is not affected during commands processing:
> 
> hm, can't process list just return true
> when the interval needs to be printed?

process_evlist() now looks suboptimal since record mode code directly calls evlist__ctlfd_process()
and then handles returned command specifically to the mode. So in v10 I replaced process_evlist()
call with direct evlist__ctlfd_process() call and then handling the returned command by printing
command msg tag and counter values in the required order. Like this:

+		clock_gettime(CLOCK_MONOTONIC, &time_start);
+		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
+			if (timeout)
+				break;
+			else
+				stop = handle_interval(interval, times);
+			time_to_sleep = sleep_time;
+		} else { /* fd revent */
+			if (evlist__ctlfd_process(evsel_list, &cmd) > 0) {
+				if (interval) {
+					switch (cmd) {
+					case EVLIST_CTL_CMD_ENABLE:
+						pr_info(EVLIST_ENABLED_MSG);
+						process_interval();
+						break;
+					case EVLIST_CTL_CMD_DISABLE:
+						process_interval();
+						pr_info(EVLIST_DISABLED_MSG);
+						break;
+					case EVLIST_CTL_CMD_ACK:
+					case EVLIST_CTL_CMD_UNSUPPORTED:
+					default:
+						break;
+					}
+				}
+			}
+			clock_gettime(CLOCK_MONOTONIC, &time_stop);
+			compute_tts(&time_start, &time_stop, &time_to_sleep);
+		}

Alexey

> 
> jirka
> 
>>
>> static void process_evlist(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>> {
>> 	if (evlist__ctlfd_process(evlist, cmd) > 0) {
>> 		switch (*cmd) {
>> 		case EVLIST_CTL_CMD_ENABLE:
>> 			pr_info(EVLIST_ENABLED_MSG);
>> 			break;
>> 		case EVLIST_CTL_CMD_DISABLE:
>> 			pr_info(EVLIST_DISABLED_MSG);
>> 			break;
>> 		case EVLIST_CTL_CMD_ACK:
>> 		case EVLIST_CTL_CMD_UNSUPPORTED:
>> 		default:
>> 			break;
>> 		}
>> 	}
>> }
>>
>> ...
>>
>> 		clock_gettime(CLOCK_MONOTONIC, &time_start);
>> 		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
>> 			if (timeout)
>> 				break;
>> 			else
>> 				stop = handle_interval(interval, times);
>> 			time_to_sleep = sleep_time;
>> 		} else { /* fd revent */
>> 			process_evlist(evsel_list, &cmd);
>> 			if (interval) {
>> 				switch (cmd) {
>> 				case EVLIST_CTL_CMD_ENABLE:
>> 				case EVLIST_CTL_CMD_DISABLE:
>> 					process_interval();
>> 				case EVLIST_CTL_CMD_ACK:
>> 				case EVLIST_CTL_CMD_UNSUPPORTED:
>> 				default:
>> 					break;
>> 				}
>> 			}
>> 			clock_gettime(CLOCK_MONOTONIC, &time_stop);
>> 			compute_tts(&time_start, &time_stop, &time_to_sleep);
>> 		}
>>
>> Alexey
>>
> 

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Jiri Olsa]

On Tue, Jul 07, 2020 at 05:55:14PM +0300, Alexey Budankov wrote:

SNIP

> process_evlist() now looks suboptimal since record mode code directly calls evlist__ctlfd_process()
> and then handles returned command specifically to the mode. So in v10 I replaced process_evlist()
> call with direct evlist__ctlfd_process() call and then handling the returned command by printing
> command msg tag and counter values in the required order. Like this:
> 
> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
> +			if (timeout)
> +				break;
> +			else
> +				stop = handle_interval(interval, times);
> +			time_to_sleep = sleep_time;
> +		} else { /* fd revent */
> +			if (evlist__ctlfd_process(evsel_list, &cmd) > 0) {
> +				if (interval) {
> +					switch (cmd) {
> +					case EVLIST_CTL_CMD_ENABLE:
> +						pr_info(EVLIST_ENABLED_MSG);
> +						process_interval();
> +						break;
> +					case EVLIST_CTL_CMD_DISABLE:
> +						process_interval();
> +						pr_info(EVLIST_DISABLED_MSG);
> +						break;
> +					case EVLIST_CTL_CMD_ACK:
> +					case EVLIST_CTL_CMD_UNSUPPORTED:
> +					default:
> +						break;
> +					}
> +				}
> +			}
> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
> +			compute_tts(&time_start, &time_stop, &time_to_sleep);
> +		}


hum, why not just get the bool from process_evlist like below?

jirka


---
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 5021f7286422..32dd3de93f35 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -485,20 +485,20 @@ static bool handle_interval(unsigned int interval, int *times)
 	return false;
 }
 
-static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
+static bool process_evlist(struct evlist *evlist)
 {
-	bool stop = false;
 	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
+	bool display = false;
 
 	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
 		switch (cmd) {
 		case EVLIST_CTL_CMD_ENABLE:
 			pr_info(EVLIST_ENABLED_MSG);
-			stop = handle_interval(interval, times);
+			display = true;
 			break;
 		case EVLIST_CTL_CMD_DISABLE:
-			stop = handle_interval(interval, times);
 			pr_info(EVLIST_DISABLED_MSG);
+			display = true;
 			break;
 		case EVLIST_CTL_CMD_ACK:
 		case EVLIST_CTL_CMD_UNSUPPORTED:
@@ -507,7 +507,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
 		}
 	}
 
-	return stop;
+	return display;
 }
 
 static void enable_counters(void)
@@ -618,7 +618,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
 				stop = handle_interval(interval, times);
 			time_to_sleep = sleep_time;
 		} else { /* fd revent */
-			stop = process_evlist(evsel_list, interval, times);
+			if (process_evlist(evsel_list))
+				stop = handle_interval(interval, times);
 			clock_gettime(CLOCK_MONOTONIC, &time_stop);
 			diff_timespec(&time_diff, &time_stop, &time_start);
 			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Alexey Budankov]

On 07.07.2020 19:05, Jiri Olsa wrote:
> On Tue, Jul 07, 2020 at 05:55:14PM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>> process_evlist() now looks suboptimal since record mode code directly calls evlist__ctlfd_process()
>> and then handles returned command specifically to the mode. So in v10 I replaced process_evlist()
>> call with direct evlist__ctlfd_process() call and then handling the returned command by printing
>> command msg tag and counter values in the required order. Like this:
>>
>> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
>> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
>> +			if (timeout)
>> +				break;
>> +			else
>> +				stop = handle_interval(interval, times);
>> +			time_to_sleep = sleep_time;
>> +		} else { /* fd revent */
>> +			if (evlist__ctlfd_process(evsel_list, &cmd) > 0) {
>> +				if (interval) {
>> +					switch (cmd) {
>> +					case EVLIST_CTL_CMD_ENABLE:
>> +						pr_info(EVLIST_ENABLED_MSG);
>> +						process_interval();
>> +						break;
>> +					case EVLIST_CTL_CMD_DISABLE:
>> +						process_interval();
>> +						pr_info(EVLIST_DISABLED_MSG);
>> +						break;
>> +					case EVLIST_CTL_CMD_ACK:
>> +					case EVLIST_CTL_CMD_UNSUPPORTED:
>> +					default:
>> +						break;
>> +					}
>> +				}
>> +			}
>> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
>> +			compute_tts(&time_start, &time_stop, &time_to_sleep);
>> +		}
> 
> 
> hum, why not just get the bool from process_evlist like below?

Yes, also possible and works. However it checks twice to implement
parts of logically the same work and passes the result using extra
memory: switch/case at process_evlist(), 'if' at dispatch_events(),
dispatch_events() should also call process_interval() instead of 
handle_interval() to avoid wasting of times counter for commands.

Alexey

> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 5021f7286422..32dd3de93f35 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -485,20 +485,20 @@ static bool handle_interval(unsigned int interval, int *times)
>  	return false;
>  }
>  
> -static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> +static bool process_evlist(struct evlist *evlist)
>  {
> -	bool stop = false;
>  	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> +	bool display = false;
>  
>  	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>  		switch (cmd) {
>  		case EVLIST_CTL_CMD_ENABLE:
>  			pr_info(EVLIST_ENABLED_MSG);
> -			stop = handle_interval(interval, times);
> +			display = true;
>  			break;
>  		case EVLIST_CTL_CMD_DISABLE:
> -			stop = handle_interval(interval, times);
>  			pr_info(EVLIST_DISABLED_MSG);
> +			display = true;
>  			break;
>  		case EVLIST_CTL_CMD_ACK:
>  		case EVLIST_CTL_CMD_UNSUPPORTED:
> @@ -507,7 +507,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
>  		}
>  	}
>  
> -	return stop;
> +	return display;
>  }
>  
>  static void enable_counters(void)
> @@ -618,7 +618,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
>  				stop = handle_interval(interval, times);
>  			time_to_sleep = sleep_time;
>  		} else { /* fd revent */
> -			stop = process_evlist(evsel_list, interval, times);
> +			if (process_evlist(evsel_list))
> +				stop = handle_interval(interval, times);
>  			clock_gettime(CLOCK_MONOTONIC, &time_stop);
>  			diff_timespec(&time_diff, &time_stop, &time_start);
>  			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
> 

Re: [PATCH v9 11/15] perf stat: implement control commands handling

[Alexey Budankov]

On 07.07.2020 17:55, Alexey Budankov wrote:
> 
> On 07.07.2020 17:23, Jiri Olsa wrote:
>> On Tue, Jul 07, 2020 at 04:24:28PM +0300, Alexey Budankov wrote:
>>>
>>> On 07.07.2020 16:14, Jiri Olsa wrote:
>>>> On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
>>>>>
>>>>> On 06.07.2020 22:34, Jiri Olsa wrote:
>>>>>> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>>>>>>>
>>>>>>> On 06.07.2020 15:34, Jiri Olsa wrote:
>>>>>>>> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
>>>>>>>>>
>>>>>>>>> Implement handling of 'enable' and 'disable' control commands
>>>>>>>>> coming from control file descriptor. process_evlist() function
>>>>>>>>> checks for events on control fds and makes required operations.
>>>>>>>>> If poll event splits initiated timeout interval then the reminder
>>>>>>>>> is calculated and still waited in the following poll() syscall.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>>>>> ---
>>>>>>>>>  tools/perf/builtin-stat.c | 75 ++++++++++++++++++++++++++++-----------
>>>>>>>>>  1 file changed, 55 insertions(+), 20 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>>>>>>> index 9e4288ecf2b8..5021f7286422 100644
>>>>>>>>> --- a/tools/perf/builtin-stat.c
>>>>>>>>> +++ b/tools/perf/builtin-stat.c
>>>>>>>>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int *times)
>>>>>>>>>  	return false;
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>>>>>>>>> +{
>>>>>>>>> +	bool stop = false;
>>>>>>>>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>>>>>>>>> +
>>>>>>>>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>>>>>>>>> +		switch (cmd) {
>>>>>>>>> +		case EVLIST_CTL_CMD_ENABLE:
>>>>>>>>> +			pr_info(EVLIST_ENABLED_MSG);
>>>>>>>>> +			stop = handle_interval(interval, times);
>>>>>>>>> +			break;
>>>>>>>>> +		case EVLIST_CTL_CMD_DISABLE:
>>>>>>>>> +			stop = handle_interval(interval, times);
>>>>>>>>
>>>>>>>> I still don't understand why you call handle_interval in here
>>>>>>>>
>>>>>>>> I don't see it being necessary.. you enable events and handle_interval,
>>>>>>>> wil be called in the next iteration of dispatch_events, why complicate
>>>>>>>> this function with that?
>>>>>>>
>>>>>>> Printing event counts at the moment of command processing lets scripts
>>>>>>> built on top of stat output to provide more plain and accurate metrics.
>>>>>>> Otherwise it may get spikes in the beginning of the next time interval
>>>>>>> because not all counts lay inside [Events enabled, Events disable]
>>>>>>> If -I interval is large tail event count can be also large. Compare the
>>>>>>> output below with the output in the cover letter. Either way is possible
>>>>>>> but the latter one likely complicates the scripts I mentioned above.
>>>>>>>
>>>>>>> perf=tools/perf/perf
>>>>>>> ${perf} stat -D -1 -e cpu-cycles -a -I 1000       \
>>>>>>>              --control fd:${ctl_fd},${ctl_fd_ack} \
>>>>>>>              -- sleep 40 &
>>>>>>>
>>>>>>> Events disabled
>>>>>>> #           time             counts unit events
>>>>>>>      1.001100723      <not counted>      cpu-cycles                                                  
>>>>>>>      2.003146566      <not counted>      cpu-cycles                                                  
>>>>>>>      3.005073317      <not counted>      cpu-cycles                                                  
>>>>>>>      4.006337062      <not counted>      cpu-cycles                                                  
>>>>>>> Events enabled
>>>>>>> enable acked(ack)
>>>>>>>      5.011182000         54,128,692      cpu-cycles <===                                                 
>>>>>>>      6.012300167      3,648,804,827      cpu-cycles                                                  
>>>>>>>      7.013631689        590,438,536      cpu-cycles                                                  
>>>>>>>      8.015558583        406,935,663      cpu-cycles                                                  
>>>>>>>      9.017455505        407,806,862      cpu-cycles                                                  
>>>>>>>     10.019300780        399,351,824      cpu-cycles                                                  
>>>>>>>     11.021180025        404,584,417      cpu-cycles                                                  
>>>>>>>     12.023033661        537,787,981      cpu-cycles                                                  
>>>>>>>     13.024422354        699,395,364      cpu-cycles                                                  
>>>>>>>     14.026325749        397,871,324      cpu-cycles                                                  
>>>>>>> disable acked()
>>>>>>> Events disabled
>>>>>>>     15.027857981        396,956,159      cpu-cycles <===
>>>>>>>     16.029279264      <not counted>      cpu-cycles                                                  
>>>>>>>     17.031131311      <not counted>      cpu-cycles                                                  
>>>>>>>     18.033010580      <not counted>      cpu-cycles                                                  
>>>>>>>     19.034918883      <not counted>      cpu-cycles                                                  
>>>>>>> enable acked(ack)
>>>>>>> Events enabled
>>>>>>>     20.036758793        183,544,975      cpu-cycles <===                                             
>>>>>>>     21.038163289        419,054,544      cpu-cycles                                                  
>>>>>>>     22.040108245        413,993,309      cpu-cycles                                                  
>>>>>>>     23.042042365        403,584,493      cpu-cycles                                                  
>>>>>>>     24.043985381        416,512,094      cpu-cycles                                                  
>>>>>>>     25.045925682        401,513,429      cpu-cycles                                                  
>>>>>>> #           time             counts unit events
>>>>>>>     26.047822238        461,205,096      cpu-cycles                                                  
>>>>>>>     27.049784263        414,319,162      cpu-cycles                                                  
>>>>>>>     28.051745360        403,706,915      cpu-cycles                                                  
>>>>>>>     29.053674600        416,502,883      cpu-cycles                                                  
>>>>>>> disable acked()
>>>>>>> Events disabled
>>>>>>>     30.054750685        414,184,409      cpu-cycles <===
>>>>>>
>>>>>> ok, but we could still take handle_interval out of process_evlist
>>>>>> and the interval process will be more clear for me (with some
>>>>>> additional comments in the code) ... perhaps something like below?
>>>>>>
>>>>>> thanks,
>>>>>> jirka
>>>>>>
>>>>>>
>>>>>> ---
>>>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>>>> index 5021f7286422..af83bf6b2db0 100644
>>>>>> --- a/tools/perf/builtin-stat.c
>>>>>> +++ b/tools/perf/builtin-stat.c
>>>>>> @@ -485,19 +485,18 @@ static bool handle_interval(unsigned int interval, int *times)
>>>>>>  	return false;
>>>>>>  }
>>>>>>  
>>>>>> -static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>>>>>> +static bool process_evlist(struct evlist *evlist)
>>>>>>  {
>>>>>> -	bool stop = false;
>>>>>>  	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>>>>>> +	bool enabled = false;
>>>>>>  
>>>>>>  	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>>>>>>  		switch (cmd) {
>>>>>>  		case EVLIST_CTL_CMD_ENABLE:
>>>>>>  			pr_info(EVLIST_ENABLED_MSG);
>>>>>> -			stop = handle_interval(interval, times);
>>>>>> +			enabled = true;
>>>>>>  			break;
>>>>>>  		case EVLIST_CTL_CMD_DISABLE:
>>>>>> -			stop = handle_interval(interval, times);
>>>>>>  			pr_info(EVLIST_DISABLED_MSG);
>>>>>>  			break;
>>>>>>  		case EVLIST_CTL_CMD_ACK:
>>>>>> @@ -507,7 +506,7 @@ static bool process_evlist(struct evlist *evlist, unsigned int interval, int *ti
>>>>>>  		}
>>>>>>  	}
>>>>>>  
>>>>>> -	return stop;
>>>>>> +	return enabled;
>>>>>>  }
>>>>>>  
>>>>>>  static void enable_counters(void)
>>>>>> @@ -618,7 +617,8 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times)
>>>>>>  				stop = handle_interval(interval, times);
>>>>>>  			time_to_sleep = sleep_time;
>>>>>>  		} else { /* fd revent */
>>>>>> -			stop = process_evlist(evsel_list, interval, times);
>>>>>> +			if (process_evlist(evsel_list))
>>>>>> +				stop = handle_interval(interval, times);
>>>>>
>>>>> It will call only on enable command and lead to artificial spikes in the beginning of interval.
>>>>> May be just take handle_interval() out of process_evlist() and have it similar to record case?
>>>>
>>>> it can be called also for disable case then
>>>
>>>
>>> Made it like this so now times counter is not affected during commands processing:
>>
>> hm, can't process list just return true
>> when the interval needs to be printed?
> 
> process_evlist() now looks suboptimal since record mode code directly calls evlist__ctlfd_process()
> and then handles returned command specifically to the mode. So in v10 I replaced process_evlist()
> call with direct evlist__ctlfd_process() call and then handling the returned command by printing
> command msg tag and counter values in the required order. Like this:
> 
> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
> +			if (timeout)
> +				break;
> +			else
> +				stop = handle_interval(interval, times);
> +			time_to_sleep = sleep_time;
> +		} else { /* fd revent */
> +			if (evlist__ctlfd_process(evsel_list, &cmd) > 0) {
> +				if (interval) {
> +					switch (cmd) {
> +					case EVLIST_CTL_CMD_ENABLE:
> +						pr_info(EVLIST_ENABLED_MSG);
> +						process_interval();
> +						break;
> +					case EVLIST_CTL_CMD_DISABLE:
> +						process_interval();
> +						pr_info(EVLIST_DISABLED_MSG);
> +						break;
> +					case EVLIST_CTL_CMD_ACK:
> +					case EVLIST_CTL_CMD_UNSUPPORTED:
> +					default:
> +						break;
> +					}
> +				}
> +			}

Does this if above want to be process_evlist(struct evlist *evlist, unsigned int interval) function?

Alexey

> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
> +			compute_tts(&time_start, &time_stop, &time_to_sleep);
> +		}
> 
> Alexey
> 
>>
>> jirka
>>
>>>
>>> static void process_evlist(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>>> {
>>> 	if (evlist__ctlfd_process(evlist, cmd) > 0) {
>>> 		switch (*cmd) {
>>> 		case EVLIST_CTL_CMD_ENABLE:
>>> 			pr_info(EVLIST_ENABLED_MSG);
>>> 			break;
>>> 		case EVLIST_CTL_CMD_DISABLE:
>>> 			pr_info(EVLIST_DISABLED_MSG);
>>> 			break;
>>> 		case EVLIST_CTL_CMD_ACK:
>>> 		case EVLIST_CTL_CMD_UNSUPPORTED:
>>> 		default:
>>> 			break;
>>> 		}
>>> 	}
>>> }
>>>
>>> ...
>>>
>>> 		clock_gettime(CLOCK_MONOTONIC, &time_start);
>>> 		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
>>> 			if (timeout)
>>> 				break;
>>> 			else
>>> 				stop = handle_interval(interval, times);
>>> 			time_to_sleep = sleep_time;
>>> 		} else { /* fd revent */
>>> 			process_evlist(evsel_list, &cmd);
>>> 			if (interval) {
>>> 				switch (cmd) {
>>> 				case EVLIST_CTL_CMD_ENABLE:
>>> 				case EVLIST_CTL_CMD_DISABLE:
>>> 					process_interval();
>>> 				case EVLIST_CTL_CMD_ACK:
>>> 				case EVLIST_CTL_CMD_UNSUPPORTED:
>>> 				default:
>>> 					break;
>>> 				}
>>> 			}
>>> 			clock_gettime(CLOCK_MONOTONIC, &time_stop);
>>> 			compute_tts(&time_start, &time_stop, &time_to_sleep);
>>> 		}
>>>
>>> Alexey
>>>
>>