Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1276488 > unrolled thread

[PATCH] perf record: Add snapshot mode support for perf's regular events

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf record: Add snapshot mode support for perf's regular events Yunlong Song <yunlong.song@huawei.com> - 2015-11-24 15:00 +0100
    [PATCH] perf record: Add snapshot mode support for perf's regular events Yunlong Song <yunlong.song@huawei.com> - 2015-11-24 15:00 +0100
      Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-11-24 15:40 +0100
        Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events Yunlong Song <yunlong.song@huawei.com> - 2015-11-25 13:50 +0100
      Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events David Ahern <dsahern@gmail.com> - 2015-11-24 16:10 +0100
        Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-11-24 16:30 +0100
          Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events David Ahern <dsahern@gmail.com> - 2015-11-24 16:30 +0100
            Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> - 2015-11-24 16:50 +0100
              Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events David Ahern <dsahern@gmail.com> - 2015-11-24 17:20 +0100
          Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events "Wangnan (F)" <wangnan0@huawei.com> - 2015-11-25 05:00 +0100
            Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events David Ahern <dsahern@gmail.com> - 2015-11-25 06:10 +0100
            Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events Adrian Hunter <adrian.hunter@intel.com> - 2015-11-25 08:30 +0100
              Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events "Wangnan (F)" <wangnan0@huawei.com> - 2015-11-25 09:00 +0100
                Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events Adrian Hunter <adrian.hunter@intel.com> - 2015-11-25 09:40 +0100
                  Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events "Wangnan (F)" <wangnan0@huawei.com> - 2015-11-25 10:00 +0100
                    Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events Adrian Hunter <adrian.hunter@intel.com> - 2015-11-25 10:10 +0100
        Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events Yunlong Song <yunlong.song@huawei.com> - 2015-11-25 09:00 +0100
    Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Peter Zijlstra <peterz@infradead.org> - 2015-11-25 10:30 +0100
      Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events "Wangnan (F)" <wangnan0@huawei.com> - 2015-11-25 10:50 +0100
        Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Peter Zijlstra <peterz@infradead.org> - 2015-11-25 13:30 +0100
          Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events "Wangnan (F)" <wangnan0@huawei.com> - 2015-11-25 14:00 +0100
            Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Ingo Molnar <mingo@kernel.org> - 2015-11-26 10:30 +0100
              Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Ingo Molnar <mingo@kernel.org> - 2015-11-26 10:50 +0100
              Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Ingo Molnar <mingo@kernel.org> - 2015-11-26 11:00 +0100
            Re: [PATCH] perf record: Add snapshot mode support for perf's regular  events "Wangnan (F)" <wangnan0@huawei.com> - 2015-11-26 10:30 +0100
            Re: [PATCH] perf record: Add snapshot mode support for perf's  regular events Ingo Molnar <mingo@kernel.org> - 2015-11-26 10:30 +0100

Page 1 of 2  [1] 2  Next page →

#1276488 — [PATCH] perf record: Add snapshot mode support for perf's regular events

This idea is issued and motivated from:
https://lwn.net/Articles/650499/

After the first RFC is sent:
http://lkml.iu.edu/hypermail/linux/kernel/1509.2/04347.html

Both David Ahern and Borislav Petkov have replied to that RFC:
http://lkml.iu.edu/hypermail/linux/kernel/1509.2/04350.html
http://lkml.iu.edu/hypermail/linux/kernel/1509.2/03914.html

Thanks to David's and Borislav's advice.

However, David's perf-based scheduling daemon just makes some count when
the signal triggers perf sched, with no sample recording and has nothing
to do with perf.data. As for Borislav's persistent events, when perf
record runs, it just makes fd to attach to the persistent event to read,
and all the persistent event's tracing info will still dump to perf.data
during perf's running.

As a result, neither David's nor Borislav's patches makes the similar
snapshot mode support as what aux trace does.

In our patch, we create and maintain a user space ring buffer to store
perf's tracing info, instead of directly writing to perf.data file as
before. In snapshot mode, only a SIGUSR2 signal can trigger perf to dump
the tracing info currently stored in the user space ring buffer to
perf.data file.

Yunlong Song (1):
  perf record: Add snapshot mode support for perf's regular events

 tools/perf/builtin-record.c | 181 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 170 insertions(+), 11 deletions(-)

-- 
1.8.5.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1276500

For aux area tracing, there is already a snapshot mode support for
intel-pt and intel-bts events. Similarly, this patch adds a snapshot
mode for perf's regular events. A user space ring buffer is allocated to
handle the tracing data from the kernel space ring buffer, and the
tracing data will only dump to perf.data when perf receives a SIGUSR2
signal.

Similarly like '-S' in aux trace snapshot mode, '-M' enables perf's
regular event's snapshot mode by defining the size (bytes) of the user
space ring buffer.

Example 1:

 $ perf record -a -M 10000000
 /*
  * Let perf record runs for some time before finally ends, and do not
  * send any SIGUSR2 signal to perf during perf's running.
  */

 $ perf report

 Error:
 The perf.data file has no samples!
 # To display the perf.data header info, please use --header/--header-only options.

As shown above, without any SIGUSR2 signal, perf record will dump no samples
to perf.data in the snapshot mode.

Example 2:

 $ perf record -a -M 10000000
 /*
  * Let perf record runs for some time before finally ends, and send
  * several times of SIGUSR2 signal to perf during perf's running.
  */

 # kill -SIGUSR2 `pidof perf`
 ...
 # kill -SIGUSR2 `pidof perf`

 $ perf report
<SNIP>
 # Total Lost Samples: 0
 #
 # Samples: 942  of event 'cycles:pp'
 # Event count (approx.): 175168972
 #
 # Overhead  Command          Shared Object            Symbol
 # ........  ...............  .......................  .........................................
 #
      8.20%  kworker/2:0      [kernel.kallsyms]        [k] default_send_IPI_mask_allbutself_phys
      6.33%  swapper          [kernel.kallsyms]        [k] intel_idle
      2.64%  pidof            [kernel.kallsyms]        [k] arch_get_unmapped_area_topdown
      2.56%  pidof            [kernel.kallsyms]        [k] unmap_region
      2.26%  pidof            [kernel.kallsyms]        [k] memcpy
      2.26%  pidof            libc-2.19.so             [.] _IO_vfscanf
      2.03%  pidof            [kernel.kallsyms]        [k] lookup_fast
      1.72%  pidof            [kernel.kallsyms]        [k] filp_close
      1.62%  pidof            [kernel.kallsyms]        [k] apparmor_file_open
      1.56%  pidof            [kernel.kallsyms]        [k] process_measurement
      1.50%  pidof            [kernel.kallsyms]        [k] find_vma
<SNIP>

As shown above, perf record will dump samples to perf.data every time
it receives a SIGUSR2 signal in the snapshot mode.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 tools/perf/builtin-record.c | 181 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 170 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 199fc31..75606a6 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -37,6 +37,16 @@
 #include <sched.h>
 #include <sys/mman.h>
 
