[PATCHv2 0/7] perf tools: Add data file write interface

[PATCHv2 0/7] perf tools: Add data file write interface

[Jiri Olsa]

hi,
adding perf_data_file__write function to centralize
output file writes. Using it in record and inject
commands.

v2 changes:
  - changes for readn function, suggested by Ingo
  - added writen function, suggested by Arnaldo
  - spliting record change into 2 separated patches
  - omiting some unnecessary changes in the inject change
 
thanks,
jirka


Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
Jiri Olsa (7):
      perf record: Unify data output code into perf_record__write function
      perf tools: Use correct return type for readn function
      perf tools: Fine tune readn function
      perf tools: Add writen function
      perf tools: Add perf_data_file__write interface
      perf record: Use perf_data_file__write for output file
      perf inject: Handle output file via perf_data_file object

 tools/perf/builtin-inject.c | 65 +++++++++++++++++++++++++++--------------------------------------
 tools/perf/builtin-record.c | 41 +++++++++++++----------------------------
 tools/perf/util/data.c      |  6 ++++++
 tools/perf/util/data.h      | 14 ++++++++------
 tools/perf/util/header.c    | 18 +++++++++---------
 tools/perf/util/session.c   |  2 +-
 tools/perf/util/util.c      | 32 ++++++++++++++++++++++++++------
 tools/perf/util/util.h      |  3 ++-
 8 files changed, 92 insertions(+), 89 deletions(-)

[PATCH 1/7] perf record: Unify data output code into perf_record__write function

[Jiri Olsa]

Unifying current 2 data output functions do_write_output
and write_output into single one perf_record__write.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 65615a8..d93e2ee 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -76,7 +76,7 @@ struct perf_record {
 	long			samples;
 };
 
-static int do_write_output(struct perf_record *rec, void *buf, size_t size)
+static int perf_record__write(struct perf_record *rec, void *buf, size_t size)
 {
 	struct perf_data_file *file = &rec->file;
 
@@ -97,21 +97,13 @@ static int do_write_output(struct perf_record *rec, void *buf, size_t size)
 	return 0;
 }
 
