diff mbox

[50/54] perf record: Toggle overwrite ring buffer for reading

Message ID 1453715801-7732-51-git-send-email-wangnan0@huawei.com
State Superseded
Headers show

Commit Message

Wang Nan Jan. 25, 2016, 9:56 a.m. UTC 
Reading from a overwrite ring buffer is unrelible. perf_evsel__pause()
should be called before reading from them.

Toggel overwrite_evt_paused director after receiving done or switch
output.

Signed-off-by: Wang Nan <wangnan0@huawei.com>

Signed-off-by: He Kuang <hekuang@huawei.com>

Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
---
 tools/perf/builtin-record.c | 77 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

-- 
1.8.3.4

Comments

Wang Nan Jan. 26, 2016, 8:25 a.m. UTC | #1 
On 2016/1/25 17:56, Wang Nan wrote:
> Reading from a overwrite ring buffer is unrelible. perf_evsel__pause()

> should be called before reading from them.

>

> Toggel overwrite_evt_paused director after receiving done or switch

> output.

>

> Signed-off-by: Wang Nan <wangnan0@huawei.com>

> Signed-off-by: He Kuang <hekuang@huawei.com>

> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>

> Cc: Jiri Olsa <jolsa@kernel.org>

> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

> Cc: Namhyung Kim <namhyung@kernel.org>

> Cc: Zefan Li <lizefan@huawei.com>

> Cc: pi3orama@163.com

> ---

>   tools/perf/builtin-record.c | 77 +++++++++++++++++++++++++++++++++++++++++++++

>   1 file changed, 77 insertions(+)


[SNIP]

> +static void

> +record__toggle_overwrite_evsels(struct record *rec,

> +				enum overwrite_evt_state state)

> +{


[SNIP]

> +	rec->overwrite_evt_state = state;

> +

> +	if (action == NONE)

> +		return;

> +

> +	evlist__for_each(evlist, pos) {

> +		if (!pos->overwrite)

> +			continue;

> +		perf_evsel__pause(pos, action == PAUSE);

> +	}

> +}

> +

This part is incorrect. We should pause ring buffers for each CPU
in a channel, not each evsel.

Already fixed at:

https://git.kernel.org/cgit/linux/kernel/git/pi3orama/linux.git/commit/?h=perf/overwrite&id=fe59d9c6621c60087ce7e6e269f2f15f152d6d71

Thank you.
diff mbox

Patch

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 09aa4ee..69f089f 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -39,6 +39,11 @@ 
 #include <sys/mman.h>
 #include <asm/bug.h>
 
+enum overwrite_evt_state {
+	OVERWRITE_EVT_RUNNING,
+	OVERWRITE_EVT_DATA_PENDING,
+	OVERWRITE_EVT_EMPTY,
+};
 
 struct record {
 	struct perf_tool	tool;
@@ -57,6 +62,7 @@  struct record {
 	bool			buildid_all;
 	bool			timestamp_filename;
 	bool			switch_output;
+	enum overwrite_evt_state overwrite_evt_state;
 	unsigned long long	samples;
 };
 
@@ -388,6 +394,7 @@  try_again:
 
 	session->evlist = evlist;
 	perf_session__set_id_hdr_size(session);
+	rec->overwrite_evt_state = OVERWRITE_EVT_RUNNING;
 out:
 	return rc;
 }
@@ -468,6 +475,50 @@  static struct perf_event_header finished_round_event = {
 	.type = PERF_RECORD_FINISHED_ROUND,
 };
 
+static void
+record__toggle_overwrite_evsels(struct record *rec,
+				enum overwrite_evt_state state)
+{
+	struct perf_evsel *pos;
+	struct perf_evlist *evlist = rec->evlist;
+	enum overwrite_evt_state old_state = rec->overwrite_evt_state;
+	enum action {
+		NONE,
+		PAUSE,
+		RESUME,
+	} action = NONE;
+
+	switch (old_state) {
+	case OVERWRITE_EVT_RUNNING:
+		if (state != OVERWRITE_EVT_RUNNING)
+			action = PAUSE;
+		break;
+	case OVERWRITE_EVT_DATA_PENDING:
+		if (state == OVERWRITE_EVT_RUNNING)
+			action = RESUME;
+		break;
+	case OVERWRITE_EVT_EMPTY:
+		if (state == OVERWRITE_EVT_RUNNING)
+			action = RESUME;
+		if (state == OVERWRITE_EVT_DATA_PENDING)
+			state = OVERWRITE_EVT_EMPTY;
+		break;
+	default:
+		WARN_ONCE(1, "Shouldn't get there\n");
+	}
+
+	rec->overwrite_evt_state = state;
+
+	if (action == NONE)
+		return;
+
+	evlist__for_each(evlist, pos) {
+		if (!pos->overwrite)
+			continue;
+		perf_evsel__pause(pos, action == PAUSE);
+	}
+}
+
 static bool record__mmap_should_read(struct record *rec, int idx)
 {
 	int channel = -1;
@@ -512,6 +563,8 @@  static int record__mmap_read_all(struct record *rec)
 	if (bytes_written != rec->bytes_written)
 		rc = record__write(rec, &finished_round_event, sizeof(finished_round_event));
 
+	if (rec->overwrite_evt_state == OVERWRITE_EVT_DATA_PENDING)
+		record__toggle_overwrite_evsels(rec, OVERWRITE_EVT_EMPTY);
 out:
 	return rc;
 }
@@ -870,6 +923,17 @@  static int __cmd_record(struct record *rec, int argc, const char **argv)
 	for (;;) {
 		unsigned long long hits = rec->samples;
 
+		/*
+		 * rec->overwrite_evt_state is possible to be
+		 * OVERWRITE_EVT_EMPTY here: when done == true and
+		 * hits != rec->samples after previous reading.
+		 *
+		 * record__toggle_overwrite_evsels ensure we never
+		 * convert OVERWRITE_EVT_EMPTY to OVERWRITE_EVT_DATA_PENDING.
+		 */
+		if (switch_output_started || done || draining)
+			record__toggle_overwrite_evsels(rec, OVERWRITE_EVT_DATA_PENDING);
+
 		if (record__mmap_read_all(rec) < 0) {
 			auxtrace_snapshot_disable();
 			err = -1;
@@ -888,7 +952,20 @@  static int __cmd_record(struct record *rec, int argc, const char **argv)
 		}
 
 		if (switch_output_started) {
+			/*
+			 * SIGUSR2 raise after or during record__mmap_read_all().
+			 * continue to read again.
+			 */
+			if (rec->overwrite_evt_state == OVERWRITE_EVT_RUNNING)
+				continue;
+
 			switch_output_started = 0;
+			/*
+			 * Reenable events in overwrite ring buffer after
+			 * record__mmap_read_all(): we should have collected
+			 * data from it.
+			 */
+			record__toggle_overwrite_evsels(rec, OVERWRITE_EVT_RUNNING);
 
 			if (!quiet)
 				fprintf(stderr, "[ perf record: dump data: Woken up %ld times ]\n",