+static volatile int memory_enabled;
+static volatile int memory_signalled;
+/* The maximum size of one perf_event is 65536*/
+#define MEMORY_SIZE_MIN 65537
+
+struct memory {
+	void *start;
+	u64 head, tail;
+	u64 size;
+};
 
 struct record {
 	struct perf_tool	tool;
@@ -51,16 +61,134 @@ struct record {
 	bool			no_buildid;
 	bool			no_buildid_cache;
 	unsigned long long	samples;
+	struct memory memory;
 };
 
-static int record__write(struct record *rec, void *bf, size_t size)
+static int buf_to_file(struct record *rec, void *buf,
+				size_t size, u64 head, u64 tail)
 {
-	if (perf_data_file__write(rec->session->file, bf, size) < 0) {
-		pr_err("failed to write perf data, error: %m\n");
+	size_t written = 0;
+
+	if (head < tail) {
+		if (perf_data_file__write(rec->session->file,
+					buf + head, tail - head) < 0)
+			goto out;
+		written += tail - head;
+	} else if (head > tail) {
+		if (perf_data_file__write(rec->session->file,
+					buf + head, size - head) < 0)
+			goto out;
+		written += size - head;
+
+		if (perf_data_file__write(rec->session->file, buf, tail) < 0)
+			goto out;
+		written += tail;
+	}
+
+	rec->bytes_written += written;
+	return 0;
+out:
+	pr_err("failed to write perf data, error: %m\n");
+	return -1;
+}
+
+static int memory_to_file(struct record *rec)
+{
+	if (buf_to_file(rec, rec->memory.start, rec->memory.size,
+			rec->memory.head, rec->memory.tail) < 0)
 		return -1;
+	rec->memory.head = rec->memory.tail;
+
+	return 0;
+}
+
+static ssize_t perf_memory__write(struct memory *memory, void *buf, size_t size)
+{
+	void *buf_start = buf;
+	size_t left = size, written, delta, skip;
+	union perf_event *event;
+	struct perf_event_header hdr;
+	struct record *rec = container_of(memory, struct record, memory);
+
+	while (left) {
+		skip = 0;
+		written = min(left, memory->size - memory->tail);
+		if (memory->head > memory->tail)
+			delta = memory->head - memory->tail;
+		else
+			delta = memory->size - memory->tail + memory->head;
+		if (delta <= written) {
+			do {
+				if ((memory->head + skip) <= (memory->size -
+						sizeof(struct perf_event_header)))
+					event = (union perf_event *)(memory->start +
+							memory->head + skip);
+				else {
+					size_t hdr_left;
+
+					hdr_left = sizeof(struct perf_event_header) -
+								memory->size + memory->head + skip;
+					memcpy(&hdr, memory->start + memory->head + skip,
+						sizeof(struct perf_event_header) - hdr_left);
+
+					if (hdr_left <= memory->tail)
+						memcpy((void *)&hdr + sizeof(struct perf_event_header) -
+								hdr_left, memory->start, hdr_left);
+					else if (!memory->tail)
+						memcpy((void *)&hdr + sizeof(struct perf_event_header) -
+								hdr_left, buf, hdr_left);
+					else {
+						memcpy((void *)&hdr + sizeof(struct perf_event_header) -
+								hdr_left, memory->start, memory->tail);
+						hdr_left -= memory->tail;
+						memcpy((void *)&hdr + sizeof(struct perf_event_header) -
+								hdr_left, buf, hdr_left);
+					}
+
+					event = (union perf_event *)&hdr;
+					if (rec->session->header.needs_swap)
+						perf_event_header__bswap(&event->header);
+				}
+
+				if (event->header.type != PERF_RECORD_SAMPLE) {
+					if (buf_to_file(rec, memory->start, memory->size,
+						memory->head + skip, (memory->head + skip +
+						event->header.size) % memory->size) < 0)
+						return -1;
+				}
+
+				skip += event->header.size;
+			} while (skip <= written - delta);
+		}
+
+		memcpy(memory->start + memory->tail, buf, written);
+
+		memory->head = (memory->head + skip) % memory->size;
+		memory->tail = (memory->tail + written) % memory->size;
+
+		left -= written;
+		buf  += written;
+	}
+
+	BUG_ON((size_t)(buf - buf_start) != size);
+	return size;
+}
+
+static int record__write(struct record *rec, void *bf, size_t size)
+{
+	if (rec->memory.size && memory_enabled) {
+		if (perf_memory__write(&rec->memory, bf, size) < 0) {
+			pr_err("failed to write memory data, error: %m\n");
+			return -1;
+		}
+	} else {
+		if (perf_data_file__write(rec->session->file, bf, size) < 0) {
+			pr_err("failed to write perf data, error: %m\n");
+			return -1;
+		}
+		rec->bytes_written += size;
 	}
 
-	rec->bytes_written += size;
 	return 0;
 }
 
@@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int idx)
 	if (old == head)
 		return 0;
 
+	memory_enabled = 1;
+
 	rec->samples++;
 
 	size = head - old;
@@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int idx)
 	md->prev = old;
 	perf_evlist__mmap_consume(rec->evlist, idx);
 out:
+	memory_enabled = 0;
 	return rc;
 }
 
@@ -426,8 +557,11 @@ static int record__mmap_read_all(struct record *rec)
 	 * Mark the round finished in case we wrote
 	 * at least one event.
 	 */
-	if (bytes_written != rec->bytes_written)
+	if (bytes_written != rec->bytes_written) {
+		memory_enabled = 1;
 		rc = record__write(rec, &finished_round_event, sizeof(finished_round_event));
+		memory_enabled = 0;
+	}
 
 out:
 	return rc;
@@ -492,7 +626,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	signal(SIGCHLD, sig_handler);
 	signal(SIGINT, sig_handler);
 	signal(SIGTERM, sig_handler);
-	if (rec->opts.auxtrace_snapshot_mode)
+	if (rec->opts.auxtrace_snapshot_mode || rec->memory.size)
 		signal(SIGUSR2, snapshot_sig_handler);
 	else
 		signal(SIGUSR2, SIG_IGN);
@@ -687,6 +821,14 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 			}
 		}
 
+		if (memory_signalled) {
+			memory_signalled = 0;
+			if (memory_to_file(rec) < 0) {
+				err = -1;
+				goto out_child;
+			}
+		}
+
 		if (hits == rec->samples) {
 			if (done || draining)
 				break;
@@ -1009,6 +1151,12 @@ static struct record record = {
 		.mmap2		= perf_event__process_mmap2,
 		.ordered_events	= true,
 	},
+	.memory = {
+		.start = NULL,
+		.head = 0,
+		.tail = 0,
+		.size = 0,
+	},
 };
 
 const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
@@ -1119,6 +1267,7 @@ struct option __record_options[] = {
 	OPT_STRING(0, "clang-opt", &llvm_param.clang_opt, "clang options",
 		   "options passed to clang when compiling BPF scriptlets"),
 #endif
+	OPT_U64('M', "memory", &record.memory.size, "user space ring buffer memory size (bytes)"),
 	OPT_END()
 };
 
@@ -1220,19 +1369,29 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 		goto out_symbol_exit;
 	}
 
+	if (rec->memory.size) {
+		if (rec->memory.size < MEMORY_SIZE_MIN)
+			rec->memory.size = MEMORY_SIZE_MIN;
+		rec->memory.start = malloc(rec->memory.size);
+	}
+
 	err = __cmd_record(&record, argc, argv);
 out_symbol_exit:
 	perf_evlist__delete(rec->evlist);
 	symbol__exit();
 	auxtrace_record__free(rec->itr);
+	if (rec->memory.size)
+		free(rec->memory.start);
 	return err;
 }
 
 static void snapshot_sig_handler(int sig __maybe_unused)
 {
-	if (!auxtrace_snapshot_enabled)
-		return;
-	auxtrace_snapshot_enabled = 0;
-	auxtrace_snapshot_err = auxtrace_record__snapshot_start(record.itr);
-	auxtrace_record__snapshot_started = 1;
+	if (record.opts.auxtrace_snapshot_mode && auxtrace_snapshot_enabled) {
+		auxtrace_snapshot_enabled = 0;
+		auxtrace_snapshot_err = auxtrace_record__snapshot_start(record.itr);
+		auxtrace_record__snapshot_started = 1;
+	}
+	if (record.memory.size && !memory_signalled)
+		memory_signalled = 1;
 }
-- 
1.8.5.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276527 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

Em Tue, Nov 24, 2015 at 10:00:32PM +0800, Yunlong Song escreveu:
> For aux area tracing, there is already a snapshot mode support for
> intel-pt and intel-bts events. Similarly, this patch adds a snapshot
> mode for perf's regular events. A user space ring buffer is allocated to
> handle the tracing data from the kernel space ring buffer, and the
> tracing data will only dump to perf.data when perf receives a SIGUSR2
> signal.
> 
> Similarly like '-S' in aux trace snapshot mode, '-M' enables perf's
> regular event's snapshot mode by defining the size (bytes) of the user
> space ring buffer.

Looks like an interesting feature, if for no other reason, to match what
we have for aux area tracing.

But perhaps having an event in the stream itself that would signal when
to dump the snapshot would be a better approach?

One that perhaps you create with 'perf probe' or a tracepoint, both
could use the in-kernel filtering capabilities to specify when to
trigger that snapshot?

If this snapshotting could happen in the kernel, that would be even
better, i.e. no need for two buffers (kernel rb + userspace rb) for
achieving this? Anyway, see below some comments.
 
> Example 1:
> 
>  $ perf record -a -M 10000000
>  /*
>   * Let perf record runs for some time before finally ends, and do not
>   * send any SIGUSR2 signal to perf during perf's running.
>   */
> 
>  $ perf report
> 
>  Error:
>  The perf.data file has no samples!
>  # To display the perf.data header info, please use --header/--header-only options.
> 
> As shown above, without any SIGUSR2 signal, perf record will dump no samples
> to perf.data in the snapshot mode.
> 
> Example 2:
> 
>  $ perf record -a -M 10000000
>  /*
>   * Let perf record runs for some time before finally ends, and send
>   * several times of SIGUSR2 signal to perf during perf's running.
>   */
> 
>  # kill -SIGUSR2 `pidof perf`
>  ...
>  # kill -SIGUSR2 `pidof perf`
> 
>  $ perf report
> <SNIP>
>  # Total Lost Samples: 0
>  #
>  # Samples: 942  of event 'cycles:pp'
>  # Event count (approx.): 175168972
>  #
>  # Overhead  Command          Shared Object            Symbol
>  # ........  ...............  .......................  .........................................
>  #
>       8.20%  kworker/2:0      [kernel.kallsyms]        [k] default_send_IPI_mask_allbutself_phys
>       6.33%  swapper          [kernel.kallsyms]        [k] intel_idle
>       2.64%  pidof            [kernel.kallsyms]        [k] arch_get_unmapped_area_topdown
>       2.56%  pidof            [kernel.kallsyms]        [k] unmap_region
>       2.26%  pidof            [kernel.kallsyms]        [k] memcpy
>       2.26%  pidof            libc-2.19.so             [.] _IO_vfscanf
>       2.03%  pidof            [kernel.kallsyms]        [k] lookup_fast
>       1.72%  pidof            [kernel.kallsyms]        [k] filp_close
>       1.62%  pidof            [kernel.kallsyms]        [k] apparmor_file_open
>       1.56%  pidof            [kernel.kallsyms]        [k] process_measurement
>       1.50%  pidof            [kernel.kallsyms]        [k] find_vma
> <SNIP>
> 
> As shown above, perf record will dump samples to perf.data every time
> it receives a SIGUSR2 signal in the snapshot mode.
> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  tools/perf/builtin-record.c | 181 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 170 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 199fc31..75606a6 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -37,6 +37,16 @@
>  #include <sched.h>
>  #include <sys/mman.h>
>  
> +static volatile int memory_enabled;
> +static volatile int memory_signalled;