-static int write_output(struct perf_record *rec, void *buf, size_t size)
-{
-	return do_write_output(rec, buf, size);
-}
-
 static int process_synthesized_event(struct perf_tool *tool,
 				     union perf_event *event,
 				     struct perf_sample *sample __maybe_unused,
 				     struct machine *machine __maybe_unused)
 {
 	struct perf_record *rec = container_of(tool, struct perf_record, tool);
-	if (write_output(rec, event, event->header.size) < 0)
-		return -1;
-
-	return 0;
+	return perf_record__write(rec, event, event->header.size);
 }
 
 static int perf_record__mmap_read(struct perf_record *rec,
@@ -136,7 +128,7 @@ static int perf_record__mmap_read(struct perf_record *rec,
 		size = md->mask + 1 - (old & md->mask);
 		old += size;
 
-		if (write_output(rec, buf, size) < 0) {
+		if (perf_record__write(rec, buf, size) < 0) {
 			rc = -1;
 			goto out;
 		}
@@ -146,7 +138,7 @@ static int perf_record__mmap_read(struct perf_record *rec,
 	size = head - old;
 	old += size;
 
-	if (write_output(rec, buf, size) < 0) {
+	if (perf_record__write(rec, buf, size) < 0) {
 		rc = -1;
 		goto out;
 	}
@@ -335,8 +327,8 @@ static int perf_record__mmap_read_all(struct perf_record *rec)
 	}
 
 	if (perf_header__has_feat(&rec->session->header, HEADER_TRACING_DATA))
-		rc = write_output(rec, &finished_round_event,
-				  sizeof(finished_round_event));
+		rc = perf_record__write(rec, &finished_round_event,
+					sizeof(finished_round_event));
 
 out:
 	return rc;
-- 
1.8.3.1

[tip:perf/core] perf record: Unify data output code into perf_record__write function

[tip-bot for Jiri Olsa]

Commit-ID:  6233dd5efdf9e2c2da1b003cfb70307b7b2028e8
Gitweb:     http://git.kernel.org/tip/6233dd5efdf9e2c2da1b003cfb70307b7b2028e8
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Thu, 28 Nov 2013 11:30:13 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 2 Dec 2013 09:22:45 -0300

perf record: Unify data output code into perf_record__write function

Unifying current 2 data output functions do_write_output and
write_output into single one perf_record__write.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1385634619-8129-2-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 65615a8..d93e2ee 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -76,7 +76,7 @@ struct perf_record {
 	long			samples;
 };
 
-static int do_write_output(struct perf_record *rec, void *buf, size_t size)
+static int perf_record__write(struct perf_record *rec, void *buf, size_t size)
 {
 	struct perf_data_file *file = &rec->file;
 
@@ -97,21 +97,13 @@ static int do_write_output(struct perf_record *rec, void *buf, size_t size)
 	return 0;
 }
 
-static int write_output(struct perf_record *rec, void *buf, size_t size)
-{
-	return do_write_output(rec, buf, size);
-}
-
 static int process_synthesized_event(struct perf_tool *tool,
 				     union perf_event *event,
 				     struct perf_sample *sample __maybe_unused,
 				     struct machine *machine __maybe_unused)
 {
 	struct perf_record *rec = container_of(tool, struct perf_record, tool);
-	if (write_output(rec, event, event->header.size) < 0)
-		return -1;
-
-	return 0;
+	return perf_record__write(rec, event, event->header.size);
 }
 
 static int perf_record__mmap_read(struct perf_record *rec,
@@ -136,7 +128,7 @@ static int perf_record__mmap_read(struct perf_record *rec,
 		size = md->mask + 1 - (old & md->mask);
 		old += size;
 
-		if (write_output(rec, buf, size) < 0) {
+		if (perf_record__write(rec, buf, size) < 0) {
 			rc = -1;
 			goto out;
 		}
@@ -146,7 +138,7 @@ static int perf_record__mmap_read(struct perf_record *rec,
 	size = head - old;
 	old += size;
 
-	if (write_output(rec, buf, size) < 0) {
+	if (perf_record__write(rec, buf, size) < 0) {
 		rc = -1;
 		goto out;
 	}
@@ -335,8 +327,8 @@ static int perf_record__mmap_read_all(struct perf_record *rec)
 	}
 
 	if (perf_header__has_feat(&rec->session->header, HEADER_TRACING_DATA))
-		rc = write_output(rec, &finished_round_event,
-				  sizeof(finished_round_event));
+		rc = perf_record__write(rec, &finished_round_event,
+					sizeof(finished_round_event));
 
 out:
 	return rc;

[PATCH 2/7] perf tools: Use correct return type for readn function

[Jiri Olsa]

Changing readn function return type to ssize_t because
read returns ssize_t not int.

Changing callers holding variable types as well.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c  | 18 +++++++++---------
 tools/perf/util/session.c |  2 +-
 tools/perf/util/util.c    |  4 ++--
 tools/perf/util/util.h    |  2 +-
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 1cd0357..3e755f2 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1709,7 +1709,7 @@ static int process_nrcpus(struct perf_file_section *section __maybe_unused,
 			  struct perf_header *ph, int fd,
 			  void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	u32 nr;
 
 	ret = readn(fd, &nr, sizeof(nr));
@@ -1753,7 +1753,7 @@ static int process_total_mem(struct perf_file_section *section __maybe_unused,
 			     void *data __maybe_unused)
 {
 	uint64_t mem;
-	size_t ret;
+	ssize_t ret;
 
 	ret = readn(fd, &mem, sizeof(mem));
 	if (ret != sizeof(mem))
@@ -1822,7 +1822,7 @@ static int process_cmdline(struct perf_file_section *section __maybe_unused,
 			   struct perf_header *ph, int fd,
 			   void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	char *str;
 	u32 nr, i;
 	struct strbuf sb;
@@ -1858,7 +1858,7 @@ static int process_cpu_topology(struct perf_file_section *section __maybe_unused
 				struct perf_header *ph, int fd,
 				void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	u32 nr, i;
 	char *str;
 	struct strbuf sb;
@@ -1914,7 +1914,7 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
 				 struct perf_header *ph, int fd,
 				 void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	u32 nr, node, i;
 	char *str;
 	uint64_t mem_total, mem_free;
@@ -1974,7 +1974,7 @@ static int process_pmu_mappings(struct perf_file_section *section __maybe_unused
 				struct perf_header *ph, int fd,
 				void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	char *name;
 	u32 pmu_num;
 	u32 type;
@@ -2534,7 +2534,7 @@ static int check_magic_endian(u64 magic, uint64_t hdr_sz,
 int perf_file_header__read(struct perf_file_header *header,
 			   struct perf_header *ph, int fd)
 {
-	int ret;
+	ssize_t ret;
 
 	lseek(fd, 0, SEEK_SET);
 
@@ -2628,7 +2628,7 @@ static int perf_file_header__read_pipe(struct perf_pipe_file_header *header,
 				       struct perf_header *ph, int fd,
 				       bool repipe)
 {
-	int ret;
+	ssize_t ret;
 
 	ret = readn(fd, header, sizeof(*header));
 	if (ret <= 0)
@@ -2669,7 +2669,7 @@ static int read_attr(int fd, struct perf_header *ph,
 	struct perf_event_attr *attr = &f_attr->attr;
 	size_t sz, left;
 	size_t our_sz = sizeof(f_attr->attr);
-	int ret;
+	ssize_t ret;
 
 	memset(f_attr, 0, sizeof(*f_attr));
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index b0b15e2..4ce146b 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1158,7 +1158,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session,
 	void *buf = NULL;
 	int skip = 0;
 	u64 head;
-	int err;
+	ssize_t err;
 	void *p;
 
 	perf_tool__fill_defaults(tool);
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 28a0a89..9440481 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -151,12 +151,12 @@ unsigned long convert_unit(unsigned long value, char *unit)
 	return value;
 }
 
-int readn(int fd, void *buf, size_t n)
+ssize_t readn(int fd, void *buf, size_t n)
 {
 	void *buf_start = buf;
 
 	while (n) {
-		int ret = read(fd, buf, n);
+		ssize_t ret = read(fd, buf, n);
 
 		if (ret <= 0)
 			return ret;
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index c8f362d..9f6b928 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -253,7 +253,7 @@ bool strlazymatch(const char *str, const char *pat);
 int strtailcmp(const char *s1, const char *s2);
 char *strxfrchar(char *s, char from, char to);
 unsigned long convert_unit(unsigned long value, char *unit);
-int readn(int fd, void *buf, size_t size);
+ssize_t readn(int fd, void *buf, size_t n);
 
 struct perf_event_attr;
 
-- 
1.8.3.1

[tip:perf/core] perf tools: Use correct return type for readn function

[tip-bot for Jiri Olsa]

Commit-ID:  727ebd544f85285a223ecc6a2a57ef90202cdc7b
Gitweb:     http://git.kernel.org/tip/727ebd544f85285a223ecc6a2a57ef90202cdc7b
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Thu, 28 Nov 2013 11:30:14 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 2 Dec 2013 09:22:45 -0300

perf tools: Use correct return type for readn function

Changing readn function return type to ssize_t because read returns
ssize_t not int.

Changing callers holding variable types as well.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1385634619-8129-3-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c  | 18 +++++++++---------
 tools/perf/util/session.c |  2 +-
 tools/perf/util/util.c    |  4 ++--
 tools/perf/util/util.h    |  2 +-
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 1cd0357..3e755f2 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1709,7 +1709,7 @@ static int process_nrcpus(struct perf_file_section *section __maybe_unused,
 			  struct perf_header *ph, int fd,
 			  void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	u32 nr;
 
 	ret = readn(fd, &nr, sizeof(nr));
@@ -1753,7 +1753,7 @@ static int process_total_mem(struct perf_file_section *section __maybe_unused,
 			     void *data __maybe_unused)
 {
 	uint64_t mem;
-	size_t ret;
+	ssize_t ret;
 
 	ret = readn(fd, &mem, sizeof(mem));
 	if (ret != sizeof(mem))
@@ -1822,7 +1822,7 @@ static int process_cmdline(struct perf_file_section *section __maybe_unused,
 			   struct perf_header *ph, int fd,
 			   void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	char *str;
 	u32 nr, i;
 	struct strbuf sb;
@@ -1858,7 +1858,7 @@ static int process_cpu_topology(struct perf_file_section *section __maybe_unused
 				struct perf_header *ph, int fd,
 				void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	u32 nr, i;
 	char *str;
 	struct strbuf sb;
@@ -1914,7 +1914,7 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
 				 struct perf_header *ph, int fd,
 				 void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	u32 nr, node, i;
 	char *str;
 	uint64_t mem_total, mem_free;
@@ -1974,7 +1974,7 @@ static int process_pmu_mappings(struct perf_file_section *section __maybe_unused
 				struct perf_header *ph, int fd,
 				void *data __maybe_unused)
 {
-	size_t ret;
+	ssize_t ret;
 	char *name;
 	u32 pmu_num;
 	u32 type;
@@ -2534,7 +2534,7 @@ static int check_magic_endian(u64 magic, uint64_t hdr_sz,
 int perf_file_header__read(struct perf_file_header *header,
 			   struct perf_header *ph, int fd)
 {
-	int ret;
+	ssize_t ret;
 
 	lseek(fd, 0, SEEK_SET);
 
@@ -2628,7 +2628,7 @@ static int perf_file_header__read_pipe(struct perf_pipe_file_header *header,
 				       struct perf_header *ph, int fd,
 				       bool repipe)
 {
-	int ret;
+	ssize_t ret;
 
 	ret = readn(fd, header, sizeof(*header));
 	if (ret <= 0)
@@ -2669,7 +2669,7 @@ static int read_attr(int fd, struct perf_header *ph,
 	struct perf_event_attr *attr = &f_attr->attr;
 	size_t sz, left;
 	size_t our_sz = sizeof(f_attr->attr);
-	int ret;
+	ssize_t ret;
 
 	memset(f_attr, 0, sizeof(*f_attr));
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index b0b15e2..4ce146b 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1158,7 +1158,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session,
 	void *buf = NULL;
 	int skip = 0;
 	u64 head;
-	int err;
+	ssize_t err;
 	void *p;
 
 	perf_tool__fill_defaults(tool);
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 28a0a89..9440481 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -151,12 +151,12 @@ unsigned long convert_unit(unsigned long value, char *unit)
 	return value;
 }
 
-int readn(int fd, void *buf, size_t n)
+ssize_t readn(int fd, void *buf, size_t n)
 {
 	void *buf_start = buf;
 
 	while (n) {
-		int ret = read(fd, buf, n);
+		ssize_t ret = read(fd, buf, n);
 
 		if (ret <= 0)
 			return ret;
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index c8f362d..9f6b928 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -253,7 +253,7 @@ bool strlazymatch(const char *str, const char *pat);
 int strtailcmp(const char *s1, const char *s2);
 char *strxfrchar(char *s, char from, char to);
 unsigned long convert_unit(unsigned long value, char *unit);
-int readn(int fd, void *buf, size_t size);
+ssize_t readn(int fd, void *buf, size_t n);
 
 struct perf_event_attr;
 

[PATCH 3/7] perf tools: Fine tune readn function

[Jiri Olsa]

Added a 'left' variable to make the flow clearer, and added a debug
check for the return value - returning 'n' is more obvious.

Added small comment for readn.

Original-patch-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/util.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 9440481..6ea0b4a 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -6,6 +6,7 @@
 #endif
 #include <stdio.h>
 #include <stdlib.h>
+#include <linux/kernel.h>
 
 /*
  * XXX We need to find a better place for these things...
@@ -151,21 +152,26 @@ unsigned long convert_unit(unsigned long value, char *unit)
 	return value;
 }
 
+/*
+ * Read exactly 'n' bytes or return an error.
+ */
 ssize_t readn(int fd, void *buf, size_t n)
 {
 	void *buf_start = buf;
+	size_t left = n;
 
-	while (n) {
-		ssize_t ret = read(fd, buf, n);
+	while (left) {
+		ssize_t ret = read(fd, buf, left);
 
 		if (ret <= 0)
 			return ret;
 
-		n -= ret;
-		buf += ret;
+		left -= ret;
+		buf  += ret;
 	}
 
-	return buf - buf_start;
+	BUG_ON((size_t)(buf - buf_start) != n);
+	return n;
 }
 
 size_t hex_width(u64 v)
-- 
1.8.3.1

Re: [PATCH 3/7] perf tools: Fine tune readn function

[David Ahern]

On 11/28/13, 3:30 AM, Jiri Olsa wrote:
> @@ -151,21 +152,26 @@ unsigned long convert_unit(unsigned long value, char *unit)
>   	return value;
>   }
>
> +/*
> + * Read exactly 'n' bytes or return an error.
> + */
>   ssize_t readn(int fd, void *buf, size_t n)
>   {
>   	void *buf_start = buf;
> +	size_t left = n;
>
> -	while (n) {
> -		ssize_t ret = read(fd, buf, n);
> +	while (left) {
> +		ssize_t ret = read(fd, buf, left);
>
>   		if (ret <= 0)
>   			return ret;

handle EINTR errors? no need to fail readn if the system call is 
interrupted.

David

[PATCHv3 3/7] perf tools: Fine tune readn function

[Jiri Olsa]

On Thu, Nov 28, 2013 at 08:19:51AM -0700, David Ahern wrote:
> On 11/28/13, 3:30 AM, Jiri Olsa wrote:
> >@@ -151,21 +152,26 @@ unsigned long convert_unit(unsigned long value, char *unit)
> >  	return value;
> >  }
> >
> >+/*
> >+ * Read exactly 'n' bytes or return an error.
> >+ */
> >  ssize_t readn(int fd, void *buf, size_t n)
> >  {
> >  	void *buf_start = buf;
> >+	size_t left = n;
> >
> >-	while (n) {
> >-		ssize_t ret = read(fd, buf, n);
> >+	while (left) {
> >+		ssize_t ret = read(fd, buf, left);
> >
> >  		if (ret <= 0)
> >  			return ret;
> 
> handle EINTR errors? no need to fail readn if the system call is
> interrupted.

right, how about the v3 below?

thanks,
jirka


---
Added a 'left' variable to make the flow clearer, and added a debug
check for the return value - returning 'n' is more obvious. Also do
not fail on EINTR error.

Added small comment for readn.

Original-patch-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/util.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 9440481..f12eada 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -6,6 +6,8 @@
 #endif
 #include <stdio.h>
 #include <stdlib.h>
+#include <linux/kernel.h>
+#include <errno.h>
 
 /*
  * XXX We need to find a better place for these things...
@@ -151,21 +153,26 @@ unsigned long convert_unit(unsigned long value, char *unit)
 	return value;
 }
 
+/*
+ * Read exactly 'n' bytes or return an error.
+ */
 ssize_t readn(int fd, void *buf, size_t n)
 {
 	void *buf_start = buf;
+	size_t left = n;
 
-	while (n) {
-		ssize_t ret = read(fd, buf, n);
+	while (left) {
+		ssize_t ret = read(fd, buf, left);
 
-		if (ret <= 0)
+		if ((ret <= 0) && (errno != EINTR))
 			return ret;
 
-		n -= ret;
-		buf += ret;
+		left -= ret;
+		buf  += ret;
 	}
 
-	return buf - buf_start;
+	BUG_ON((size_t)(buf - buf_start) != n);
+	return n;
 }
 
 size_t hex_width(u64 v)
-- 
1.8.3.1

Re: [PATCHv3 3/7] perf tools: Fine tune readn function

[David Ahern]

On 11/28/13, 8:43 AM, Jiri Olsa wrote:

> -		if (ret <= 0)
> +		if ((ret <= 0) && (errno != EINTR))
>   			return ret;

I think you want:
if (ret < 0 && errno == EINTR)
	continue;

if (ret <= 0)
	return ret;

You certainly do not want ret < 0 here: ;-)

>
> -		n -= ret;
> -		buf += ret;
> +		left -= ret;
> +		buf  += ret;
>   	}
>
> -	return buf - buf_start;
> +	BUG_ON((size_t)(buf - buf_start) != n);
> +	return n;
>   }
>
>   size_t hex_width(u64 v)
>

[tip:perf/core] perf tools: Fine tune readn function

[tip-bot for Jiri Olsa]

Commit-ID:  838d14520267769648fb2fc2a637107a1d102590
Gitweb:     http://git.kernel.org/tip/838d14520267769648fb2fc2a637107a1d102590
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Thu, 28 Nov 2013 11:30:15 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 2 Dec 2013 09:22:46 -0300

perf tools: Fine tune readn function

Added a 'left' variable to make the flow clearer, and added a debug
check for the return value - returning 'n' is more obvious.

Added small comment for readn.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Original-patch-by: Ingo Molnar <mingo@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1385634619-8129-4-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/util.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 9440481..6ea0b4a 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -6,6 +6,7 @@
 #endif
 #include <stdio.h>
 #include <stdlib.h>
+#include <linux/kernel.h>
 
 /*
  * XXX We need to find a better place for these things...
@@ -151,21 +152,26 @@ unsigned long convert_unit(unsigned long value, char *unit)
 	return value;
 }
 
+/*
+ * Read exactly 'n' bytes or return an error.
+ */
 ssize_t readn(int fd, void *buf, size_t n)
 {
 	void *buf_start = buf;
+	size_t left = n;
 
-	while (n) {
-		ssize_t ret = read(fd, buf, n);
+	while (left) {
+		ssize_t ret = read(fd, buf, left);
 
 		if (ret <= 0)
 			return ret;
 
-		n -= ret;
-		buf += ret;
+		left -= ret;
+		buf  += ret;
 	}
 
-	return buf - buf_start;
+	BUG_ON((size_t)(buf - buf_start) != n);
+	return n;
 }
 
 size_t hex_width(u64 v)

[PATCH 4/7] perf tools: Add writen function

[Jiri Olsa]

Adding 'writen' function as a synchronous wrapper
for write syscall with following prototype:

  ssize_t writen(int fd, void *buf, size_t n)

Returns the number of bytes written on success
or -1 in case of err.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/util.c | 24 +++++++++++++++++++-----
 tools/perf/util/util.h |  1 +
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 6ea0b4a..b1d5376 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -152,16 +152,14 @@ unsigned long convert_unit(unsigned long value, char *unit)
 	return value;
 }
 
-/*
- * Read exactly 'n' bytes or return an error.
- */
-ssize_t readn(int fd, void *buf, size_t n)
+static ssize_t ion(bool is_read, int fd, void *buf, size_t n)
 {
 	void *buf_start = buf;
 	size_t left = n;
 
 	while (left) {
-		ssize_t ret = read(fd, buf, left);
+		ssize_t ret = is_read ? read(fd, buf, left) :
+					write(fd, buf, left);
 
 		if (ret <= 0)
 			return ret;
@@ -174,6 +172,22 @@ ssize_t readn(int fd, void *buf, size_t n)
 	return n;
 }
 
+/*
+ * Read exactly 'n' bytes or return an error.
+ */
+ssize_t readn(int fd, void *buf, size_t n)
+{
+	return ion(true, fd, buf, n);
+}
+
+/*
+ * Write exactly 'n' bytes or return an error.
+ */
+ssize_t writen(int fd, void *buf, size_t n)
+{
+	return ion(false, fd, buf, n);
+}
+
 size_t hex_width(u64 v)
 {
 	size_t n = 1;
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 9f6b928..ce0f73d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -254,6 +254,7 @@ int strtailcmp(const char *s1, const char *s2);
 char *strxfrchar(char *s, char from, char to);
 unsigned long convert_unit(unsigned long value, char *unit);
 ssize_t readn(int fd, void *buf, size_t n);
+ssize_t writen(int fd, void *buf, size_t n);
 
 struct perf_event_attr;
 
-- 
1.8.3.1

[tip:perf/core] perf tools: Add writen function

[tip-bot for Jiri Olsa]

Commit-ID:  bc3a502bc2bc78d03526d6abcc5697aab18d5ae9
Gitweb:     http://git.kernel.org/tip/bc3a502bc2bc78d03526d6abcc5697aab18d5ae9
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Thu, 28 Nov 2013 11:30:16 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 2 Dec 2013 09:22:46 -0300

perf tools: Add writen function

Adding 'writen' function as a synchronous wrapper for write syscall with
following prototype:

  ssize_t writen(int fd, void *buf, size_t n)

Returns the number of bytes written on success or -1 in case of err.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Requested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1385634619-8129-5-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/util.c | 24 +++++++++++++++++++-----
 tools/perf/util/util.h |  1 +
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 6ea0b4a..b1d5376 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -152,16 +152,14 @@ unsigned long convert_unit(unsigned long value, char *unit)
 	return value;
 }
 
-/*
- * Read exactly 'n' bytes or return an error.
- */
-ssize_t readn(int fd, void *buf, size_t n)
+static ssize_t ion(bool is_read, int fd, void *buf, size_t n)
 {
 	void *buf_start = buf;
 	size_t left = n;
 
 	while (left) {
-		ssize_t ret = read(fd, buf, left);
+		ssize_t ret = is_read ? read(fd, buf, left) :
+					write(fd, buf, left);
 
 		if (ret <= 0)
 			return ret;
@@ -174,6 +172,22 @@ ssize_t readn(int fd, void *buf, size_t n)
 	return n;
 }
 
+/*
+ * Read exactly 'n' bytes or return an error.
+ */
+ssize_t readn(int fd, void *buf, size_t n)
+{
+	return ion(true, fd, buf, n);
+}
+
+/*
+ * Write exactly 'n' bytes or return an error.
+ */
+ssize_t writen(int fd, void *buf, size_t n)
+{
+	return ion(false, fd, buf, n);
+}
+
 size_t hex_width(u64 v)
 {
 	size_t n = 1;
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 9f6b928..ce0f73d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -254,6 +254,7 @@ int strtailcmp(const char *s1, const char *s2);
 char *strxfrchar(char *s, char from, char to);
 unsigned long convert_unit(unsigned long value, char *unit);
 ssize_t readn(int fd, void *buf, size_t n);
+ssize_t writen(int fd, void *buf, size_t n);
 
 struct perf_event_attr;
 

[PATCH 5/7] perf tools: Add perf_data_file__write interface

[Jiri Olsa]

Adding perf_data_file__write interface to centralize
output to files. The function prototype is:

  ssize_t perf_data_file__write(struct perf_data_file *file,
                                void *buf, size_t size);

Returns number of bytes written or -1 in case of error.

NOTE Indenting 'struct perf_data_file' members,
     no functional change done.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/data.c |  6 ++++++
 tools/perf/util/data.h | 14 ++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 7d09faf..1fbcd8b 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -118,3 +118,9 @@ void perf_data_file__close(struct perf_data_file *file)
 {
 	close(file->fd);
 }
+
+ssize_t perf_data_file__write(struct perf_data_file *file,
+			      void *buf, size_t size)
+{
+	return writen(file->fd, buf, size);
+}
diff --git a/tools/perf/util/data.h b/tools/perf/util/data.h
index 8c2df80..2b15d0c 100644
--- a/tools/perf/util/data.h
+++ b/tools/perf/util/data.h
@@ -9,12 +9,12 @@ enum perf_data_mode {
 };
 
 struct perf_data_file {
-	const char *path;
-	int fd;
-	bool is_pipe;
-	bool force;
-	unsigned long size;
-	enum perf_data_mode mode;
+	const char		*path;
+	int			 fd;
+	bool			 is_pipe;
+	bool			 force;
+	unsigned long		 size;
+	enum perf_data_mode	 mode;
 };
 
 static inline bool perf_data_file__is_read(struct perf_data_file *file)
@@ -44,5 +44,7 @@ static inline unsigned long perf_data_file__size(struct perf_data_file *file)
 
 int perf_data_file__open(struct perf_data_file *file);
 void perf_data_file__close(struct perf_data_file *file);
+ssize_t perf_data_file__write(struct perf_data_file *file,
+			      void *buf, size_t size);
 
 #endif /* __PERF_DATA_H */
-- 
1.8.3.1

[tip:perf/core] perf tools: Add perf_data_file__write interface

[tip-bot for Jiri Olsa]

Commit-ID:  6f9a317f2a2d4950880ecfa7eea53ed79f85255f
Gitweb:     http://git.kernel.org/tip/6f9a317f2a2d4950880ecfa7eea53ed79f85255f
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Thu, 28 Nov 2013 11:30:17 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 2 Dec 2013 09:22:46 -0300

perf tools: Add perf_data_file__write interface

Adding perf_data_file__write interface to centralize output to files.
The function prototype is:

  ssize_t perf_data_file__write(struct perf_data_file *file,
                                void *buf, size_t size);

Returns number of bytes written or -1 in case of error.

NOTE: Also indenting 'struct perf_data_file' members, no functional
      change done.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1385634619-8129-6-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/data.c |  6 ++++++
 tools/perf/util/data.h | 14 ++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 7d09faf..1fbcd8b 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -118,3 +118,9 @@ void perf_data_file__close(struct perf_data_file *file)
 {
 	close(file->fd);
 }
+
+ssize_t perf_data_file__write(struct perf_data_file *file,
+			      void *buf, size_t size)
+{
+	return writen(file->fd, buf, size);
+}
diff --git a/tools/perf/util/data.h b/tools/perf/util/data.h
index 8c2df80..2b15d0c 100644
--- a/tools/perf/util/data.h
+++ b/tools/perf/util/data.h
@@ -9,12 +9,12 @@ enum perf_data_mode {
 };
 
 struct perf_data_file {
-	const char *path;
-	int fd;
-	bool is_pipe;
-	bool force;
-	unsigned long size;
-	enum perf_data_mode mode;
+	const char		*path;
+	int			 fd;
+	bool			 is_pipe;
+	bool			 force;
+	unsigned long		 size;
+	enum perf_data_mode	 mode;
 };
 
 static inline bool perf_data_file__is_read(struct perf_data_file *file)
@@ -44,5 +44,7 @@ static inline unsigned long perf_data_file__size(struct perf_data_file *file)
 
 int perf_data_file__open(struct perf_data_file *file);
 void perf_data_file__close(struct perf_data_file *file);
+ssize_t perf_data_file__write(struct perf_data_file *file,
+			      void *buf, size_t size);
 
 #endif /* __PERF_DATA_H */

[PATCH 6/7] perf record: Use perf_data_file__write for output file

[Jiri Olsa]

Changing the file output code to use the newly
added perf_data_file__write interface.

No functional change intended.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d93e2ee..1be39e2 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -76,24 +76,17 @@ struct perf_record {
 	long			samples;
 };
 
-static int perf_record__write(struct perf_record *rec, void *buf, size_t size)
+static ssize_t perf_record__write(struct perf_record *rec,
+				  void *buf, size_t size)
 {
-	struct perf_data_file *file = &rec->file;
-
-	while (size) {
-		ssize_t ret = write(file->fd, buf, size);
-
-		if (ret < 0) {
-			pr_err("failed to write perf data, error: %m\n");
-			return -1;
-		}
-
-		size -= ret;
-		buf += ret;
+	struct perf_session *session = rec->session;
+	ssize_t ret;
 
-		rec->bytes_written += ret;
-	}
+	ret = perf_data_file__write(session->file, buf, size);
+	if (ret < 0)
+		return -1;
 
+	rec->bytes_written += ret;
 	return 0;
 }
 
-- 
1.8.3.1

Re: [PATCH 6/7] perf record: Use perf_data_file__write for output file

[Arnaldo Carvalho de Melo]

Em Thu, Nov 28, 2013 at 11:30:18AM +0100, Jiri Olsa escreveu:
> Changing the file output code to use the newly
> added perf_data_file__write interface.
> 
> No functional change intended.

But there is one, if we fail the pr_err() will not be called, will some
warning be emitter later on?

If so, we should make it clear in the changelog. Lemme check by looking
at that code...

There are several places where the return of this function is propagated
and handled with relevant error messages, but at least one, the most
important, doesn't, i.e. this would be the only message the user would
receive, can you verify that?

- Arnaldo
 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-record.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index d93e2ee..1be39e2 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -76,24 +76,17 @@ struct perf_record {
>  	long			samples;
>  };
>  
> -static int perf_record__write(struct perf_record *rec, void *buf, size_t size)
> +static ssize_t perf_record__write(struct perf_record *rec,
> +				  void *buf, size_t size)
>  {
> -	struct perf_data_file *file = &rec->file;
> -
> -	while (size) {
> -		ssize_t ret = write(file->fd, buf, size);
> -
> -		if (ret < 0) {
> -			pr_err("failed to write perf data, error: %m\n");
> -			return -1;
> -		}
> -
> -		size -= ret;
> -		buf += ret;
> +	struct perf_session *session = rec->session;
> +	ssize_t ret;
>  
> -		rec->bytes_written += ret;
> -	}
> +	ret = perf_data_file__write(session->file, buf, size);
> +	if (ret < 0)
> +		return -1;
>  
> +	rec->bytes_written += ret;
>  	return 0;
>  }
>  
> -- 
> 1.8.3.1

[PATCHv3 6/7] perf record: Use perf_data_file__write for output file

[Jiri Olsa]

On Thu, Nov 28, 2013 at 12:03:44PM -0200, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 28, 2013 at 11:30:18AM +0100, Jiri Olsa escreveu:
> > Changing the file output code to use the newly
> > added perf_data_file__write interface.
> > 
> > No functional change intended.
> 
> But there is one, if we fail the pr_err() will not be called, will some
> warning be emitter later on?

ouch right, I missed that one... attached v3 ;-)

> 
> If so, we should make it clear in the changelog. Lemme check by looking
> at that code...

I think it's better we dont change the functionality,
just factor the code..

> 
> There are several places where the return of this function is propagated
> and handled with relevant error messages, but at least one, the most
> important, doesn't, i.e. this would be the only message the user would
> receive, can you verify that?

AFAICS they print out high(er) level errors, like for sythesizing
events:
                pr_err("Couldn't record guest kernel [%d]'s reference"
                       " relocation symbol.\n", machine->pid);

I think we want that write failure message as well

thanks,
jirka


---
Changing the file output code to use the newly
added perf_data_file__write interface.

No functional change intended.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d93e2ee..2402eff 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -76,24 +76,19 @@ struct perf_record {
 	long			samples;
 };
 
-static int perf_record__write(struct perf_record *rec, void *buf, size_t size)
+static ssize_t perf_record__write(struct perf_record *rec,
+				  void *buf, size_t size)
 {
-	struct perf_data_file *file = &rec->file;
-
-	while (size) {
-		ssize_t ret = write(file->fd, buf, size);
-
-		if (ret < 0) {
-			pr_err("failed to write perf data, error: %m\n");
-			return -1;
-		}
-
-		size -= ret;
-		buf += ret;
+	struct perf_session *session = rec->session;
+	ssize_t ret;
 
-		rec->bytes_written += ret;
+	ret = perf_data_file__write(session->file, buf, size);
+	if (ret < 0) {
+		pr_err("failed to write perf data, error: %m\n");
+		return -1;
 	}
 
+	rec->bytes_written += ret;
 	return 0;
 }
 
-- 
1.8.3.1

[PATCH 7/7] perf inject: Handle output file via perf_data_file object

[Jiri Olsa]

Using the perf_data_file object to handle output
file processing.

No functional change intended.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-inject.c | 65 +++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 38 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 6a25085..939999c 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -22,14 +22,13 @@
 #include <linux/list.h>
 
 struct perf_inject {
-	struct perf_tool tool;
-	bool		 build_ids;
-	bool		 sched_stat;
-	const char	 *input_name;
-	int		 pipe_output,
-			 output;
-	u64		 bytes_written;
-	struct list_head samples;
+	struct perf_tool	 tool;
+	bool			 build_ids;
+	bool			 sched_stat;
+	const char		*input_name;
+	struct perf_data_file	 output;
+	u64			 bytes_written;
+	struct list_head	 samples;
 };
 
 struct event_entry {
@@ -42,21 +41,14 @@ static int perf_event__repipe_synth(struct perf_tool *tool,
 				    union perf_event *event)
 {
 	struct perf_inject *inject = container_of(tool, struct perf_inject, tool);
-	uint32_t size;
-	void *buf = event;
+	ssize_t size;
 
-	size = event->header.size;
-
-	while (size) {
-		int ret = write(inject->output, buf, size);
-		if (ret < 0)
-			return -errno;
-
-		size -= ret;
-		buf += ret;
-		inject->bytes_written += ret;
-	}
+	size = perf_data_file__write(&inject->output, event,
+				     event->header.size);
+	if (size < 0)
+		return -errno;
 
+	inject->bytes_written += size;
 	return 0;
 }
 
@@ -80,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
 	if (ret)
 		return ret;
 
-	if (!inject->pipe_output)
+	if (&inject->output.is_pipe)
 		return 0;
 
 	return perf_event__repipe_synth(tool, event);
@@ -355,6 +347,7 @@ static int __cmd_inject(struct perf_inject *inject)
 		.path = inject->input_name,
 		.mode = PERF_DATA_MODE_READ,
 	};
+	struct perf_data_file *file_out = &inject->output;
 
 	signal(SIGINT, sig_handler);
 
@@ -391,14 +384,14 @@ static int __cmd_inject(struct perf_inject *inject)
 		}
 	}
 
-	if (!inject->pipe_output)
-		lseek(inject->output, session->header.data_offset, SEEK_SET);
+	if (!file_out->is_pipe)
+		lseek(file_out->fd, session->header.data_offset, SEEK_SET);
 
 	ret = perf_session__process_events(session, &inject->tool);
 
-	if (!inject->pipe_output) {
+	if (!file_out->is_pipe) {
 		session->header.data_size = inject->bytes_written;
-		perf_session__write_header(session, session->evlist, inject->output, true);
+		perf_session__write_header(session, session->evlist, file_out->fd, true);
 	}
 
 	perf_session__delete(session);
@@ -427,14 +420,17 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
 		},
 		.input_name  = "-",
 		.samples = LIST_HEAD_INIT(inject.samples),
+		.output = {
+			.path = "-",
+			.mode = PERF_DATA_MODE_WRITE,
+		},
 	};
-	const char *output_name = "-";
 	const struct option options[] = {
 		OPT_BOOLEAN('b', "build-ids", &inject.build_ids,
 			    "Inject build-ids into the output stream"),
 		OPT_STRING('i', "input", &inject.input_name, "file",
 			   "input file name"),
-		OPT_STRING('o', "output", &output_name, "file",
+		OPT_STRING('o', "output", &inject.output.path, "file",
 			   "output file name"),
 		OPT_BOOLEAN('s', "sched-stat", &inject.sched_stat,
 			    "Merge sched-stat and sched-switch for getting events "
@@ -456,16 +452,9 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (argc)
 		usage_with_options(inject_usage, options);
 
-	if (!strcmp(output_name, "-")) {
-		inject.pipe_output = 1;
-		inject.output = STDOUT_FILENO;
-	} else {
-		inject.output = open(output_name, O_CREAT | O_WRONLY | O_TRUNC,
-						  S_IRUSR | S_IWUSR);
-		if (inject.output < 0) {
-			perror("failed to create output file");
-			return -1;
-		}
+	if (perf_data_file__open(&inject.output)) {
+		perror("failed to create output file");
+		return -1;
 	}
 
 	if (symbol__init() < 0)
-- 
1.8.3.1

Re: [PATCH 7/7] perf inject: Handle output file via perf_data_file object

[Arnaldo Carvalho de Melo]

Em Thu, Nov 28, 2013 at 11:30:19AM +0100, Jiri Olsa escreveu:
> Using the perf_data_file object to handle output
> file processing.
> 
> No functional change intended.

There is one, before we were using:

-		inject.output = open(output_name, O_CREAT | O_WRONLY | O_TRUNC,
-						  S_IRUSR | S_IWUSR);

And 

+	if (perf_data_file__open(&inject.output)) {

Does:

          return open(file->path, O_CREAT|O_RDWR|O_TRUNC, S_IRUSR|S_IWUSR);

Why do we need to do O_RDWR there? Can we live with plain O_WRONLY as
before?

Also, while reviewing this I got something that I missed in previous
patches, I think we could reuse:

  O_WRONLY to replace the long constant PERF_DATA_MODE_WRITE
  O_RDONLY ditto -> PERF_DATA_MODE_READ

Shorter, and matches what we need here: a open flag value to specify
which access mode the perf.data file is operating, no?

- Arnaldo
 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-inject.c | 65 +++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 38 deletions(-)
> 
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 6a25085..939999c 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -22,14 +22,13 @@
>  #include <linux/list.h>
>  
>  struct perf_inject {
> -	struct perf_tool tool;
> -	bool		 build_ids;
> -	bool		 sched_stat;
> -	const char	 *input_name;
> -	int		 pipe_output,
> -			 output;
> -	u64		 bytes_written;
> -	struct list_head samples;
> +	struct perf_tool	 tool;
> +	bool			 build_ids;
> +	bool			 sched_stat;
> +	const char		*input_name;
> +	struct perf_data_file	 output;
> +	u64			 bytes_written;
> +	struct list_head	 samples;
>  };
>  
>  struct event_entry {
> @@ -42,21 +41,14 @@ static int perf_event__repipe_synth(struct perf_tool *tool,
>  				    union perf_event *event)
>  {
>  	struct perf_inject *inject = container_of(tool, struct perf_inject, tool);
> -	uint32_t size;
> -	void *buf = event;
> +	ssize_t size;
>  
> -	size = event->header.size;
> -
> -	while (size) {
> -		int ret = write(inject->output, buf, size);
> -		if (ret < 0)
> -			return -errno;
> -
> -		size -= ret;
> -		buf += ret;
> -		inject->bytes_written += ret;
> -	}
> +	size = perf_data_file__write(&inject->output, event,
> +				     event->header.size);
> +	if (size < 0)
> +		return -errno;
>  
> +	inject->bytes_written += size;
>  	return 0;
>  }
>  
> @@ -80,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
>  	if (ret)
>  		return ret;
>  
> -	if (!inject->pipe_output)
> +	if (&inject->output.is_pipe)
>  		return 0;
>  
>  	return perf_event__repipe_synth(tool, event);
> @@ -355,6 +347,7 @@ static int __cmd_inject(struct perf_inject *inject)
>  		.path = inject->input_name,
>  		.mode = PERF_DATA_MODE_READ,
>  	};
> +	struct perf_data_file *file_out = &inject->output;
>  
>  	signal(SIGINT, sig_handler);
>  
> @@ -391,14 +384,14 @@ static int __cmd_inject(struct perf_inject *inject)
>  		}
>  	}
>  
> -	if (!inject->pipe_output)
> -		lseek(inject->output, session->header.data_offset, SEEK_SET);
> +	if (!file_out->is_pipe)
> +		lseek(file_out->fd, session->header.data_offset, SEEK_SET);
>  
>  	ret = perf_session__process_events(session, &inject->tool);
>  
> -	if (!inject->pipe_output) {
> +	if (!file_out->is_pipe) {
>  		session->header.data_size = inject->bytes_written;
> -		perf_session__write_header(session, session->evlist, inject->output, true);
> +		perf_session__write_header(session, session->evlist, file_out->fd, true);
>  	}
>  
>  	perf_session__delete(session);
> @@ -427,14 +420,17 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
>  		},
>  		.input_name  = "-",
>  		.samples = LIST_HEAD_INIT(inject.samples),
> +		.output = {
> +			.path = "-",
> +			.mode = PERF_DATA_MODE_WRITE,
> +		},
>  	};
> -	const char *output_name = "-";
>  	const struct option options[] = {
>  		OPT_BOOLEAN('b', "build-ids", &inject.build_ids,
>  			    "Inject build-ids into the output stream"),
>  		OPT_STRING('i', "input", &inject.input_name, "file",
>  			   "input file name"),
> -		OPT_STRING('o', "output", &output_name, "file",
> +		OPT_STRING('o', "output", &inject.output.path, "file",
>  			   "output file name"),
>  		OPT_BOOLEAN('s', "sched-stat", &inject.sched_stat,
>  			    "Merge sched-stat and sched-switch for getting events "
> @@ -456,16 +452,9 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (argc)
>  		usage_with_options(inject_usage, options);
>  
> -	if (!strcmp(output_name, "-")) {
> -		inject.pipe_output = 1;
> -		inject.output = STDOUT_FILENO;
> -	} else {
> -		inject.output = open(output_name, O_CREAT | O_WRONLY | O_TRUNC,
> -						  S_IRUSR | S_IWUSR);
> -		if (inject.output < 0) {
> -			perror("failed to create output file");
> -			return -1;
> -		}
> +	if (perf_data_file__open(&inject.output)) {
> +		perror("failed to create output file");
> +		return -1;
>  	}
>  
>  	if (symbol__init() < 0)
> -- 
> 1.8.3.1

Re: [PATCH 7/7] perf inject: Handle output file via perf_data_file object

[Jiri Olsa]

On Thu, Nov 28, 2013 at 12:14:35PM -0200, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 28, 2013 at 11:30:19AM +0100, Jiri Olsa escreveu:
> > Using the perf_data_file object to handle output
> > file processing.
> > 
> > No functional change intended.
> 
> There is one, before we were using:
> 
> -		inject.output = open(output_name, O_CREAT | O_WRONLY | O_TRUNC,
> -						  S_IRUSR | S_IWUSR);
> 
> And 
> 
> +	if (perf_data_file__open(&inject.output)) {
> 
> Does:
> 
>           return open(file->path, O_CREAT|O_RDWR|O_TRUNC, S_IRUSR|S_IWUSR);
> 
> Why do we need to do O_RDWR there? Can we live with plain O_WRONLY as
> before?

I think the reason is the record's buildid's hunt
during the exit.. we read the file we just stored
without reopening it for reading

hum, how about the output file mmapping David is working on,
that might need file to be readable as well..

> 
> Also, while reviewing this I got something that I missed in previous
> patches, I think we could reuse:
> 
>   O_WRONLY to replace the long constant PERF_DATA_MODE_WRITE
>   O_RDONLY ditto -> PERF_DATA_MODE_READ
> 
> Shorter, and matches what we need here: a open flag value to specify
> which access mode the perf.data file is operating, no?

I guess that would work.. but I planned for future more
perf specific flags like PERF_DATA_MODE_DIR or something
like that.. I haven't this fully thought through yet..

jirka

Re: [PATCHv2 0/7] perf tools: Add data file write interface

[Jiri Olsa]

On Thu, Nov 28, 2013 at 11:30:12AM +0100, Jiri Olsa wrote:
> hi,
> adding perf_data_file__write function to centralize
> output file writes. Using it in record and inject
> commands.
> 
> v2 changes:
>   - changes for readn function, suggested by Ingo
>   - added writen function, suggested by Arnaldo
>   - spliting record change into 2 separated patches
>   - omiting some unnecessary changes in the inject change

available in here:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/data5

jirka