Those are really too generic names :-\

> +/* The maximum size of one perf_event is 65536*/
> +#define MEMORY_SIZE_MIN 65537
> +
> +struct memory {

How about: snapshot_ring_buffer? If that is deemed too big, perhaps
snapshot_rb?

> +	void *start;
> +	u64 head, tail;
> +	u64 size;
> +};
>  
>  struct record {
>  	struct perf_tool	tool;
> @@ -51,16 +61,134 @@ struct record {
>  	bool			no_buildid;
>  	bool			no_buildid_cache;
>  	unsigned long long	samples;
> +	struct memory memory;
>  };
>  
> -static int record__write(struct record *rec, void *bf, size_t size)
> +static int buf_to_file(struct record *rec, void *buf,
> +				size_t size, u64 head, u64 tail)


Please continue following the existing convention and name this:

 static int record__write_rb()

As the buffer you're writing is a ring buffer, thus needs checking about
wrap around, like you do here.

>  {
> -	if (perf_data_file__write(rec->session->file, bf, size) < 0) {
> -		pr_err("failed to write perf data, error: %m\n");
> +	size_t written = 0;
> +
> +	if (head < tail) {
> +		if (perf_data_file__write(rec->session->file,
> +					buf + head, tail - head) < 0)
> +			goto out;
> +		written += tail - head;
> +	} else if (head > tail) {
> +		if (perf_data_file__write(rec->session->file,
> +					buf + head, size - head) < 0)
> +			goto out;
> +		written += size - head;
> +
> +		if (perf_data_file__write(rec->session->file, buf, tail) < 0)
> +			goto out;
> +		written += tail;
> +	}
> +
> +	rec->bytes_written += written;
> +	return 0;
> +out:
> +	pr_err("failed to write perf data, error: %m\n");
> +	return -1;
> +}
> +
> +static int memory_to_file(struct record *rec)
> +{
> +	if (buf_to_file(rec, rec->memory.start, rec->memory.size,
> +			rec->memory.head, rec->memory.tail) < 0)
>  		return -1;
> +	rec->memory.head = rec->memory.tail;
> +
> +	return 0;
> +}
> +
> +static ssize_t perf_memory__write(struct memory *memory, void *buf, size_t size)
> +{
> +	void *buf_start = buf;
> +	size_t left = size, written, delta, skip;
> +	union perf_event *event;
> +	struct perf_event_header hdr;
> +	struct record *rec = container_of(memory, struct record, memory);
> +
> +	while (left) {
> +		skip = 0;
> +		written = min(left, memory->size - memory->tail);
> +		if (memory->head > memory->tail)
> +			delta = memory->head - memory->tail;
> +		else
> +			delta = memory->size - memory->tail + memory->head;
> +		if (delta <= written) {
> +			do {
> +				if ((memory->head + skip) <= (memory->size -
> +						sizeof(struct perf_event_header)))
> +					event = (union perf_event *)(memory->start +
> +							memory->head + skip);
> +				else {
> +					size_t hdr_left;
> +
> +					hdr_left = sizeof(struct perf_event_header) -
> +								memory->size + memory->head + skip;
> +					memcpy(&hdr, memory->start + memory->head + skip,
> +						sizeof(struct perf_event_header) - hdr_left);
> +
> +					if (hdr_left <= memory->tail)
> +						memcpy((void *)&hdr + sizeof(struct perf_event_header) -
> +								hdr_left, memory->start, hdr_left);
> +					else if (!memory->tail)
> +						memcpy((void *)&hdr + sizeof(struct perf_event_header) -
> +								hdr_left, buf, hdr_left);
> +					else {
> +						memcpy((void *)&hdr + sizeof(struct perf_event_header) -
> +								hdr_left, memory->start, memory->tail);
> +						hdr_left -= memory->tail;
> +						memcpy((void *)&hdr + sizeof(struct perf_event_header) -
> +								hdr_left, buf, hdr_left);
> +					}
> +
> +					event = (union perf_event *)&hdr;
> +					if (rec->session->header.needs_swap)
> +						perf_event_header__bswap(&event->header);
> +				}
> +
> +				if (event->header.type != PERF_RECORD_SAMPLE) {
> +					if (buf_to_file(rec, memory->start, memory->size,
> +						memory->head + skip, (memory->head + skip +
> +						event->header.size) % memory->size) < 0)
> +						return -1;
> +				}
> +
> +				skip += event->header.size;
> +			} while (skip <= written - delta);
> +		}
> +
> +		memcpy(memory->start + memory->tail, buf, written);
> +
> +		memory->head = (memory->head + skip) % memory->size;
> +		memory->tail = (memory->tail + written) % memory->size;
> +
> +		left -= written;
> +		buf  += written;
> +	}
> +
> +	BUG_ON((size_t)(buf - buf_start) != size);
> +	return size;
> +}
> +
> +static int record__write(struct record *rec, void *bf, size_t size)
> +{
> +	if (rec->memory.size && memory_enabled) {
> +		if (perf_memory__write(&rec->memory, bf, size) < 0) {
> +			pr_err("failed to write memory data, error: %m\n");
> +			return -1;
> +		}
> +	} else {
> +		if (perf_data_file__write(rec->session->file, bf, size) < 0) {
> +			pr_err("failed to write perf data, error: %m\n");
> +			return -1;
> +		}
> +		rec->bytes_written += size;
>  	}
>  
> -	rec->bytes_written += size;
>  	return 0;
>  }
>  
> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int idx)
>  	if (old == head)
>  		return 0;
>  
> +	memory_enabled = 1;
> +

This really looks hackish, using a global to change the behaviour of
some existing function, and a volatile at that, ouch, please change the
prototypes in some way to avoid globals like this.

>  	rec->samples++;
>  
>  	size = head - old;
> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int idx)
>  	md->prev = old;
>  	perf_evlist__mmap_consume(rec->evlist, idx);
>  out:
> +	memory_enabled = 0;
>  	return rc;
>  }
>  
> @@ -426,8 +557,11 @@ static int record__mmap_read_all(struct record *rec)
>  	 * Mark the round finished in case we wrote
>  	 * at least one event.
>  	 */
> -	if (bytes_written != rec->bytes_written)
> +	if (bytes_written != rec->bytes_written) {
> +		memory_enabled = 1;
>  		rc = record__write(rec, &finished_round_event, sizeof(finished_round_event));
> +		memory_enabled = 0;
> +	}
>  
>  out:
>  	return rc;
> @@ -492,7 +626,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  	signal(SIGCHLD, sig_handler);
>  	signal(SIGINT, sig_handler);
>  	signal(SIGTERM, sig_handler);
> -	if (rec->opts.auxtrace_snapshot_mode)
> +	if (rec->opts.auxtrace_snapshot_mode || rec->memory.size)
>  		signal(SIGUSR2, snapshot_sig_handler);
>  	else
>  		signal(SIGUSR2, SIG_IGN);
> @@ -687,6 +821,14 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  			}
>  		}
>  
> +		if (memory_signalled) {
> +			memory_signalled = 0;
> +			if (memory_to_file(rec) < 0) {
> +				err = -1;
> +				goto out_child;
> +			}
> +		}
> +
>  		if (hits == rec->samples) {
>  			if (done || draining)
>  				break;
> @@ -1009,6 +1151,12 @@ static struct record record = {
>  		.mmap2		= perf_event__process_mmap2,
>  		.ordered_events	= true,
>  	},
> +	.memory = {
> +		.start = NULL,
> +		.head = 0,
> +		.tail = 0,
> +		.size = 0,
> +	},
>  };
>  
>  const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
> @@ -1119,6 +1267,7 @@ struct option __record_options[] = {
>  	OPT_STRING(0, "clang-opt", &llvm_param.clang_opt, "clang options",
>  		   "options passed to clang when compiling BPF scriptlets"),
>  #endif
> +	OPT_U64('M', "memory", &record.memory.size, "user space ring buffer memory size (bytes)"),
>  	OPT_END()
>  };
>  
> @@ -1220,19 +1369,29 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>  		goto out_symbol_exit;
>  	}
>  
> +	if (rec->memory.size) {
> +		if (rec->memory.size < MEMORY_SIZE_MIN)
> +			rec->memory.size = MEMORY_SIZE_MIN;
> +		rec->memory.start = malloc(rec->memory.size);
> +	}
> +
>  	err = __cmd_record(&record, argc, argv);
>  out_symbol_exit:
>  	perf_evlist__delete(rec->evlist);
>  	symbol__exit();
>  	auxtrace_record__free(rec->itr);
> +	if (rec->memory.size)
> +		free(rec->memory.start);
>  	return err;
>  }
>  
>  static void snapshot_sig_handler(int sig __maybe_unused)
>  {
> -	if (!auxtrace_snapshot_enabled)
> -		return;
> -	auxtrace_snapshot_enabled = 0;
> -	auxtrace_snapshot_err = auxtrace_record__snapshot_start(record.itr);
> -	auxtrace_record__snapshot_started = 1;
> +	if (record.opts.auxtrace_snapshot_mode && auxtrace_snapshot_enabled) {
> +		auxtrace_snapshot_enabled = 0;
> +		auxtrace_snapshot_err = auxtrace_record__snapshot_start(record.itr);
> +		auxtrace_record__snapshot_started = 1;
> +	}
> +	if (record.memory.size && !memory_signalled)
> +		memory_signalled = 1;
>  }
> -- 
> 1.8.5.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277357 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 2015/11/24 22:30, Arnaldo Carvalho de Melo wrote:

> Looks like an interesting feature, if for no other reason, to match what
> we have for aux area tracing.
> 
> But perhaps having an event in the stream itself that would signal when
> to dump the snapshot would be a better approach?
> 
> One that perhaps you create with 'perf probe' or a tracepoint, both
> could use the in-kernel filtering capabilities to specify when to
> trigger that snapshot?
> 
> If this snapshotting could happen in the kernel, that would be even
> better, i.e. no need for two buffers (kernel rb + userspace rb) for
> achieving this? Anyway, see below some comments.

Hi, Arnaldo,

Thanks for your suggestions and comments. As for the suggestion of snapshot
mode implemented in kernel ring buffer rather than user space ring buffer, I
have thought about that before I made this patch, the problems are:

1. Memory Handle

If snapshot mode implemented in kernel space ring buffer, what's proper size of
that ring buffer? If the size is small (such as 512k), it may lose the older
part of target info which is really related with the reason causing that snapshot.
For example, a temporary process launches and runs for a while, and it cannot
terminate properly in the end. The real reason is that something is wrong during
the launching stage, however, we do not know this prior knowledge and has to
record all the information from the beginning to the end of this process's liftime.
By analyzing the log of perf.data, we can finally figure out that the problem exists
in the launching stage. Here if kernel space ring buffer is not big enough, we will
lose the older part of the logs.

In this case, we may have to set the size of kernel space ring buffer big enough, but
can we kmalloc or vmalloc such big size easily in kernel space in any case? There
may be some limitations to do this. Since allocating memory in the kernel is not as
simple as allocating memory in user space. From this point of view, I prefer to use
user space ring buffer to store the records. It's more flexible and feasible to handle
memory.

2. Overwrite Overhead

Without overwrite feature, snapshot mode implemented in kernel space ring buffer
will also lose target info. For example, the mmap page of kernel space ring buffer
is not full at time A. The target info is filled to this mmap page and then it is
full at time B, then it wakes up perf to mmap_read all its records. In snapshot mode,
all the records are totally dropped without writing to perf.data. Later at time C,
a signal triggers the perf to dump, but the older part of target info was lost at
time B. This is because kernel space ring buffer does not overwrite itself, and dump
all its content once a page is full.

So, we need "overwrite" support, but it needs further fixes and the corresponding patch
is not applied to perf now probably due to its executive overhead (in the critical path,
not very sure yet).

Thus instead of incurring any probable overhead in the kernel space and thus may cause
any problems, I prefer to implement the "overwrite" feature in the user space ring
buffer.

3. Metadata Handle

Even if problem 2 above is solved, i.e., we finally fix the overwrite feature of kernel space
ring buffer. What about metadata handling (mmap_event, comm_event, fork_event, exit_event,
etc.)? When it's time to overwrite this info during the wraparound procedure, this metadata
events have to be handled separately to wake up perf (trigger the poll) to write to perf.data.
This may increase complexity in the kernel space. However, as for the user space ring buffer
version, it directly writes to perf.data once it notices the metadata event, no action of waking
up perf by poll at all. Since the user space ring buffer design has to parse each event to
figure out its size to skip when overwriting, it can easily figure out the event type by the
way (which event is perf_record_sample, and which event is metadata).

However, for this problem, there is a under-discussing solution. As what Wang Nan replies, "For
such tracking events (PERF_RECORD_FORK...), we have dummy event so it is possible for us to receive
tracking events from a separated channel, therefore we don't have to parse every events to pick
those events out."

4. Trigger Semantic

As what you mentioned, snapshot mode implemented in kernel space ring buffer "can" use the in-kernel
filtering capabilities to specify when to trigger that snapshot. In my personal opinion, I regard
the in-kernel filtering capabilities as something all about controlling, or say, programming with
the tracepoints/trace events, which does not relate to the semantic of snapshot. You can use the
in-kernel filtering capabilities (such as eBPF) to enable/disable tracing, filter tracing, aggregate
tracing, but all this tracing info should be written to perf.data normally. However, we can decide
when to really do this writing action via snapshot mode. Snapshot mode is just a semantic in the
user space from a high level view. Without snapshot mode, those "in-kernel filtering capabilities"
can still work for the regular perf. In the production case, apps can send SIGUSR2 signal to perf to
easily record the most useful info which is strongly related with the apps performance problem, no
redundant info any more (only collect the trace at the time any problem happens).


> How about: snapshot_ring_buffer? If that is deemed too big, perhaps
> snapshot_rb?
> 
> Please continue following the existing convention and name this:
> 
>  static int record__write_rb()
> 
> As the buffer you're writing is a ring buffer, thus needs checking about
> wrap around, like you do here.
> 
> This really looks hackish, using a global to change the behaviour of
> some existing function, and a volatile at that, ouch, please change the
> prototypes in some way to avoid globals like this.

Thanks for your careful review and helpful comments, my patch stands for certain kind of design for
snapshot mode. And other people may have suggestions and different designs, let's talk and compare
different designs first. If the design of this patch is taken finally, I will rewrite it then.

-- 
Thanks,
Yunlong Song

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276559 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 11/24/15 7:00 AM, Yunlong Song wrote:
> +static int record__write(struct record *rec, void *bf, size_t size)
> +{
> +	if (rec->memory.size && memory_enabled) {
> +		if (perf_memory__write(&rec->memory, bf, size) < 0) {
> +			pr_err("failed to write memory data, error: %m\n");
> +			return -1;
> +		}
> +	} else {
> +		if (perf_data_file__write(rec->session->file, bf, size) < 0) {
> +			pr_err("failed to write perf data, error: %m\n");
> +			return -1;
> +		}
> +		rec->bytes_written += size;
>   	}
>
> -	rec->bytes_written += size;
>   	return 0;
>   }
>
> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int idx)
>   	if (old == head)
>   		return 0;
>
> +	memory_enabled = 1;
> +
>   	rec->samples++;
>
>   	size = head - old;
> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int idx)
>   	md->prev = old;
>   	perf_evlist__mmap_consume(rec->evlist, idx);
>   out:
> +	memory_enabled = 0;
>   	return rc;
>   }
>

So you are basically ignoring all samples until SIGUSR2 is received. 
That means the resulting data file will have limited history of task 
events for example. And for other events the quantity is random as to 
when the mmaps were last scanned.

Your cover letter mentioned my code "just makes some count when the 
signal triggers perf sched, with no sample recording and has nothing to 
do with perf.data". That is not correct. If you look at the perf-daemon 
code I pointed you to it processes task events as they are received and 
saves the last N-events after time sorting (limited by memory or time). 
When a signal is received it processes the saved events and dumps them 
to stdout versus writing a perf.data file.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276570 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
> On 11/24/15 7:00 AM, Yunlong Song wrote:
> >+static int record__write(struct record *rec, void *bf, size_t size)
> >+{
> >+	if (rec->memory.size && memory_enabled) {
> >+		if (perf_memory__write(&rec->memory, bf, size) < 0) {
> >+			pr_err("failed to write memory data, error: %m\n");
> >+			return -1;
> >+		}
> >+	} else {
> >+		if (perf_data_file__write(rec->session->file, bf, size) < 0) {
> >+			pr_err("failed to write perf data, error: %m\n");
> >+			return -1;
> >+		}
> >+		rec->bytes_written += size;
> >  	}
> >
> >-	rec->bytes_written += size;
> >  	return 0;
> >  }
> >
> >@@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int idx)
> >  	if (old == head)
> >  		return 0;
> >
> >+	memory_enabled = 1;
> >+
> >  	rec->samples++;
> >
> >  	size = head - old;
> >@@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int idx)
> >  	md->prev = old;
> >  	perf_evlist__mmap_consume(rec->evlist, idx);
> >  out:
> >+	memory_enabled = 0;
> >  	return rc;
> >  }
> >
> 
> So you are basically ignoring all samples until SIGUSR2 is received. That

No, he is not, its just that his code is difficult to follow, has to be
rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
will..

> means the resulting data file will have limited history of task events for

... have a complete history of task events, since PERF_RECORD_FORK, etc
are not being ignored.

No?

- Arnaldo

> example. And for other events the quantity is random as to when the mmaps
> were last scanned.
> 
> Your cover letter mentioned my code "just makes some count when the signal
> triggers perf sched, with no sample recording and has nothing to do with
> perf.data". That is not correct. If you look at the perf-daemon code I
> pointed you to it processes task events as they are received and saves the
> last N-events after time sorting (limited by memory or time). When a signal
> is received it processes the saved events and dumps them to stdout versus
> writing a perf.data file.
> 
> David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276573 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 11/24/15 8:20 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
>> On 11/24/15 7:00 AM, Yunlong Song wrote:
>>> +static int record__write(struct record *rec, void *bf, size_t size)
>>> +{
>>> +	if (rec->memory.size && memory_enabled) {
>>> +		if (perf_memory__write(&rec->memory, bf, size) < 0) {
>>> +			pr_err("failed to write memory data, error: %m\n");
>>> +			return -1;
>>> +		}
>>> +	} else {
>>> +		if (perf_data_file__write(rec->session->file, bf, size) < 0) {
>>> +			pr_err("failed to write perf data, error: %m\n");
>>> +			return -1;
>>> +		}
>>> +		rec->bytes_written += size;
>>>   	}
>>>
>>> -	rec->bytes_written += size;
>>>   	return 0;
>>>   }
>>>
>>> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int idx)
>>>   	if (old == head)
>>>   		return 0;
>>>
>>> +	memory_enabled = 1;
>>> +
>>>   	rec->samples++;
>>>
>>>   	size = head - old;
>>> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int idx)
>>>   	md->prev = old;
>>>   	perf_evlist__mmap_consume(rec->evlist, idx);
>>>   out:
>>> +	memory_enabled = 0;
>>>   	return rc;
>>>   }
>>>
>>
>> So you are basically ignoring all samples until SIGUSR2 is received. That
>
> No, he is not, its just that his code is difficult to follow, has to be
> rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
> will..
>
>> means the resulting data file will have limited history of task events for
>
> ... have a complete history of task events, since PERF_RECORD_FORK, etc
> are not being ignored.
>
> No?

perf-record does not process events, it only writes to a file. If that 
is skipped then it skips all events regardless of type.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276582 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

Em Tue, Nov 24, 2015 at 08:24:07AM -0700, David Ahern escreveu:
> On 11/24/15 8:20 AM, Arnaldo Carvalho de Melo wrote:
> >Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
> >>On 11/24/15 7:00 AM, Yunlong Song wrote:
> >>>+static int record__write(struct record *rec, void *bf, size_t size)
> >>>+{
> >>>+	if (rec->memory.size && memory_enabled) {
> >>>+		if (perf_memory__write(&rec->memory, bf, size) < 0) {
> >>>+			pr_err("failed to write memory data, error: %m\n");
> >>>+			return -1;
> >>>+		}
> >>>+	} else {
> >>>+		if (perf_data_file__write(rec->session->file, bf, size) < 0) {
> >>>+			pr_err("failed to write perf data, error: %m\n");
> >>>+			return -1;
> >>>+		}
> >>>+		rec->bytes_written += size;
> >>>  	}
> >>>
> >>>-	rec->bytes_written += size;
> >>>  	return 0;
> >>>  }
> >>>
> >>>@@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int idx)
> >>>  	if (old == head)
> >>>  		return 0;
> >>>
> >>>+	memory_enabled = 1;
> >>>+
> >>>  	rec->samples++;
> >>>
> >>>  	size = head - old;
> >>>@@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int idx)
> >>>  	md->prev = old;
> >>>  	perf_evlist__mmap_consume(rec->evlist, idx);
> >>>  out:
> >>>+	memory_enabled = 0;
> >>>  	return rc;
> >>>  }
> >>>
> >>
> >>So you are basically ignoring all samples until SIGUSR2 is received. That
> >
> >No, he is not, its just that his code is difficult to follow, has to be
> >rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
> >will..

> >>means the resulting data file will have limited history of task events for

> >... have a complete history of task events, since PERF_RECORD_FORK, etc
> >are not being ignored.

> >No?
 
> perf-record does not process events, it only writes to a file. If that is
> skipped then it skips all events regardless of type.

perf-record without his patch? yes, but with his patch it does:

    __cmd_record()
        for (;;)
            record__mmap_read_all()
                record__write()
                    perf_memory__write()
			event = (union perf_event *)(memory->start + memory->head + skip);
                        if (event->header.type != PERF_RECORD_SAMPLE) {
                            if (buf_to_file(rec, memory->start, memory->size,
                        }

I almost thought that I had been fooled by the difficulty to follow his
patch and was forgetting that 'perf record' doesn't processes events,
and hasn't done so for a very good reason: to reduce its impact on the
observed workload, but that ain't so, no?

So, when not snapshotting, what you said remains true:

static int record__write(struct record *rec, void *bf, size_t size)
{
       if (rec->memory.size && memory_enabled) {
	        if (perf_memory__write(&rec->memory, bf, size) < 0) {
                       pr_err("failed to write memory data, error: %m\n");
	                return -1;
               }
       } else {
               if (perf_data_file__write(rec->session->file, bf, size) < 0) {

I'll continue taking the else branch and in that case, no events are
processed, we just dump that bf into the rec->session->file and go on
with life.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276604 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 11/24/15 8:40 AM, Arnaldo Carvalho de Melo wrote:
> perf-record without his patch? yes, but with his patch it does:
>
>      __cmd_record()
>          for (;;)
>              record__mmap_read_all()
>                  record__write()
>                      perf_memory__write()
> 			event = (union perf_event *)(memory->start + memory->head + skip);
>                          if (event->header.type != PERF_RECORD_SAMPLE) {
>                              if (buf_to_file(rec, memory->start, memory->size,
>                          }
>
> I almost thought that I had been fooled by the difficulty to follow his
> patch and was forgetting that 'perf record' doesn't processes events,
> and hasn't done so for a very good reason: to reduce its impact on the
> observed workload, but that ain't so, no?

exactly. And I missed the above. Thanks for pointing that out.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277002 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 2015/11/24 23:20, Arnaldo Carvalho de Melo wrote:
> Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
>> On 11/24/15 7:00 AM, Yunlong Song wrote:
>>> +static int record__write(struct record *rec, void *bf, size_t size)
>>> +{
>>> +	if (rec->memory.size && memory_enabled) {
>>> +		if (perf_memory__write(&rec->memory, bf, size) < 0) {
>>> +			pr_err("failed to write memory data, error: %m\n");
>>> +			return -1;
>>> +		}
>>> +	} else {
>>> +		if (perf_data_file__write(rec->session->file, bf, size) < 0) {
>>> +			pr_err("failed to write perf data, error: %m\n");
>>> +			return -1;
>>> +		}
>>> +		rec->bytes_written += size;
>>>   	}
>>>
>>> -	rec->bytes_written += size;
>>>   	return 0;
>>>   }
>>>
>>> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int idx)
>>>   	if (old == head)
>>>   		return 0;
>>>
>>> +	memory_enabled = 1;
>>> +
>>>   	rec->samples++;
>>>
>>>   	size = head - old;
>>> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int idx)
>>>   	md->prev = old;
>>>   	perf_evlist__mmap_consume(rec->evlist, idx);
>>>   out:
>>> +	memory_enabled = 0;
>>>   	return rc;
>>>   }
>>>
>> So you are basically ignoring all samples until SIGUSR2 is received. That
> No, he is not, its just that his code is difficult to follow, has to be
> rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
> will..
>
>> means the resulting data file will have limited history of task events for
> ... have a complete history of task events, since PERF_RECORD_FORK, etc
> are not being ignored.
>
> No?

Actually we are discussing about this problem.

For such tracking events (PERF_RECORD_FORK...), we have dummy event so
it is possible for us to receive tracking events from a separated
channel, therefore we don't have to parse every events to pick those
events out. Instead, we can process tracking events differently, then
more interesting things can be done. For example, squashing those tracking
events if it takes too much memory...

Furthermore, there's another problem being discussed: if userspace 
ringbuffer
is bytes based, parsing event is unavoidable. Without parsing event we are
unable to find the new 'head' pointer when overwriting. Instead, we are
thinking about a bucket-based ringbuffer that, let perf maintain a series
of bucket, each time 'poll' return, perf copies new events to the start of
a bucket. If all bucket is occupied, we drop the oldest bucket. Bucket-based
ringbuffer watest some memory but can avoid event parsing.

And there's many other problems in this patch. For example, when SIGUSR2 is
received, we need to do something to let all perf events start dumping.
Current implementation can't ensure we receive events just before the
SIGUSR2 if we not set 'no-buffer'.

Also, output events are in one perf.data, which is not user friendly.
Our final goal is to make perf a daemonized moniter, which can run 7x24
in user's environment. Each time a glitch is detected, a framework sends
a signal to perf to get a perf.data from it perf. The framework manage
those perf.data like logrotate, help developer analysis those glitch.

We are seeking the route implementing the final monitor. This patch is
an attempt to let you know what we want and get your thought about it.
Looks like you agree out basic idea. That's good. Then we decide to
start from some small feature to support the final goal. For example:
snapshot mode for specific events:

  # perf record -a -e cycles/snapshot/

And when C-c is pressed, for cycles event, only those data still in
kernel would be dump.

Thank you.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277016 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 11/24/15 8:50 PM, Wangnan (F) wrote:
> Actually we are discussing about this problem.
>
> For such tracking events (PERF_RECORD_FORK...), we have dummy event so
> it is possible for us to receive tracking events from a separated
> channel, therefore we don't have to parse every events to pick those
> events out. Instead, we can process tracking events differently, then
> more interesting things can be done. For example, squashing those tracking
> events if it takes too much memory...

If you look at my daemon code I process task events (FORK, MMAP, EXIT) 
to maintain task state including flushing threads when they terminate. 
This is a trade-off to having the knowledge to pretty-print addresses 
(address to symbol resolution) yet not grow without bounds -- be it a 
file or memory.

>
> Furthermore, there's another problem being discussed: if userspace
> ringbuffer
> is bytes based, parsing event is unavoidable. Without parsing event we are
> unable to find the new 'head' pointer when overwriting. Instead, we are
> thinking about a bucket-based ringbuffer that, let perf maintain a series
> of bucket, each time 'poll' return, perf copies new events to the start of
> a bucket. If all bucket is occupied, we drop the oldest bucket.
> Bucket-based
> ringbuffer watest some memory but can avoid event parsing.
>
> And there's many other problems in this patch. For example, when SIGUSR2 is
> received, we need to do something to let all perf events start dumping.
> Current implementation can't ensure we receive events just before the
> SIGUSR2 if we not set 'no-buffer'.
>
> Also, output events are in one perf.data, which is not user friendly.
> Our final goal is to make perf a daemonized moniter, which can run 7x24
> in user's environment. Each time a glitch is detected, a framework sends
> a signal to perf to get a perf.data from it perf. The framework manage
> those perf.data like logrotate, help developer analysis those glitch.

Exactly. And that's why my daemon is written the way it is. It is 
intended to run 24x7x365. It retains the last N events which are dumped 
when some external trigger tells it to.

Arnaldo: you asked about an event in the stream but that is not 
possible. My scheduling daemon targets CPU usage prior to a significant 
event (what was running, how long, where, etc). The significant event in 
the motivating case was STP timeouts -- if stp daemon is not able to 
send BPDUs why? What was running leading up to the timeout. The point is 
something external to the perf daemon says 'hey, save the last N-events 
for analysis'.

This case sounds like a generalization of my problem with the desire to 
write a perf.data file instead of processing the events and dumping to a 
file. It is doable. For example, synthesize task events for all threads 
in memory and then write out the saved samples.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277069 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 25/11/15 05:50, Wangnan (F) wrote:
> 
> 
> On 2015/11/24 23:20, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
>>> On 11/24/15 7:00 AM, Yunlong Song wrote:
>>>> +static int record__write(struct record *rec, void *bf, size_t size)
>>>> +{
>>>> +    if (rec->memory.size && memory_enabled) {
>>>> +        if (perf_memory__write(&rec->memory, bf, size) < 0) {
>>>> +            pr_err("failed to write memory data, error: %m\n");
>>>> +            return -1;
>>>> +        }
>>>> +    } else {
>>>> +        if (perf_data_file__write(rec->session->file, bf, size) < 0) {
>>>> +            pr_err("failed to write perf data, error: %m\n");
>>>> +            return -1;
>>>> +        }
>>>> +        rec->bytes_written += size;
>>>>       }
>>>>
>>>> -    rec->bytes_written += size;
>>>>       return 0;
>>>>   }
>>>>
>>>> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int
>>>> idx)
>>>>       if (old == head)
>>>>           return 0;
>>>>
>>>> +    memory_enabled = 1;
>>>> +
>>>>       rec->samples++;
>>>>
>>>>       size = head - old;
>>>> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int
>>>> idx)
>>>>       md->prev = old;
>>>>       perf_evlist__mmap_consume(rec->evlist, idx);
>>>>   out:
>>>> +    memory_enabled = 0;
>>>>       return rc;
>>>>   }
>>>>
>>> So you are basically ignoring all samples until SIGUSR2 is received. That
>> No, he is not, its just that his code is difficult to follow, has to be
>> rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
>> will..
>>
>>> means the resulting data file will have limited history of task events for
>> ... have a complete history of task events, since PERF_RECORD_FORK, etc
>> are not being ignored.
>>
>> No?
> 
> Actually we are discussing about this problem.
> 
> For such tracking events (PERF_RECORD_FORK...), we have dummy event so
> it is possible for us to receive tracking events from a separated
> channel, therefore we don't have to parse every events to pick those
> events out. Instead, we can process tracking events differently, then
> more interesting things can be done. For example, squashing those tracking
> events if it takes too much memory...
> 
> Furthermore, there's another problem being discussed: if userspace ringbuffer
> is bytes based, parsing event is unavoidable. Without parsing event we are
> unable to find the new 'head' pointer when overwriting.

Have you considered trying to find the head by trial-and-error at the time
you make the snapshot i.e. look at the first 8 bytes (event records are 8
byte aligned) and see if it is a valid record header, if not try the next 8
bytes.  When you find a real event record it should parse without error and
the subsequent events should all parse without error too, all the way to the
tail.  Then you can use timestamps and compare the events byte-by-byte to
avoid overlaps between 2 snapshots.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277073 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 2015/11/25 15:22, Adrian Hunter wrote:
> On 25/11/15 05:50, Wangnan (F) wrote:
>>
>> On 2015/11/24 23:20, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
>>>> On 11/24/15 7:00 AM, Yunlong Song wrote:
>>>>> +static int record__write(struct record *rec, void *bf, size_t size)
>>>>> +{
>>>>> +    if (rec->memory.size && memory_enabled) {
>>>>> +        if (perf_memory__write(&rec->memory, bf, size) < 0) {
>>>>> +            pr_err("failed to write memory data, error: %m\n");
>>>>> +            return -1;
>>>>> +        }
>>>>> +    } else {
>>>>> +        if (perf_data_file__write(rec->session->file, bf, size) < 0) {
>>>>> +            pr_err("failed to write perf data, error: %m\n");
>>>>> +            return -1;
>>>>> +        }
>>>>> +        rec->bytes_written += size;
>>>>>        }
>>>>>
>>>>> -    rec->bytes_written += size;
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int
>>>>> idx)
>>>>>        if (old == head)
>>>>>            return 0;
>>>>>
>>>>> +    memory_enabled = 1;
>>>>> +
>>>>>        rec->samples++;
>>>>>
>>>>>        size = head - old;
>>>>> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int
>>>>> idx)
>>>>>        md->prev = old;
>>>>>        perf_evlist__mmap_consume(rec->evlist, idx);
>>>>>    out:
>>>>> +    memory_enabled = 0;
>>>>>        return rc;
>>>>>    }
>>>>>
>>>> So you are basically ignoring all samples until SIGUSR2 is received. That
>>> No, he is not, its just that his code is difficult to follow, has to be
>>> rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
>>> will..
>>>
>>>> means the resulting data file will have limited history of task events for
>>> ... have a complete history of task events, since PERF_RECORD_FORK, etc
>>> are not being ignored.
>>>
>>> No?
>> Actually we are discussing about this problem.
>>
>> For such tracking events (PERF_RECORD_FORK...), we have dummy event so
>> it is possible for us to receive tracking events from a separated
>> channel, therefore we don't have to parse every events to pick those
>> events out. Instead, we can process tracking events differently, then
>> more interesting things can be done. For example, squashing those tracking
>> events if it takes too much memory...
>>
>> Furthermore, there's another problem being discussed: if userspace ringbuffer
>> is bytes based, parsing event is unavoidable. Without parsing event we are
>> unable to find the new 'head' pointer when overwriting.
> Have you considered trying to find the head by trial-and-error at the time
> you make the snapshot i.e. look at the first 8 bytes (event records are 8
> byte aligned) and see if it is a valid record header, if not try the next 8
> bytes.  When you find a real event record it should parse without error and
> the subsequent events should all parse without error too, all the way to the
> tail.  Then you can use timestamps and compare the events byte-by-byte to
> avoid overlaps between 2 snapshots.

It seems not work. Now we have BPF output event, it is possible that a
BPF program output anything through that event. Even if we have a magic
in head of each event, we can't prevent BPF output event output that
magic, except we introduce some 'escape' method to prevent BPF output
event output some data pattern. So although might work in reallife,
this solution is logically incorrect. Or am I miss someting?

Thank you.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277098 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 25/11/15 09:47, Wangnan (F) wrote:
> 
> 
> On 2015/11/25 15:22, Adrian Hunter wrote:
>> On 25/11/15 05:50, Wangnan (F) wrote:
>>>
>>> On 2015/11/24 23:20, Arnaldo Carvalho de Melo wrote:
>>>> Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
>>>>> On 11/24/15 7:00 AM, Yunlong Song wrote:
>>>>>> +static int record__write(struct record *rec, void *bf, size_t size)
>>>>>> +{
>>>>>> +    if (rec->memory.size && memory_enabled) {
>>>>>> +        if (perf_memory__write(&rec->memory, bf, size) < 0) {
>>>>>> +            pr_err("failed to write memory data, error: %m\n");
>>>>>> +            return -1;
>>>>>> +        }
>>>>>> +    } else {
>>>>>> +        if (perf_data_file__write(rec->session->file, bf, size) < 0) {
>>>>>> +            pr_err("failed to write perf data, error: %m\n");
>>>>>> +            return -1;
>>>>>> +        }
>>>>>> +        rec->bytes_written += size;
>>>>>>        }
>>>>>>
>>>>>> -    rec->bytes_written += size;
>>>>>>        return 0;
>>>>>>    }
>>>>>>
>>>>>> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int
>>>>>> idx)
>>>>>>        if (old == head)
>>>>>>            return 0;
>>>>>>
>>>>>> +    memory_enabled = 1;
>>>>>> +
>>>>>>        rec->samples++;
>>>>>>
>>>>>>        size = head - old;
>>>>>> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int
>>>>>> idx)
>>>>>>        md->prev = old;
>>>>>>        perf_evlist__mmap_consume(rec->evlist, idx);
>>>>>>    out:
>>>>>> +    memory_enabled = 0;
>>>>>>        return rc;
>>>>>>    }
>>>>>>
>>>>> So you are basically ignoring all samples until SIGUSR2 is received. That
>>>> No, he is not, its just that his code is difficult to follow, has to be
>>>> rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
>>>> will..
>>>>
>>>>> means the resulting data file will have limited history of task events for
>>>> ... have a complete history of task events, since PERF_RECORD_FORK, etc
>>>> are not being ignored.
>>>>
>>>> No?
>>> Actually we are discussing about this problem.
>>>
>>> For such tracking events (PERF_RECORD_FORK...), we have dummy event so
>>> it is possible for us to receive tracking events from a separated
>>> channel, therefore we don't have to parse every events to pick those
>>> events out. Instead, we can process tracking events differently, then
>>> more interesting things can be done. For example, squashing those tracking
>>> events if it takes too much memory...
>>>
>>> Furthermore, there's another problem being discussed: if userspace
>>> ringbuffer
>>> is bytes based, parsing event is unavoidable. Without parsing event we are
>>> unable to find the new 'head' pointer when overwriting.
>> Have you considered trying to find the head by trial-and-error at the time
>> you make the snapshot i.e. look at the first 8 bytes (event records are 8
>> byte aligned) and see if it is a valid record header, if not try the next 8
>> bytes.  When you find a real event record it should parse without error and
>> the subsequent events should all parse without error too, all the way to the
>> tail.  Then you can use timestamps and compare the events byte-by-byte to
>> avoid overlaps between 2 snapshots.
> 
> It seems not work. Now we have BPF output event, it is possible that a
> BPF program output anything through that event. Even if we have a magic
> in head of each event, we can't prevent BPF output event output that
> magic, except we introduce some 'escape' method to prevent BPF output
> event output some data pattern. So although might work in reallife,
> this solution is logically incorrect. Or am I miss someting?

When you find the head, all the events will parse correctly.  It seems to me
highly unlikely that would happen if you guessed the head wrongly.
It is only incorrect if it gives the wrong result.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277101 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 2015/11/25 16:27, Adrian Hunter wrote:
> On 25/11/15 09:47, Wangnan (F) wrote:
>>
>> On 2015/11/25 15:22, Adrian Hunter wrote:
>>> On 25/11/15 05:50, Wangnan (F) wrote:
>>>> On 2015/11/24 23:20, Arnaldo Carvalho de Melo wrote:
>>>>> Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
>>>>>> On 11/24/15 7:00 AM, Yunlong Song wrote:
>>>>>>> +static int record__write(struct record *rec, void *bf, size_t size)
>>>>>>> +{
>>>>>>> +    if (rec->memory.size && memory_enabled) {
>>>>>>> +        if (perf_memory__write(&rec->memory, bf, size) < 0) {
>>>>>>> +            pr_err("failed to write memory data, error: %m\n");
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +    } else {
>>>>>>> +        if (perf_data_file__write(rec->session->file, bf, size) < 0) {
>>>>>>> +            pr_err("failed to write perf data, error: %m\n");
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +        rec->bytes_written += size;
>>>>>>>         }
>>>>>>>
>>>>>>> -    rec->bytes_written += size;
>>>>>>>         return 0;
>>>>>>>     }
>>>>>>>
>>>>>>> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int
>>>>>>> idx)
>>>>>>>         if (old == head)
>>>>>>>             return 0;
>>>>>>>
>>>>>>> +    memory_enabled = 1;
>>>>>>> +
>>>>>>>         rec->samples++;
>>>>>>>
>>>>>>>         size = head - old;
>>>>>>> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec, int
>>>>>>> idx)
>>>>>>>         md->prev = old;
>>>>>>>         perf_evlist__mmap_consume(rec->evlist, idx);
>>>>>>>     out:
>>>>>>> +    memory_enabled = 0;
>>>>>>>         return rc;
>>>>>>>     }
>>>>>>>
>>>>>> So you are basically ignoring all samples until SIGUSR2 is received. That
>>>>> No, he is not, its just that his code is difficult to follow, has to be
>>>>> rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
>>>>> will..
>>>>>
>>>>>> means the resulting data file will have limited history of task events for
>>>>> ... have a complete history of task events, since PERF_RECORD_FORK, etc
>>>>> are not being ignored.
>>>>>
>>>>> No?
>>>> Actually we are discussing about this problem.
>>>>
>>>> For such tracking events (PERF_RECORD_FORK...), we have dummy event so
>>>> it is possible for us to receive tracking events from a separated
>>>> channel, therefore we don't have to parse every events to pick those
>>>> events out. Instead, we can process tracking events differently, then
>>>> more interesting things can be done. For example, squashing those tracking
>>>> events if it takes too much memory...
>>>>
>>>> Furthermore, there's another problem being discussed: if userspace
>>>> ringbuffer
>>>> is bytes based, parsing event is unavoidable. Without parsing event we are
>>>> unable to find the new 'head' pointer when overwriting.
>>> Have you considered trying to find the head by trial-and-error at the time
>>> you make the snapshot i.e. look at the first 8 bytes (event records are 8
>>> byte aligned) and see if it is a valid record header, if not try the next 8
>>> bytes.  When you find a real event record it should parse without error and
>>> the subsequent events should all parse without error too, all the way to the
>>> tail.  Then you can use timestamps and compare the events byte-by-byte to
>>> avoid overlaps between 2 snapshots.
>> It seems not work. Now we have BPF output event, it is possible that a
>> BPF program output anything through that event. Even if we have a magic
>> in head of each event, we can't prevent BPF output event output that
>> magic, except we introduce some 'escape' method to prevent BPF output
>> event output some data pattern. So although might work in reallife,
>> this solution is logically incorrect. Or am I miss someting?
> When you find the head, all the events will parse correctly.  It seems to me
> highly unlikely that would happen if you guessed the head wrongly.
> It is only incorrect if it gives the wrong result.

Right, so I said it might work in reallife. However, I think we
should better to try to provide some logically correct solution.
Also, 'guessing' means some sort of intelligence, or how do we
deal with guessing error? Simply drop them?

And what's your opinion on the bucket besed ring buffer? With that
design we only need to maintain a ringbuffer of pointers. It should
be much simpler. The only drawback I can image is the waste of memory
because we have to alloc buckets pessimistically. Do you think
that method have other problem I haven't considered?

Thank you.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277116 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 25/11/15 10:43, Wangnan (F) wrote:
> 
> 
> On 2015/11/25 16:27, Adrian Hunter wrote:
>> On 25/11/15 09:47, Wangnan (F) wrote:
>>>
>>> On 2015/11/25 15:22, Adrian Hunter wrote:
>>>> On 25/11/15 05:50, Wangnan (F) wrote:
>>>>> On 2015/11/24 23:20, Arnaldo Carvalho de Melo wrote:
>>>>>> Em Tue, Nov 24, 2015 at 08:06:41AM -0700, David Ahern escreveu:
>>>>>>> On 11/24/15 7:00 AM, Yunlong Song wrote:
>>>>>>>> +static int record__write(struct record *rec, void *bf, size_t size)
>>>>>>>> +{
>>>>>>>> +    if (rec->memory.size && memory_enabled) {
>>>>>>>> +        if (perf_memory__write(&rec->memory, bf, size) < 0) {
>>>>>>>> +            pr_err("failed to write memory data, error: %m\n");
>>>>>>>> +            return -1;
>>>>>>>> +        }
>>>>>>>> +    } else {
>>>>>>>> +        if (perf_data_file__write(rec->session->file, bf, size) < 0) {
>>>>>>>> +            pr_err("failed to write perf data, error: %m\n");
>>>>>>>> +            return -1;
>>>>>>>> +        }
>>>>>>>> +        rec->bytes_written += size;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -    rec->bytes_written += size;
>>>>>>>>         return 0;
>>>>>>>>     }
>>>>>>>>
>>>>>>>> @@ -86,6 +214,8 @@ static int record__mmap_read(struct record *rec, int
>>>>>>>> idx)
>>>>>>>>         if (old == head)
>>>>>>>>             return 0;
>>>>>>>>
>>>>>>>> +    memory_enabled = 1;
>>>>>>>> +
>>>>>>>>         rec->samples++;
>>>>>>>>
>>>>>>>>         size = head - old;
>>>>>>>> @@ -113,6 +243,7 @@ static int record__mmap_read(struct record *rec,
>>>>>>>> int
>>>>>>>> idx)
>>>>>>>>         md->prev = old;
>>>>>>>>         perf_evlist__mmap_consume(rec->evlist, idx);
>>>>>>>>     out:
>>>>>>>> +    memory_enabled = 0;
>>>>>>>>         return rc;
>>>>>>>>     }
>>>>>>>>
>>>>>>> So you are basically ignoring all samples until SIGUSR2 is received.
>>>>>>> That
>>>>>> No, he is not, its just that his code is difficult to follow, has to be
>>>>>> rewritten, but he is ignoring just PERF_RECORD_SAMPLE events, so it
>>>>>> will..
>>>>>>
>>>>>>> means the resulting data file will have limited history of task
>>>>>>> events for
>>>>>> ... have a complete history of task events, since PERF_RECORD_FORK, etc
>>>>>> are not being ignored.
>>>>>>
>>>>>> No?
>>>>> Actually we are discussing about this problem.
>>>>>
>>>>> For such tracking events (PERF_RECORD_FORK...), we have dummy event so
>>>>> it is possible for us to receive tracking events from a separated
>>>>> channel, therefore we don't have to parse every events to pick those
>>>>> events out. Instead, we can process tracking events differently, then
>>>>> more interesting things can be done. For example, squashing those tracking
>>>>> events if it takes too much memory...
>>>>>
>>>>> Furthermore, there's another problem being discussed: if userspace
>>>>> ringbuffer
>>>>> is bytes based, parsing event is unavoidable. Without parsing event we are
>>>>> unable to find the new 'head' pointer when overwriting.
>>>> Have you considered trying to find the head by trial-and-error at the time
>>>> you make the snapshot i.e. look at the first 8 bytes (event records are 8
>>>> byte aligned) and see if it is a valid record header, if not try the next 8
>>>> bytes.  When you find a real event record it should parse without error and
>>>> the subsequent events should all parse without error too, all the way to
>>>> the
>>>> tail.  Then you can use timestamps and compare the events byte-by-byte to
>>>> avoid overlaps between 2 snapshots.
>>> It seems not work. Now we have BPF output event, it is possible that a
>>> BPF program output anything through that event. Even if we have a magic
>>> in head of each event, we can't prevent BPF output event output that
>>> magic, except we introduce some 'escape' method to prevent BPF output
>>> event output some data pattern. So although might work in reallife,
>>> this solution is logically incorrect. Or am I miss someting?
>> When you find the head, all the events will parse correctly.  It seems to me
>> highly unlikely that would happen if you guessed the head wrongly.
>> It is only incorrect if it gives the wrong result.
> 
> Right, so I said it might work in reallife. However, I think we
> should better to try to provide some logically correct solution.
> Also, 'guessing' means some sort of intelligence, or how do we
> deal with guessing error? Simply drop them?

It is not "intelligence" it is a linear search.  If it gives more than one
answer, it is a fatal error.  You can mitigate that by adding more
validation of the event records.

But it is only a suggestion.

> And what's your opinion on the bucket besed ring buffer? With that
> design we only need to maintain a ringbuffer of pointers. It should
> be much simpler. The only drawback I can image is the waste of memory
> because we have to alloc buckets pessimistically. Do you think
> that method have other problem I haven't considered?

The drawback is that you have to copy all the events all the time instead of
letting the kernel ring buffer wraparound without any userspace involvement
until you make a snapshot.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277072 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 2015/11/24 23:06, David Ahern wrote:
> 
> So you are basically ignoring all samples until SIGUSR2 is received. That means the resulting data file will have limited history of task events for example. And for other events the quantity is random as to when the mmaps were last scanned.
> 
> Your cover letter mentioned my code "just makes some count when the signal triggers perf sched, with no sample recording and has nothing to do with perf.data". That is not correct. If you look at the perf-daemon code I pointed you to it processes task events as they are received and saves the last N-events after time sorting (limited by memory or time). When a signal is received it processes the saved events and dumps them to stdout versus writing a perf.data file.
> 
> David
> 

Hi, David,

Yes, I know that your sched daemon can store and print info when the signal triggers,
however, what I mean 'makes some count' is: sched daemon parses and processes the events
to extract the tracing info related with sched, rather than a general use of perf.data
like "perf script", "perf report", "perf data convert --to-ctf", etc. And what I mean
'no sample recording and has nothing to do with perf.data' is: when perf receives a signal,
sched daemon uses timehist_print_summary and timehist_pstree to record those tracing info
related with sched to a new file rather than the raw perf event records in the perf.data.
We can not use those files generated by sched daemon to enjoy the strong functions like how
perf.data can be used in "perf script", "perf report", "perf data convert --to-ctf", etc.

Sched daemon is good, but it is carefully designed for specific use of perf sched. In general
case of perf record, with snapshot mode, we still want a perf.data as before. Your sched daemon
concurrently does the work of storing and sched-parsing action for each signal trigger. To get
a general style of perf.data, the sched-parsing semantic action may have to be removed.

-- 
Thanks,
Yunlong Song

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277129 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On Tue, Nov 24, 2015 at 10:00:31PM +0800, Yunlong Song wrote:
> In our patch, we create and maintain a user space ring buffer to store
> perf's tracing info, instead of directly writing to perf.data file as
> before. In snapshot mode, only a SIGUSR2 signal can trigger perf to dump
> the tracing info currently stored in the user space ring buffer to
> perf.data file.

I would very much like to first fix the perf overwrite mode: see
lkml.kernel.org/r/20151023151205.GW11639@twins.programming.kicks-ass.net
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277138 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On 2015/11/25 17:27, Peter Zijlstra wrote:
> On Tue, Nov 24, 2015 at 10:00:31PM +0800, Yunlong Song wrote:
>> In our patch, we create and maintain a user space ring buffer to store
>> perf's tracing info, instead of directly writing to perf.data file as
>> before. In snapshot mode, only a SIGUSR2 signal can trigger perf to dump
>> the tracing info currently stored in the user space ring buffer to
>> perf.data file.
> I would very much like to first fix the perf overwrite mode: see
> lkml.kernel.org/r/20151023151205.GW11639@twins.programming.kicks-ass.net

I think they can be done in parallel. We can first do something with
tracking events and perf's output file, and wait for kernel level
overwrite mode fixed, then decide whether to implement perf's own
ringbuffer.

Thank you.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277332 — Re: [PATCH] perf record: Add snapshot mode support for perf's regular events

On Wed, Nov 25, 2015 at 05:44:00PM +0800, Wangnan (F) wrote:
> 
> 
> On 2015/11/25 17:27, Peter Zijlstra wrote:
> >On Tue, Nov 24, 2015 at 10:00:31PM +0800, Yunlong Song wrote:
> >>In our patch, we create and maintain a user space ring buffer to store
> >>perf's tracing info, instead of directly writing to perf.data file as
> >>before. In snapshot mode, only a SIGUSR2 signal can trigger perf to dump
> >>the tracing info currently stored in the user space ring buffer to
> >>perf.data file.
> >I would very much like to first fix the perf overwrite mode: see
> >lkml.kernel.org/r/20151023151205.GW11639@twins.programming.kicks-ass.net
> 
> I think they can be done in parallel. We can first do something with
> tracking events and perf's output file, and wait for kernel level
> overwrite mode fixed, then decide whether to implement perf's own
> ringbuffer.

That seems backwards; why would you ever want to endlessly copy the
events if you're not going to use them?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web